-
-
Notifications
You must be signed in to change notification settings - Fork 4.4k
invert bevy_gizmos and bevy_light dependency #22583
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
Conversation
|
It looks like your PR is a breaking change, but you didn't provide a migration guide. Please review the instructions for writing migration guides, then expand or revise the content in the migration guides directory to reflect your changes. |
|
Very good, but needs a migration guide for the moved code. |
mgi388
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.
Also tested the 3d_gizmos example. My comment is just food for thought, I don't think there's anything blocking there unless you think otherwise @atlv24.
| ); | ||
|
|
||
| #[cfg(feature = "bevy_gizmos")] | ||
| app.add_plugins(gizmos::LightGizmoPlugin); |
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.
Just want to note that previously the order of plugins being added was:
DefaultPlugins- adds
LightPlugin - adds
GizmoPlugin- adds
LightGizmoPlugin
- adds
- adds
Now it is:
DefaultPlugins- adds
LightPlugin- adds
LightGizmoPlugin
- adds
- adds
GizmoPlugin
- adds
From looking at the plugins I cannot see any dependency that would cause an issue so I think this is fine, just wanted to note.
I suppose technically if GizmoPlugin is more of a "provides gizmo functionality and primitives", I'd probably expect that it be registered before LightPlugin (and before an imaginary AudioPlugin that also provides gizmos via AudioGizmoPlugin), but again, not sure that there's any issue right now so probably fine to leave as is.
Alternatively, inside LightGizmoPlugin you could add the GizmoPlugin if it's not already added. This comment speaks to a user who is using bevy_light directly but not bevy (and therefore not bevy_internal)—will their light gizmos work if they don't add GizmoPlugin themselves?
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.
this is a case we don't cater for in general; there are many many crates that depend on other crates but dont add the plugins. This is because plugin ordering gets very gnarly if you do this - its not actually the case that if plugin A depends on another plugin B that means B must be added before A, look at the web assets plugin for example, among others.
If a user is not using DefaultPlugins, they're doing something highly custom and are a fringe user, and "doing stuff for them" is not something they care about, especially if it can get in their way imo. They can add the plugin manually and order them however they like manually, and test to make sure it works for them in whatever they're doing.
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 other half of it is its just hard to do that, you'd need to be checking if a plugin is already added before adding it in every place and also have gates for this in the DefaultPlugins, and you'll need to have the crates somehow know that both are being used without having a parent crate (bevy_internal) to propagate features between them so they know.
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.
That all makes sense. Thanks for the consideration and thoughts. Mostly just wanted to call out the plug-in order was different just in case, but seems a non issue.
Objective
Solution
Testing
3d_gizmos runs and looks fine