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

[3.x] Mark node groups as dirty for every children if parent is moved #61578

Merged

Conversation

YuriSizov
Copy link
Contributor

Backport of #61577.

As much as I wish it for 3.5, it's probably too risky. But maybe 3.5.1?

@YuriSizov YuriSizov added this to the 3.x milestone May 31, 2022
@YuriSizov YuriSizov requested a review from a team as a code owner May 31, 2022 18:31
@akien-mga akien-mga added the cherrypick:3.5 Considered for cherry-picking into a future 3.5.x release label Jun 27, 2022
Copy link
Member

@lawnjelly lawnjelly left a comment

Choose a reason for hiding this comment

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

Agree with @reduz that this has the potential to be overly expensive, especially if there are any calling points that call move_child() in a loop, where this propagation might happen multiple times.

This is similar-ish in concept to the problems in #61929 .

Agree this is probably best merged after 3.5 stable so we can test for performance regressions.

@YuriSizov
Copy link
Contributor Author

YuriSizov commented Jun 28, 2022

Yeah, that's my concern overall, but I think it shouldn't be too bad. This only marks the groups for an update, but update itself happens on the next frame, once per group, I believe. So the number of nodes should not contribute as much as variety of groups. We can also limit what groups we want to update (only processing order related ones?) and mark them always, without propagating.

@YuriSizov
Copy link
Contributor Author

YuriSizov commented Aug 5, 2022

Should be safe to merge now, I guess. Whether or not to push it to 3.5.x, I don't know.

@akien-mga akien-mga merged commit 25c87e9 into godotengine:3.x Aug 5, 2022
@akien-mga
Copy link
Member

Thanks!

@akien-mga akien-mga modified the milestones: 3.x, 3.6 Aug 5, 2022
@YuriSizov YuriSizov deleted the core-fix-events-for-moved-nodes-3.x branch August 5, 2022 17:57
@akien-mga
Copy link
Member

I suggest erring on the side of caution and not cherry-picking this for 3.5.1 (as we'd likely get less testing for 3.5.1 RC 1 as we got for 3.5 RCs and thus little chance to actually spot potential issues in this core code). It does look fairly safe so if it's really needed I'm happy to reconsider but I think our time is better spent making sure that 3.6 gets released fast :)

@akien-mga akien-mga removed the cherrypick:3.5 Considered for cherry-picking into a future 3.5.x release label Aug 30, 2022
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.

3 participants