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.

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

[TileMap] Fix forcing cleanup on exiting tree/canvas #89975

Merged
merged 1 commit into from
Apr 4, 2024

Conversation

AThousandShips
Copy link
Member

@AThousandShips AThousandShips commented Mar 28, 2024

The code relies on that the deferred call is handled after the node exits the tree or canvas, this is because before the end of these notifications call both is_inside_tree and is_visible_in_tree retain the values they had before, so these changes being called immediately makes it not force cleanup properly

This feels like a pretty robust fix, and removes some unnecessary code, like the dedicated destructor flag, with the p_force_cleanup argument doing double duty here, I also moved the pending_update clearing to _internal_update, this avoids going through two other methods to reach the update (update_internals -> _deferred_internal_update -> _internal_update), while still catching any pending updates that might come after

Edit 2: Will write up a 4.2 dedicated version as this can't be cherry-picked directly, will open a PR for that when this is approved
Done, as the solution got a basic go-ahead and will wait on other changes in 4.3


@AThousandShips AThousandShips added bug topic:rendering regression topic:2d cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release labels Mar 28, 2024
@AThousandShips AThousandShips added this to the 4.3 milestone Mar 28, 2024
@AThousandShips AThousandShips requested a review from a team as a code owner March 28, 2024 11:46
@Macksaur
Copy link
Contributor

I tested this on my end, looks good!

@groud
Copy link
Member

groud commented Mar 29, 2024

I prefer we merge #89179 first as it's a bigger change, but that looks like a correct fix to me. We can merge this PR afterwards.

@AThousandShips
Copy link
Member Author

AThousandShips commented Mar 29, 2024

Agreed, just wanted a confirmation that it looks right, I'll rebase this on top of #89179 and fix the differences

Edit: Rebased and works well, will push once the layer expose has been merged

@AThousandShips AThousandShips removed the cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release label Mar 29, 2024
@akien-mga akien-mga merged commit 63135ac into godotengine:master Apr 4, 2024
16 checks passed
@akien-mga
Copy link
Member

Thanks!

@AThousandShips AThousandShips deleted the tile_fix_exit branch April 4, 2024 15:15
@AThousandShips
Copy link
Member Author

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Tilemap light occluders affect all open scenes in the editor
4 participants