-
-
Notifications
You must be signed in to change notification settings - Fork 21.1k
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.
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
Stop freeing child scene tiles when the TileMap is removed from the tree #92189
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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(); | ||
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); | ||
|
@@ -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. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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: There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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: { | ||
|
@@ -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: { | ||
|
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CC @groud WDYT?
There was a problem hiding this comment.
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.