-
-
Notifications
You must be signed in to change notification settings - Fork 3.8k
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
Migrate bevy_transform
to required components
#14964
Conversation
Missed MaterialMeshletMeshBundle |
oops |
I'm in favor of this. The opposite direction (GlobalTransform -> Transform) is much more intrusive and interferes with our plans to split apart Transform (#1275), but I think that this is nearly universally desirable. |
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.
The file /errors/B0004.md
contains rust doc which uses Transform bundles and should be updated to use this change. (It's why CI is failing). Otherwise, looks good.
Co-authored-by: Alice Cecile <[email protected]>
It seems [the guidance to fix] this error would be obsolete or inconsistent with the required components workflow. Should we block this PR on updating things like this or leave that to a later documentation pass? |
I'm not entirely sure it would be? I don't think required components add components to the parent entities, do they? Either way, this can be a follow-up if it is actually redundant in a post required components world. (Also, the bundle docs seem to be having an issue with |
I don't think I expressed that clearly lol. What I meant was that we're going to need to update a lot of docs anyway for the new workflow, so I was wondering if it might make more sense to do a later pass rather than make a lot of changes to that page just because it referenced |
Fixed the docs. They're (specifically errors/B0004.md) in a bit of a weird half-way state since we're intending to nuke SpatialBundle also, but they do accurately reflect the state of things in between PRs. |
This LGTM, but I'm not going to merge until we have consensus from @cart :) Thanks for tackling this! |
1d9817d
to
91c7d9e
Compare
@ecoskey once CI is passing, please ping me. We're ready to proceed with this work :) See this message from Cart on Discord. |
The first step in the migration to required components! This PR removes `GlobalTransform` from all user-facing code, since it's now added automatically wherever `Transform` is used. ## Testing - None of the examples I tested were broken, and I assume breaking transforms in any way would be visible *everywhere* --- ## Changelog - Make `Transform` require `GlobalTransform` ~~- Remove `GlobalTransform` from all engine bundles~~ - Remove in-engine insertions of GlobalTransform and TransformBundle - Deprecate `TransformBundle` - update docs to reflect changes ## Migration Guide Replace all insertions of `GlobalTransform` and/or `TransformBundle` with `Transform` alone. --------- Co-authored-by: Alice Cecile <[email protected]> Co-authored-by: Tim <[email protected]>
Thank you to everyone involved with the authoring or reviewing of this PR! This work is relatively important and needs release notes! Head over to bevyengine/bevy-website#1699 if you'd like to help out. |
The first step in the migration to required components! This PR removes
GlobalTransform
from all user-facing code, since it's now added automatically whereverTransform
is used.Testing
Changelog
Transform
requireGlobalTransform
- RemoveGlobalTransform
from all engine bundlesTransformBundle
Migration Guide
Replace all insertions of
GlobalTransform
and/orTransformBundle
withTransform
alone.