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.

Sign up for GitHub

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

Jump to bottom

Stop freeing child scene tiles when the TileMap is removed from the tree #92189

Closed
wants to merge 1 commit into from
Closed

Stop freeing child scene tiles when the TileMap is removed from the tree #92189

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 6 additions & 6 deletions scene/2d/tile_map_layer.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1195,15 +1195,15 @@ void TileMapLayer::_navigation_draw_cell_debug(const RID &p_canvas_item, const V

void TileMapLayer::_scenes_update(bool p_force_cleanup) {
// Check if we should cleanup everything.
bool forced_cleanup = p_force_cleanup || !enabled || !is_inside_tree() || tile_set.is_null();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Disabling a layer should still remove the nodes I think.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, should disabling a layer remove the nodes? Or just disable them?

I'm imagining a scenario where you disable/re-enable a layer. You'd probably want your scene nodes to not get freed/re-instantiated.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Disabling a layer makes it invisible and non-interactable, so the same should apply to nodes. If you want to achieve that while keeping the nodes' state, the solution could be temporarily removing them from scene tree and re-adding once the layer is enabled. It sounds complex to implement though.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, not only that, but it also seems like one should still be able to find/access/modify the nodes in the Layer subtree even if the Layer is disabled.

This is a more impactful change, but one solution might be:

  1. Parent tile scene nodes to the Layer instead of the TileMap
  2. Now, I think setting Node.ProcessMode and CanvasItem.Visible on the Layer cover the exact functionality that Layer.enabled does

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CC @groud WDYT?

Copy link
Member

@KoBeWi KoBeWi May 21, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we could go with removing for now, just to fix the regression, and improve it in a follow-up. Changing behavior of layer disabling wouldn't count as bugfix.

bool forced_cleanup = p_force_cleanup || !enabled || tile_set.is_null();

if (forced_cleanup) {
// Clean everything.
for (KeyValue<Vector2i, CellData> &kv : tile_map_layer_data) {
_scenes_clear_cell(kv.value);
}
} else {
if (_scenes_was_cleaned_up || dirty.flags[DIRTY_FLAGS_TILE_SET] || dirty.flags[DIRTY_FLAGS_LAYER_IN_TREE]) {
if (_scenes_was_cleaned_up || dirty.flags[DIRTY_FLAGS_TILE_SET]) {
// Update all cells.
for (KeyValue<Vector2i, CellData> &kv : tile_map_layer_data) {
_scenes_update_cell(kv.value);
Expand Down Expand Up @@ -1691,8 +1691,8 @@ void TileMapLayer::_notification(int p_what) {

case NOTIFICATION_EXIT_TREE: {
dirty.flags[DIRTY_FLAGS_LAYER_IN_TREE] = true;
// Update immediately on exiting, and force cleanup.
_internal_update(true);
// Update immediately on exiting, but do not force cleanup.
Copy link
Member

@AThousandShips AThousandShips May 21, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why this change? The force part is necessary, please explain the change as it affects far more than just the scenes, because I wrote this code and the forced update is required, so please detail how you established that this is a necessary change and that it won't cause issues

I explicitly added the forced part because things didn't work correctly without it (otherwise I'd not have gone through all that work to add it everywhere):

This change reintroduces:

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I took a look at your repro with the light occluder, and it looks like while force cleaning the atlas tiles did solve it, scene tiles never had that issue, and should not have been changed to force clean when the TileMap is removed from the tree.

Force cleaning the scene tile nodes when the TileMap is removed from the tree erases any state those nodes have accumulated since being instantiated and causes them to be reloaded in their default state when the TileMap is eventually re-added to the tree.

For a solution that solves both issues, I wish I could just take the force_cleanup out of _scenes_update, but that causes other problems. Now that the scene tile nodes haven't been wiped out, the rest of _scenes_update tries to access them in a bad non-deferred way (which is part of why I originally wanted to switch to use _queue_internal_update).

To do this right, I think perhaps I will either have to split up the atlas/scene tile update code paths a bit more, or split up the force_clean/update code paths a bit more.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It did so before so it wasn't changed by this, it was restored to how it was done before the thread fixes, my PR restored the behavior before that that had been done for a long time, so this change shouldn't be made like this, please restore it and we can work with a solution down the line, but this change is invalid

Again, scene tiles were deleted when exiting the tree for a long time, but that behavior was accidentally removed for a short while after thread fixes were applied, the reason for the forced cleanup is that is_inside_tree returns false when exiting the tree, so it worked because the deferred call. But the deferred call was thread unsafe

The problem is that there'd be no way to solve this without failing to remove the scene tiles even when cleaning up at destruction, since this was the behavior previously I'd say it's invalid to change it this way, unless it's safe to not force cleaning up when deleting the node itself

But it should be done without touching the forced cleanup of other parts

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The fact that the updates break things if the force is delayed implies this is a more involved problem and that the clearing of the tiles is assumed and built into the process IMO, I'm not convinced it's a bug, but a possible limitation that might need more workarounds and other solutions, and at least might not be appropriate for the 4.3 release given that it risks causing bugs and issues

For the forced part a simple solution would be to add two arguments, or to restore the old variable tracking if we're in the destructor and free them only then, but I'm not sure it's trivial to implement this change without a lot of other changes, all making this fragile

But I think we should wait for the opinion of groud on this

But I feel that using scene tiles should be adjusted to adapt to this, by having them handle states in a way that handles this, rather than keeping them around, it makes a lot of processing with the tiles more manageable I think

We could also implement some way to track and retain state, but I don't know that scene tiles being stateful is something that's guaranteed to be supported, we don't allow providing per-tile options to them for example, there's a proposal for that I believe

Copy link
Author

@Gerfalerf Gerfalerf May 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Definitely, I recognize the change is invalid as-is, and I'm not proposing that we submit it yet. I super don't want to re-introduce the issue that you linked.

It sounds like you're saying that you think maybe scene tile nodes should be deleted and re-instantiated when the TileMap exits/re-enters the tree. If that is true, then you're right that this is not a bug, and we can abandon trying to fix this PR.

However, that seems at odds with Godot's tree model to delete and re-instantiate those nodes when their parent is simply added/removed. Happy to wait for others' opinions on this as well.

If we aren't going to fix this issue, I think this behavior should be documented here, and that the child nodes that are created should maybe be changed to be "Internal" children of the TileMap.

Copy link
Member

@AThousandShips AThousandShips May 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

However, that seems at odds with Godot's tree model to delete and re-instantiate those nodes when their parent is simply added/removed. Happy to wait for others' opinions on this as well.

That's arguably different, these aren't normal node children but internally generated nodes, handled by the node itself.

But let's see what the maintainers for this part says on the desired behavior, or the limitations we should follow. The solution should handle things safely so must be adjusted to the change here, so depends on the assumptions that was made for the rest of the code, which seems to be that the scenes should be removed on exiting the tree, and the question is if that's desired or not, but the behavior is not fully accidental, it might be a case of copy-pasting some of the code, but the condition for deletion is explicit.

Can you point to a time in the history of this node where it didn't delete the tiles when exiting the tree? It did after #67330

_internal_update(false);
} break;

case TileMap::NOTIFICATION_ENTER_CANVAS: {
Expand All @@ -1702,8 +1702,8 @@ void TileMapLayer::_notification(int p_what) {

case TileMap::NOTIFICATION_EXIT_CANVAS: {
dirty.flags[DIRTY_FLAGS_LAYER_IN_CANVAS] = true;
// Update immediately on exiting, and force cleanup.
_internal_update(true);
// Update immediately on exiting, but do not force cleanup.
_internal_update(false);
} break;

case TileMap::NOTIFICATION_VISIBILITY_CHANGED: {
Expand Down
Loading