-
-
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
Merged
+21
−18
Merged
Changes from all commits
Commits
Show all changes
2 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,6 @@ | ||
| --- | ||
| title: "Light gizmos have been moved from `bevy_gizmos` to `bevy_light`" | ||
| pull_requests: [22583] | ||
| --- | ||
|
|
||
| If you were importing `LightGizmoPlugin`, `LightGizmoColor`, `LightGizmoConfigGroup`, or `ShowLightGizmo` from `bevy_gizmos::light`, they have been moved to live in `bevy_light` now. |
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.
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:
DefaultPluginsLightPluginGizmoPluginLightGizmoPluginNow it is:
DefaultPluginsLightPluginLightGizmoPluginGizmoPluginFrom 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
GizmoPluginis more of a "provides gizmo functionality and primitives", I'd probably expect that it be registered beforeLightPlugin(and before an imaginaryAudioPluginthat also provides gizmos viaAudioGizmoPlugin), but again, not sure that there's any issue right now so probably fine to leave as is.Alternatively, inside
LightGizmoPluginyou could add theGizmoPluginif it's not already added. This comment speaks to a user who is usingbevy_lightdirectly but notbevy(and therefore notbevy_internal)—will their light gizmos work if they don't addGizmoPluginthemselves?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.