ivan-mogilko
commented
1 year ago So, thinking more about this...
The property reset issue is connected with the issue of validating this property on assignment. Because the Graphic property is not a pointer (sadly) but an integer ID, there's no way to know where the value came from. We may only know if it refers to a real sprite ID at the moment of assignment, but we cannot know if that was user's intention. To put it simply: if user script generates a random number, and that number matches one of the existing sprites (dynamic or normal), then this number is a valid sprite ID value but received with a invalid method.
I came to a belief that for the engine it only makes sense to either validate this property on both cases (assignment and when sprite is removed) or not at all. Because if a property is reset with sprite's deletion, but is not validated on assignment, then this may lead to inconsistent behavior. Say, user uses the property for non-standard purpose, like a "tag" of sorts, then a property might loose its value unexpectedly if it happens to match an ID of a real sprite, that was created and deleted a while later.
There's an opposite problem: if I'd like to write down information that a sprite was assigned to a game object, in order to optimize the process of sprite's deletion, then this is not going to work reliably if a user might just assign arbitrary number to Graphic property, that will later match certain dynamic sprite's ID.
tl;dr - it's either resetting in both cases (resetting property on assignment if the value is invalid too) or not at all.
On another hand, let's assume that we don't validate or reset the Graphic property. This lets us avoid a slow process of checking every game object to see if a sprite is assigned to them. But this also means that it won't be reliable to detect when a dynamic sprite is assigned to an object through a Graphic property.
What is still necessary to update, to force game redraw things / update textures?
Here's a list of things that are checked on both dynamicsprite's update and deletion:
- shared textures;
- character/object cache;
- marking GUI, GUI controls and Overlays for update.
In the case of the shared texture it's quite fast to find out that the sprite was connected, because otherwise there will be no texture with the matching ID. In case of the cache... likely it's possible to remember that a dynamic sprite was referenced when saving a cache with a new sprite ID. But this has to be clarified. Also, testing whole cache is not extremely slow, since that's only characters and current room objects, but it's still better to find a way to skip that if a sprite was not assigned anywhere.
In case of GUIs and Overlays, that may be more complicated. Need to figure out if Marking them for update directly may be avoided. In theory this may be worked around by also having a "cache" for them, in other words having a list of sprite IDs per each GUI, Control and Overlay, where we save last used sprite and reset these would the sprite be changed, similar to how it's done for characters/objects now. In case of overlays, perhaps checking and marking a texture would be enoug; but that won't work for GUIs because they method of drawing is still complex. So, this is something that has to be investigated.
messengerbag
This is written after #1880, and doing small experiment as noted in these couple of comments.
The problem is following. The DynamicSprites are not assigned to game objects using a pointer, but instead using their IDs, just like the regular sprites.
One consequence of that is that when the dynamic sprite is deleted, the objects still keep their ID in their properties. Also as soon as ID becomes unused, it may later be assigned to another created dynamic sprite. Thus, if object's sprite property remains (e.g. if user forgot to reset/update it in script), then it may point at non-existing sprite, or a completely unrelated sprite.
Historically AGS did not reset Graphic property for all of the game objects, it only reset it for RoomObjects and Buttons (afaik). I thought to make this consistent and also added everything else that may reference a sprite to this list (I think this was done around 3.5.1, or maybe 3.6.0), but this of course slows things down considerably when alot of dynamic sprites are created and deleted consequently during a single game frame, because the current algorithm is very dumb and simply checks every possible sprite property in every single object in game's memory. In an ordinary adventure game the primary culprit will be Views, because this algorithm must check every single view frame for its graphic. If a game creates alot of Overlays, then they may become the main slowdown factor for this case.
So, indeed, this was a rather stupid thing to do.
The silly thing here is that above does not really cover all the situations where Graphic property has a wrong value, as currently nothing prevents user from assigning a non-existent sprite ID to a Graphic property (and I believe that will cause engine to crash badly).
There are already real games where this caused a slowdown, this was reported by a user to me. Mostly this was because of a suboptimal script that created too many dynamic sprites, but this means a engine regression nevertheless. Hence I'd like to fix this situation in 3.6.1.
I have some ideas on how this could be improved, but first of all we must decide on what is required at all. Trying to summarize, there are following related questions here:
Do we have a logical necessity to reset Graphic property when a DynamicSprite is deleted? By "logical" I mean - if we forget of technical issues, such as potentially trying to get non-existant sprite using saved ID and getting null pointer instead, - as these may have a solution of their own. I know one actual problem with not resetting the property: because sprite's ID is freed by a deleted DynamicSprite, next DynamicSprite may get the same ID. If the Graphic property remains, then it will implicitly refer to a different sprite. (Ironically this behavior was utilized in some of the old games, possibly unintended, where the coder recreated dynamic sprite without resetting Graphic, and it still pointed to a new valid sprite.) Strictly speaking, it's a question of responsibility. Do we want users to be fully responsible for updating object properties after deleing a DynamicSprite, and get all kinds of weird consequences if they forget to, or we want the engine to do the automatic fixup for them?
In relation to the above, do we have a logical obligation to prevent setting a wrong value to Graphic property by hand in user script? That is - test if this ID really refers to a sprite, and if not, then do something, like keep old value, reset to 0, issue a warning to the log, etc.
Is it possible to safeguard the program from crashing in case the Graphic property refering a non-existant sprite? I believe that it is, without adding any more overhead. Currently SpriteCache already does checks for the sprite's existance, because it must know when to reload a missing one. I think SpriteCache could simply return a sprite 0, or even some programmatically generated placeholder, in case there's no real sprite, thus preventing engine from receiving a nullpointer, and also making it unnecessary to litter the code with nullpointer checks everywhere where engine wants to check a sprite.
What are the ways to speed up the algorithm of on-screen object updates when the DynamicSprite is updated (transformed, or drawn upon) or deleted? This still has to be done, in order to update the object texture and/or mark object for redraw. I only thought briefly about this, and overall there are two approaches: either map an object to its sprite, or map a sprite to objects that use it. The mapping does not have to be direct. For example, it may be possible to mark the sprite as "assigned to some object" or "some viewframe" as a simple flag, which would narrow down the amount of search necessary to do after sprite update/deletion. This will also let completely avoid doing a search for temporary dynamic sprites that are only used to draw to other surfaces, but not assigned anywhere.