Change the rules of how game objects are updated after sprite's update/deletion

[ivan-mogilko]

Fixes #2104 . Please read the ticket and comments for detailed explanation of the problem.

There are following changes done here.

1. SpriteCache now has a "placeholder" field initialized to a 1x1 transparent bitmap by default. operator [] is ensured to always return this placeholder if an invalid sprite was requested. This is a safeguard against program exceptions. Note that it's still possible to test for sprite's existence explicitly by calling DoesSpriteExist.
   I think it may be feasible to add a method for setting custom placeholder. Possibly a copy of sprite 0 may be assigned as a placeholder. I may add this behavior if someone thinks that's a good idea. At first I thought to just use sprite 0 implicitly, but it's a real possibility that sprite 0 does not exist in game. So I went with a dedicated member instead. After all it's a exception safeguard, so should work regardless of the game data.

2. Deleting dynamic sprite no longer resets Graphic property of objects which had it set, at all. The property values stays as they are, which may result in them referring to non-existing sprite, or another dynamic sprite, would the same ID be assigned to them further.

3. In regards to updating objects on screen after sprite's change or deletion, I implemented a new method that does that. The details may be found in commit called "rewrote dynamic sprite change notification method". In short: for texture-based render updating shared texture seem to be enough. For software mode there's a vector that tells when the sprites were modified. The elements are marked when the sprite is updated or deleted. The game objects test these flags along with other conditions that tell whether object has been modified and should be redrawn. This is done for all objects except for GUI (see below).

NOTE: I found a potential problem with this new method, which is explained in this comment. In short, some object properties or actions may depend on sprite's size, and if they cache sprite's size, or any other value derived from the sprite's size, then the objects also have to be updated as soon as sprite is resized. This may currently occur upon DynamicSprite.Resize, Rotate and ChangeCanvasSize.
Examining current objects, it seems like this is not an immediate problem, as the sprite size is never cached in them (it's only cached in draw caches, but that is still updated properly in this pr). In case this becomes a problem in the future, there remains a solution: track sprite use upon a Graphic property assignment too.

Potential TODO:

1. Support setting placeholder to sprite 0, if it exists in game data?
2. GUI: this is the only major problem left. Because they are still drawn differently, sometimes combining a sprite with their own drawing (and some have >1 sprite, like sliders that have base graphic and a handle graphic), they would need a special approach, which I did not invent yet. For now I left them updated as before: whenever a dynamic sprite is changed or deleted it will run through all guis and controls that may have sprites, and marks those that have this sprite assigned. I've got a feeling that GUIs may also use this "sprites modified" vector somehow, but I left this for the future.

For now I'm interested to know if other contributors could see actual flaws in this approach.

Also, this has to be tested for any performance changes. I guess there should be 2 kinds of tests:

1. Regular test with many drawn objects, just to see that drawing itself did not slow down.
2. Test that creating and deleting a lot of dynamic sprites is not slowing things down more than in earlier versions (prior to 3.5.0 probably).

[ivan-mogilko]

@messengerbag, in regards to crashing the engine in case of user error, curiously, I missed this earlier, but found when testing these changes. There are already 2 checks for invalid sprites when drawing room objects and characters only. So, this pr brings back this old behavior probably, where deleting a sprite and forgetting to reassign a new one will stop the game with error message, like:

There was an error drawing object N. Its current sprite, X, is invalid.

I may leave this for the time being, although I'd wish that such reaction was consistent among all the things.

What will happen for other things in general, is that they will display the "placeholder" instead. Which is currently a 1x1 transparent sprite.

[ericoporto]

I tried most of my games and Dave's game and a few others and they worked ok...

But this PR instantly crashes my Future Flashback with the error below - but I checked and using the acwin.exe from master works fine. I am trying to debug to figure it out what I am doing that causes - it's a complex game...

Edit: debugging Future Flashback in VS I get an early assert error because apparently I am somehow tinting a sprite 5182 at the same time that spriteuse size is 5182 so it's like the sprite is deleted. But spriteset size is indeed 5192 and looking the specific sprite alBitmap I think it's there yet. Anyway, this is not the cause of the error that happens later I think...

[ivan-mogilko]

This breaks some things, need to do more testing.

[ivan-mogilko]

Hmm okay, I did some stupid oversights here, and gui is not updated along with dynamic sprites.

Also, I see now there's probably ways to simplify this further. So I have to redo half of this pr.

UPDATE: I am currently rethinking the whole "sprite was modified" notification part. There are several possible approaches here, and I'm experimenting with them. It's also possible that I need to make few other changes first to make things simpler in code.

[ivan-mogilko]

I did some thinking and code research again, and realized that for the texture-based rendering most of the object checking and marking for update is simply not needed anymore. After introducing shared textures (even before the texture cache, but more so after) the objects will already get updated as soon as you update the texture data itself, or mark it deleted. This refers to characters, room objects and overlays. GUI is different though (see below).

Then there is software drawing mode. My thinking was: so when you have a shared texture, you may mark texture updated, and all objects that use it will receive this update. So why not do something similar in software mode too?
What I tried is a shared notification block, which is a simple struct with just a Modified bool in it. There's a map which stores these "control blocks", and whenever an object draws with a dynamic sprite for the first time, it gets assigned this block from the map.
When a dynamic sprite is deleted or changed, the Modified flag is set in a control block, and so every object that still uses it will read this flag when checking whether a redraw is necessary.

GUI: this is the only major problem left. Because they are still drawn differently, sometimes combining a sprite with their own drawing (and some have >1 sprite, like sliders that have base graphic and a handle graphic), they would need a special approach, which I did not invent yet. For now I left them updated as before: whenever a dynamic sprite is changed or deleted it will run through all guis and controls that may have sprites, and marks those that have this sprite assigned.

I've got a feeling that GUIs may also use this "notification block" somehow, I will think about it for bit more.

Of course this PR must be compared for performance with the master branch.
I guess there should be 2 kinds of tests:

1. Regular test with many drawn objects, just to see that drawing itself did not slow down.
2. Test that creating and deleting a lot of dynamic sprites is not slowing things down more than in old AGS (prior to 3.5.0 probably).

[ivan-mogilko]

Note: this does not need #2123, but merging that first will simplify some code here.

[ivan-mogilko]

I did performance tests, and judging by them, the primary culprit in slowing things down was checking properties and resetting them on sprite deletion. I presume that Views did the most impact, in a regular p&c game. Removing these checks and property reassignments alone already seem to give good results.
And of course, not having to test through all Overlays, which would be very bad in case there are thousands of those.

In comparison to that, disabling searching through all GUIs and marking those containing deleted dynamic sprite for redraw makes an almost unnoticeable effect (maybe couple of % gain in fps, in "unlimited fps" mode).

For the reference: I've been testing a real game scene where around 200+ temporary dynamic sprites were created and deleted per game frame, which contained 90 GUIs and 380 buttons, each of them was checked on each sprite deletion.

So while it would be good to improve this further, and only check the guis that matter, I think it would be already fine even like it is now. If I won't be able to figure out a nice way to handle the gui notification...

NOTE: I prefer to merge this after #2123, because the code will become bit simpler.

[ivan-mogilko]

So, in regards to software mode, I redid back to the variant which uses vector of bools to mark "recently modified sprites". This might be faster to check regularly at a cost of extra memory use. Also it is just simpler code-wise. From what I heard before, the std::vector<bool> is a specialized container which stores elements in bits, meaning its 8 sprites per byte, which in worst case of game with many tens of thousands sprites this will result in kilobytes of extra data. Note that a previously tested "notification block" solution would require at least a size of a pointer (4 to 8 bytes) per a drawn object + something like 2 ints (8 bytes more) per a dynamic sprite drawn at least once. The old variant is temporarily kept here: ivan-mogilko@7c4fcdc
I could not decide which is best or worst.
Hypothetically, the sprite flags vector may be accessed from GUI drawing code, in order to check whether it needs update too. But the problem is that before doing per-element check, GUI checks for a flag called "controls changed" (to avoid checking every control before drawing). So I did not figure out how to update that still. I think I prefer to leave this as is for the time being, and come back to this after a planned refactor is done, and there are clear and distinct "runtime" classes for GUIs (see #2058), then it will be perhaps easier to see which is a more convenient way to "connect" GUI elements with dynamic sprites to receive notifications about latter being modified.
For now, I'm afraid, I'm spending too much time thinking this over and over again...

[ericoporto]

The recent version of this doesn't error in Future Flashback anymore. I tried some other games and they all seem to work well. I tried to compare performance but can't tell any regression - it looks the same? At least on the test games I had. The only one I didn't test was the sandwalker because it's using ags4, but I assume it's not significantly impacted because it doesn't do sprite deletion at any point - afaict.

[ivan-mogilko]

The truth is, this becomes noticeable (at least on systems with a good CPU) only when the game creates and deletes a lot of dynamic sprites at once; but also has alot of objects where these sprites may be present (like lots of characters, or lots of Views with many frames in them). Like in AGD's "Mage's Initiation" there's a particle system for magic spells which may repeatedly create and delete 200+ dynamic sprites each frame (it's not done very efficiently).
In that case I got a jump from like 30 fps to 110 fps with this PR. But I guess that's a very rare situation.

That's also why I don't like to spend much time on this...