Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Script API: add Game.PrecacheSprite() and PrecacheView() #2237

Conversation

ivan-mogilko
Copy link
Contributor

@ivan-mogilko ivan-mogilko commented Nov 20, 2023

This is the last thing i wanted to try adding in 3.6.1.

It's been always a problem in AGS that game developers have little control over the resource management. That is a big topic, and has to be thought through well before writing a good API, but I wanted to add couple of simple temporary methods, acting as a replacement for existing hacks that let precache a number of sprites and fix performance in certain scenes.

  • Game.PrecacheSprite(int sprnum) precaches a single sprite;
  • Game.PrecacheView(int view, int first_loop, int last_loop) precaches sprites and linked sounds for all frames in the given range of loops for the view.

Precaching a sprite in both of these methods does not load only a raw sprite, but also prepares a texture for it, since we have a texture cache now too, and converting sprites to textures may also have an impact on performance.

An important note is that these functions work according to the existing cache rules. That is - cached assets are normally counted towards the cache limit, and if the limit is exceeded, then older objects will be freed. In addition, the sounds are only cached if their individual size does not exceed "stream_threshold" value from ags config.

This means that in the end the result of using these functions also depends on active game config, and this has to be honestly stated.

@ivan-mogilko ivan-mogilko added context: script api context: performance related to improving program speed or lowering system requirements labels Nov 20, 2023
@ericoporto
Copy link
Member

I can't see LockSprite get called, I thought DynamicSprites had to be locked. Additionally, I also thought that the mouse cursor had to be locked - as in it would crash if not, there is/was a comment in the code about this.

It seems, from the comments, this changes the rules so that LockedSprites don't count to the cache limit. What I am concerned is in the Windows build where ram is limited to 2GB, so I guess the cache is more limited, can this impact in some way?

@ivan-mogilko
Copy link
Contributor Author

ivan-mogilko commented Nov 21, 2023

I can't see LockSprite get called, I thought DynamicSprites had to be locked.

They are locked by passing a "external" and "lock" flags to SetSprite call.

Additionally, I also thought that the mouse cursor had to be locked - as in it would crash if not, there is/was a comment in the code about this.

I removed the code that depended on that in commit named "cleaned up mouse cursor caching", it has bit more info in extended description.

It seems, from the comments, this changes the rules so that LockedSprites don't count to the cache limit. What I am concerned is in the Windows build where ram is limited to 2GB, so I guess the cache is more limited, can this impact in some way?

The point of this is to have clear distinction between resource types, and knowing how much memory is reserved for which purpose:

  • cache memory, which is reserved for freely loading and disposing objects;
  • locked memory, which is reserved for objects that must be kept in memory until released;
  • external objects, that are created by user (dynamic sprites).

If locked sprites count towards the normal cache limit, then they theoretically may fill it up, so that caching stops working. This is probably what happened in the Dave's game. On another hand, locking rule currently allows to lock any size of sprites, then what's the point in counting them towards the cache limit?
(Maybe they need their own limit in configuration...)

It's been always like this historically, until I changed it when implementing texture cache. Now I am reverting this back.
Prior to this only player's view and mouse cursors were locked (but never unlocked by mistake). After this PR even them are not locked. Actually nothing from the regular sprites will be locked now, unless someone reintroduces locking of particular views (but then they'd also need to unlock properly when the view changes).

This is not related to dynamic sprites, as these are tagged as "external" objects that are always locked and not a part of the cache's size.

@ivan-mogilko
Copy link
Contributor Author

ivan-mogilko commented Nov 21, 2023

But I need to think more about this, because i also had arguments for changing it earlier. Maybe it's more correct to count locked regular sprites along with the cached ones, if we consider "max sprite cache" setting to define the total allowed size of regular sprites in memory.

@ericoporto
Copy link
Member

ericoporto commented Nov 21, 2023

Well if I understood, currently regular sprites don't get locked simply because the calls that did so got removed. So this doesn't matter much at this time.

Ah, it is necessary to test the Editor too just in case this changes anything there - I think not, but would just check.

Edit: I think it may be relevant to mention somewhere that this API is blocking - like if you ask to cache a view it will try to do so in a single game loop and freeze that frame if it takes too long.

Avoid remembering sprite bitmap pointers in an array. Only use these locally in functions. Removed obsolete bits.
@ivan-mogilko ivan-mogilko force-pushed the 361--precachesprites-in-scapi branch from 43049c1 to 47f2df3 Compare November 25, 2023 18:31
@ivan-mogilko
Copy link
Contributor Author

ivan-mogilko commented Nov 25, 2023

I removed the change to the locked items calculation, but added a comment about this to ResourceCache class.
It's not too hard to change anytime, as this is simply a counter logic inside a class.

I think this has to be rethought when designing a better resource management for AGS, including giving more control to the game developers.

@ivan-mogilko ivan-mogilko force-pushed the 361--precachesprites-in-scapi branch 2 times, most recently from 0bcac63 to a751371 Compare November 27, 2023 03:07
@ivan-mogilko
Copy link
Contributor Author

ivan-mogilko commented Nov 27, 2023

Fixed a bug in PrecacheView (forgot to convert from 1-based view index to 0-based).

Added diagnostic logging.

Example output for precaching that giant "Old Skies" intro animation:

Precache view 352 (loops 0-0) frames = 32, total = 596 ms, average file->mem = 8 ms, bm->tx = 9 ms,
		loaded 0 sounds = 0 ms
	Sprite cache: 70 -> 259270 KB, texture cache: 0 -> 259200 KB
Precache view 353 (loops 0-0) frames = 30, total = 627 ms, average file->mem = 10 ms, bm->tx = 10 ms,
		loaded 0 sounds = 0 ms
	Sprite cache: 259270 -> 502270 KB, texture cache: 259200 -> 502200 KB

@ivan-mogilko ivan-mogilko force-pushed the 361--precachesprites-in-scapi branch from a751371 to 28d9bad Compare November 27, 2023 03:44
@ivan-mogilko
Copy link
Contributor Author

ivan-mogilko commented Nov 28, 2023

Alright, I guess I will merge this, because this works as expected and may even be useful under some circumstances.

@ivan-mogilko ivan-mogilko merged commit b192800 into adventuregamestudio:master Nov 28, 2023
20 checks passed
@ivan-mogilko ivan-mogilko deleted the 361--precachesprites-in-scapi branch November 28, 2023 01:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
context: performance related to improving program speed or lowering system requirements context: script api
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants