-
-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
Add bevy_dev_tools crate #11341
Add bevy_dev_tools crate #11341
Conversation
Added the I'm going to wait on @cart's sign-off before merging this, but I think we're starting to get a clearer vision of what this crate might look like and can unblock editor-adjacent work now. |
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.
I think we can make the docs clearer, by focusing on a well-defined niche. That's non-blocking though, and I think this is the right step at this time.
Not necessarily, but I think we should have something in those tools before merging. You could try moving https://github.com/bevyengine/bevy/blob/main/crates/bevy_app/src/ci_testing.rs |
I'm fine to move that into here as part of the initial PR. There's a number of other things I want to move and add, but this is a good uncontroversial first option. |
Probably, we also want an option to enable only some tools 1st solutionWe remove 2nd solutionWe create a resource to store information about dev tools (see DiagnosticsStore). There are a few reasons why I think this is better:
I believe this could be done in a follow-up PR, to make it easier for review. |
Yep, IMO we add a few tools first, then start thinking about how to abstract them. I'd prefer to have it runtime configured too. |
afd5663
to
ea86fe0
Compare
Just left some thoughts in this thread: https://github.com/bevyengine/bevy/pull/11341/files#r1459692523 |
#12305 is a feature that fits the best in |
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.
A few changes requested, mainly with the bump to 0.14.0-dev.
So, IMO:
If there's no movement on this PR in the next week or so I'll adopt it and get it through :) |
If we want more tools here, maybe we can move Aabb gizmos here, or merge some tools that are not merged yet, like #11237, and move here. I think gizmos has a lot of tools hanging out there that can be moved. |
Co-authored-by: Alice Cecile <[email protected]>
I've rebased onto main |
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.
I'm happy with where this landed and how it is communicated to users in the documentation. Still slightly worried about the "grab bag" nature of this. We should pay careful attention to what ends up in here and consider spinning specific features out into their own crates as they mature / expand.
# Objective - #11341 broke running code using `MinimalPlugins` in CI ## Solution - include `DevToolsPlugin` in `MinimalPlugins`
# Objective - Resolves bevyengine#11309 ## Solution - Add `bevy_dev_tools` crate as a default feature. - Add `DevToolsPlugin` and add it to an app if the `bevy_dev_tools` feature is enabled. `bevy_dev_tools` is reserved by @alice-i-cecile, should we wait until it gets transferred to cart before merging? --------- Co-authored-by: Alice Cecile <[email protected]> Co-authored-by: BD103 <[email protected]>
# Objective - bevyengine#11341 broke running code using `MinimalPlugins` in CI ## Solution - include `DevToolsPlugin` in `MinimalPlugins`
# Objective Fixes typo by #11341. Functionally doesn't change anything other than naming consistency and stop IDE's from screaming at you.
Objective
Solution
bevy_dev_tools
crate as a default feature.DevToolsPlugin
and add it to an app if thebevy_dev_tools
feature is enabled.bevy_dev_tools
is reserved by @alice-i-cecile, should we wait until it gets transferred to cart before merging?