Add TileSetAtlasSource::TileAnimationMode options and allow to shuffle tile animations

[rakkarage]

• to tile animation properties
• when true (default) all animations of that type will start at first frame of animation.
• when false all animations of that type will start at random frame of animation.

sorry i accidentally merged and/or messed up that branch somehow ok to make a new request? sorry. thanks.
replaces: #77255

this adds a sync option that is on by default just above separation
if turned off all animations of that type will start at random frame instead of 0
to address my recent issue and proposal

godotengine/godot-proposals#6856
#57677

thanks.

Bugsquad edit: This closes godotengine/godot-proposals#6856.

[rakkarage]

That position in list of tile properties was arbitrary and maybe not best spot?
Should I move to end or somewhere else... maybe fine?

Not sure I understand the merge error:
"If a diff is shown, it means that your code/doc changes are incomplete and you should update the class reference with --doctool."
Seems to be diffing with self since it was not there before, and I don't see any real diff? idk

I need to run doctool locally? Naw I did not do that last time I changed docs for tween?

Thanks for help.

[Calinou]

That position in list of tile properties was arbitrary and maybe not best spot?
Should I move to end or somewhere else... maybe fine?

The class reference is automatically alphabetically sorted. You cannot influence this order; it's here by design to make diffs easier to read 🙂

[KoBeWi]

Doctool is built into the executable. You just do bin/godot_exec_name --doctool. Running it ensures that everything is in its place. You didn't need it last time, because your change could be easily copy-pasted.

[rakkarage]

thanks i will try to sort my docs to fix the error
but i meant this position in editor, above separation:

ok will make that alphabetic too maybe thats what u mean

[KoBeWi]

I think the checkbox should be at the bottom.

[kleonc]

Currently what's being randomized is only the starting frame, which I don't think is the way to go. Like e.g. for an animation with 2 frames with durations 1 and 100 respectively it would still pick each frame with ~50% probabiility. I suggest changing it to be time based.

For this you could do (more or less) something like real_t animation_offset = sync ? 0.0 : randf() * animation_duration; and keep the loop as it was before, changing only the last argument of canvas_item_add_animation_slice to be such animation_offset.

Besides that, such animation_offset would need to be stored per cell, instead of choosing a new random offset each time such cell is being redrawn (by calling draw_tile).

[rakkarage]

I was originally intending to use the probability values that are already stored and set per tile, like in TileMapEditorTilesPlugin::_pick_random_tile? Perhaps that would be better? Otherwise, probability exists but is meaningless for animation tiles. (PreBug)

[rakkarage]

Sorry for my mistake, not available for all animation tiles, because cannot select all, and already used by Place Random Tile.

[kleonc]

I wonder if adding just a "sync" bool is the way to go. Instead of allowing only synced/random options, it could probably make sense to provide a way to manually set animation offsets for each cell, so e.g. creating animated patterns at will would be possible. Not fully sure about the implementation though, would such offsets need to be stored in the TileData (so they'd be overridable per cell with TileMap._tile_data_runtime_update)? 🤔 cc @groud

[groud]

I think the feature is good and simple enough to be added. I have a few remarks:

• I think "animations sync" is not a great name, it does not make it clear that animations simply start from 0 and loop by default. And like, I would prefer to "enable randomization" instead of "disabling synchronization".
• I think that two options are enough for now. But I think future-proofing it with an enum instead of a bool would not hurt. With the enum names, we can have a bit more space to describe what each mode do.

I would thus suggest to replace the bool by an enum called "animation_mode" (or something like that), with two values:
"Default" and "Random start position". it would make it a bit clearer and would allow for more modes later on.
We could also add other modes, including one that would call a callback on the TileMap (as @kleonc suggests), but I don't think it is needed for now.

Not fully sure about the implementation though, would such offsets need to be stored in the TileData (so they'd be overridable per cell with TileMap._tile_data_runtime_update)? thinking cc @groud

It would require a dedicated callback, as the animation frame is not stored in the TileData object.

[rakkarage]

One small issue I noticed is a kind of flickering when setting lots of random tiles.

• Does not happen for Default Mode animations.
• Looks like it is just updating the nearby tiles.
• I could not figure out how to fix. Changed back to my old frame code to test but still happens.
• Maybe not a big deal or not easy to fix.

Thanks.

[kleonc]

One small issue I noticed is a kind of flickering when setting lots of random tiles.

* Does not happen for Default Mode animations.

* Looks like it is just updating the nearby tiles.

* I could not figure out how to fix. Changed back to my old frame code to test but still happens.

* Maybe not a big deal or not easy to fix.

I've mentioned this in #77257 (comment):

Besides that, such animation_offset would need to be stored per cell, instead of choosing a new random offset each time such cell is being redrawn (by calling draw_tile).

Whenever draw_tile is called again for each such animated tile (it happens when reissuing draw commands for the given quadrant of tiles in TileMap::_rendering_update_dirty_quadrants) then a new animation_offset is being picked, hence the flickering. Once a random offset is picked for the given (cell, tile) pair, it would need to be preserved / not changed to avoid such flickering.

To be clear: it's not editor only issue, as tiles can be placed / changed on runtime too (which would triggers qudrant redrawing / reissuing draw commands). Hence this needs to be solved now, when adding the feature. So the question is how/where such animation offsets should be stored (and whether they should be serialized?:thinking:).

[rakkarage]

Thanks, I changed to an enum and added a HashMap<Vector2, real_t> animation_offset_cache and it is smooth when adding now.

[groud]

This look quite good already, but I am quite concerned by the performance impact the offset cache will have. Using a hashmap is quite memory inefficient and can easily become slow.

What I would suggest to replace that is to make the offset fixed per cell. Basically, that mean that each cell would be assigned a fixed random value, but that would depend only the tile's coordinates (and maybe the TileMap's pointer address, or something like that (basically the this pointer ?).

Basically, you could do something like this (haven't tested):

Array to_hash;
to_hash.push_back(p_position);
to_hash.push_back(int(this));
animation_offset = RandomPCG(to_hash.hash()).randf();

This would ensure a random value, unique for each tile, but that would stay consistent between each call. This should be enough to avoid using a HashMap.

[rakkarage]

Thanks. I tried it and it does work but:

• draw_tile is static, could map it to TileMap too but,
• No access to tileMap since static and not passed in, tried using RID p_canvas_item but changes too often and causes flicker, maybe a way to get the associated node?
  • Ref<Resource> resource = ResourceLoader::get_singleton()->get_resource(rid);
• But then hash() and (lookup?) and "new" Array is being called and recalled for each cell each draw? that is better? should we then cache the hashes? idk :)

I am not even sure it needs to be fixed. Since we are basically just dealing with noise, it is maybe not AS important that every tile has a 'special' number? Just that they all look generally different and preserve that difference through drawing interference?

It is maybe not worth it to "duplicate" the random noise for each tileMap when there is no visible difference? not really special or associated noise... just noise?

I foresee a similar issue for times when the cache 'needs to be regenerated' but luckily it is just noise and cannot even notice it is using an old cache? so those bugs will never be discovered.

I was gonna say the same thing about changing from frame to time but not sure it is the same... maybe noticeable with long delays idk

Is it a bug or is it visual trickery / optimization? :)

Thanks

[groud]

Ah you are right. The static part is the problem and I did not realize that p_position was not the position on the TileMap.

So the cleanest solution to me then is to add an optional argument animation_offset in the static draw_tile, then compute the random value in the TileMap itself. This values could stay to 0 when draw_tile is called from outside the TileMap. Random values are not really needed when tiles are drawn outside of the TileMap.

It is mayb not worth it to "duplicate" the random noise for each tileMap when there is no visible difference? not really special or associated noise... just noise?

I think it might be visible in some cases, where you have multiple tiles on top of each other on the same layer. So I think it should probably be dependent on the TileMap pointer or something.

But then hash() and (lookup?) and "new" Array is being called and recalled for each cell each draw? that is better? should we then cache the hashes? idk :)

Hashing is pretty fast, while the time to access a bin in a hashmap takes a time that is dependent on how big the map is. For big tilemap it might be a problem. hash() is likely safer.

[rakkarage]

Thanks. You are right. I was easily able to see dupes / no dupes just by adding lots to multiple layers.

[groud]

A small nitpick on a variable name, but aside from that, looks good!

[groud]

LGTM! Thanks!

[rakkarage]

One thing I am curious about is if there is a better way do these casts:

VariantCaster<TileSetAtlasSource::TileAnimationMode>::cast(p_value)

Nothing else seems to be using VARIANT_ENUM_CAST like this? there is maybe a better way? Seems to work, idk just curious if anyone knows.

Thanks.

[YuriSizov]

One thing I am curious about is if there is a better way do these casts:

VariantCaster<TileSetAtlasSource::TileAnimationMode>::cast(p_value)

Nothing else seems to be using VARIANT_ENUM_CAST like this? there is maybe a better way? Seems to work, idk just curious if anyone knows.

I don't think there is a significantly better way. Most of the time we only deal with enums represented as integers, so conversion is straightforward. Here we have a variant, so it needs extra work. So this seems appropriate.

[YuriSizov]

Thanks!