-
-
Notifications
You must be signed in to change notification settings - Fork 4.4k
Replace validate_parent_has_component with ValidateParentHasComponentPlugin.
#22675
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
base: main
Are you sure you want to change the base?
Conversation
19663f4 to
f03e861
Compare
|
oooh nicely done. I'm a bit tired tonight so I'll give it an actual review tomorrow, but on scanning the approach looks good to me, and I like that it only uses resources for the "bad cases", which feels aligned with the original implementation. |
f03e861 to
7d8e770
Compare
ChristopherBiscardi
left a comment
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.
One thing to note is I only perform this check in the Last schedule. We could move this check if necessary, but I doubt many users will spawn a child, and then only add the parent's GlobalTransform after PostUpdate (not sure if this will even affect transform propagation though?)
Since GlobalTransform is a required component of Transform I don't think there's a way for this to cause issues unless bevy support is disabled for transform.
iirc this was the issue that boiled down to being an archetype ordering issue in the scene spawning logic, so it would be hard to write a real test for it, but the previous approach was inherently flawed in that way.
I've run some examples locally and was able to use the new plugin to trigger the log on AnimationPlayer in the animated_mesh example (which obviously should not have another player on the parent).
kfc35
left a comment
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.
Looks good to me! Appreciate the thorough documentation
I checked out the branch and ran the test example in the PR; everything looks good and I also get no warnings in the branch versus the two on main about InheritedVisibility and GlobalTransform
release-content/migration-guides/validate_parent_has_component_is_now_a_plugin.md
Outdated
Show resolved
Hide resolved
Co-authored-by: Kevin Chen <[email protected]>
Objective
Solution
Instead of warning in the hook, we instead send a message for the entity if its parent is missing the component. A system later reads these messages and checks again if the parent is missing the component, and only then logs. This is done by:
bevy_appto add all these things to the app.bevy_util, but I didn't want to add to the "kitchen sink".GlobalTransform/InheritedVisibilityto use this plugin instead.Lastschedule. We could move this check if necessary, but I doubt many users will spawn a child, and then only add the parent's GlobalTransform afterPostUpdate(not sure if this will even affect transform propagation though?)Note: the memory usage is proportional to how many of these bad entities you spawn in a single frame.
Testing
On main, this logs two warnings. With this PR, there are no logs! I also verified that omitting attaching the components to the parent still logs the warning.