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

fixed Tree UI control to no longer corrupt child cache on item moving operations #63891

Merged
merged 1 commit into from
Aug 22, 2022

Conversation

derammo
Copy link
Contributor

@derammo derammo commented Aug 3, 2022

Update code for children_cache was corrupting the cache when tree items are moved while the cache has not been built. This would lead to an array index error on subsequent use of the cache (since it would only have one item in it.)

The DEV_ENABLED safety code was agreed with @KoBeWi to stay in the code until refactoring work in this area is completed. It detects the bug fixed here and similar cache corruption. There are no unit tests for this, so this safety code will be useful until the class is again stable.

@Calinou Calinou added this to the 4.0 milestone Aug 3, 2022
Copy link
Contributor

@trollodel trollodel left a comment

Choose a reason for hiding this comment

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

That code in the move_* methods is wrong. Thanks for fixing it.
You cited the scene tree dock in the title, in what case did you trigger a bug using it? I don't think the tree there uses any of the method that creates the cache, nor the move_* methods.

@derammo derammo changed the title fixed scene tree bug corrupting child cache fixed Tree UI control to no longer corrupt child cache on item moving operations Aug 4, 2022
@derammo derammo force-pushed the derammo_tree_cache branch from d6e419d to 4e6c8e0 Compare August 4, 2022 12:25
@derammo
Copy link
Contributor Author

derammo commented Aug 4, 2022

Comment updated. Sorry for snarky response [I blame not enough coffee.] :)

@derammo derammo marked this pull request as ready for review August 4, 2022 12:41
@derammo derammo requested a review from a team as a code owner August 4, 2022 12:41
Copy link
Member

@KoBeWi KoBeWi left a comment

Choose a reason for hiding this comment

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

Looks ok.

I think I originally misunderstood your question about the validation; that code isn't really necessary. But it's ok to keep it.

@akien-mga akien-mga merged commit 9744bec into godotengine:master Aug 22, 2022
@akien-mga
Copy link
Member

Thanks!

@derammo derammo deleted the derammo_tree_cache branch September 15, 2022 10:20
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.

5 participants