Don't free instanced scenes when recreating tiles

[KoBeWi]

Fixes #58034
Fixes #68347

Scenes are no longer re-instantiated when quadrant is recreated, including when setting cells and changing layer properties (modulate etc). Only modifying TileSet will recreate the scenes (although there's high chance they'll just get unlinked at that point...).

To me it's rather weird that the scenes are re-instanced at all. The purpose of scene collections is to put objects on your level. At no point you want them freed and recreated by the TileMap.

That said, not sure if my solution is good. Probably not 🙃

[akien-mga]

CC @groud

[groud]

Hmm, it does not look right to me. How do you know whether or not the packed scene was modified or not ?
If I am not mistaking, this function is also called when the TileSet is modified, which might cause cached scenes to be wrong.

[KoBeWi]

If I am not mistaking, this function is also called when the TileSet is modified, which might cause cached scenes to be wrong.

When exactly this can happen at runtime?

[groud]

When exactly this can happen at runtime?

I guess procedural modifications of the TileSet, which is possible right now.
I guess that the "runtime" check also means that in the editor, the issue of scene "resetting" will still be there, though most of the time they have no tool scripts so that's fine.

To be honest, I am not perfectly happy with that implementation. I think we should make sure somehow that the underlying PackedScene wasn't modified. This, I suppose, should be maybe done by passing an argument to the update functions ?
(This seems the simplest solution IMO)

[KoBeWi]

I guess procedural modifications of the TileSet, which is possible right now.

I reset the instanced scenes on tileset changed, so this is handled.

To be honest, I am not perfectly happy with that implementation. I think we should make sure somehow that the underlying PackedScene wasn't modified. This, I suppose, should be maybe done by passing an argument to the update functions ?

The way I implemented it, only tiles that weren't instantiated previously will instantiate. Not sure what argument should be passed to preserve this behavior. You need to store the used tiles somewhere.

Also not sure what you mean by modified scene. Scenes don't get modified at runtime, unless you use live edit.

I'm also not 100% happy with this implementation (as I noted in the OP), but I don't have much better idea.

[groud]

I reset the instanced scenes on tileset changed, so this is handled.

Ah that's true, my bad.

The way I implemented it, only tiles that weren't instantiated previously will instantiate. Not sure what argument should be passed to preserve this behavior. You need to store the used tiles somewhere.

The idea would be to have a p_tile_set_modified argument in _scenes_update_dirty_quadrants that will bypass the cache if the TileSet resource was updated.
I think ultimately it would allow optimizing quadrants even more, to only update parts of the quadrant that need an update. But well that might be for later.

Also not sure what you mean by modified scene. Scenes don't get modified at runtime, unless you use live edit.

Not the scene itself, but you can replace a scene in the TileSet resource at runtime. For example using tileset.get_source(0).set_scene_tile_scene(id, new_packed_scene). Which is a valid use case IMO (though not the most efficient for sure).

Anyway, I didn't see the cache was cleared when the TileSet was modified, so I think this implementation should be fine in the end. I feel an implementation with the added argument as I suggested could maybe be cleaner, but I am not sure about how much rewrite that would need. So well, as you prefer. :)

[KoBeWi]

The idea would be to have a p_tile_set_modified argument in _scenes_update_dirty_quadrants that will bypass the cache if the TileSet resource was updated.

What does that change exactly? Clearing cache is the same as bypassing it. You need to remove the old node and cache the new one. (although it might change something if the used cells have changed)
Also what about _scenes_cleanup_quadrant()?

tbh I'm not sure how scene tiles are supposed to work. I'd just make them instance and forget, no clearing.

[groud]

What does that change exactly? Clearing cache is the same as bypassing it. You need to remove the old node and cache the new one. (although it might change something if the used cells have changed)

I think it's more about the way the code is organized right now, where a method is dedicated to each type of sources, and I avoided spreading the responsibilities over. So like, I'd rather have the scene cache cleaning the method dedicated to scene tiles rather than having it in the generic _tileset_changed callback. But I don't mind much to be honest, it is more of a cosmetic change.

Even if I didn't have the time to implement it, and to find interesting use cases, I kind of wanted to allow users to implement their own TileSetSource if needed. So I kept things modular just in case. This special handling in the _tileset_changed function kind of goes against this modularity, but since it's not fully implemented anyway, it's not a big deal.

Also what about _scenes_cleanup_quadrant()?

I guess I would have added the same argument.

[akien-mga]

Thanks!

[Minious]

I think this PR made it so that it's not possible to use set_cell with a scene tile if that tile is not empty (already had a value wether it was a regular tile or a scene tile) because all tiles are added to the instantiated_scenes set, not only the scene tiles, and they are also never removed from that set (the set is only emptied when the tileset changes). See the attached MRE, left click doesn't place new tiles on the bottoom layer which already has tiles set while right click works on an empty layer above.
TestPR.zip
Related to #69596