Skip to content
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

Move aabb gizmos to bevy_dev_tools #12354

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

chompaa
Copy link
Member

@chompaa chompaa commented Mar 6, 2024

Objective

Done as part of #12351.

Solution

Moved the aabb module out of bevy_gizmos and into bevy_dev_tools.

Migration Guide

Aabb gizmos are no longer used through GizmoPlugin. You must enable the bevy_dev_tools feature instead.

@chompaa chompaa changed the title Move Aabb gizmos to bevy_dev_tools Move aabb gizmos to bevy_dev_tools Mar 6, 2024
@alice-i-cecile alice-i-cecile added M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide A-Gizmos Visual editor and debug gizmos A-Dev-Tools Tools used to debug Bevy applications. labels Mar 6, 2024
Copy link
Contributor

github-actions bot commented Mar 6, 2024

It looks like your PR is a breaking change, but you didn't provide a migration guide.

Could you add some context on what users should update when this change get released in a new version of Bevy?
It will be used to help writing the migration guide for the version. Putting it after a ## Migration Guide will help it get automatically picked up by our tooling.

@@ -1,10 +1,13 @@
//! This crate provides additional utilities for the [Bevy game engine](https://bevyengine.org),
//! focused on improving developer experience.

use bevy_app::prelude::*;
pub mod aabb;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this probably makes mores sense as aabb_gizmo or something here :)

Copy link
Contributor

@pablo-lua pablo-lua left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good, but I was thinking in the future: Should things added to the tools be all behind feature flags so the user can easily disable then? Or maybe we should do the DevToolPlugin into a PluginGroup to make it a easy thing to do? 🤔

@chompaa
Copy link
Member Author

chompaa commented Mar 7, 2024

Unless I'm missing something, I think this is blocked by the fact that bevy_render depends on bevy_dev_tools and e.g. bevy_gizmos depends on bevy_render.

@pablo-lua
Copy link
Contributor

pablo-lua commented Mar 7, 2024

I think we are missing imports on cargo, we might want to add to the imports bevy_render and bevy_color (and other needed crates)

@chompaa
Copy link
Member Author

chompaa commented Mar 7, 2024

I think we are missing imports on cargo, we might want to add to the imports bevy_render and bevy_color (and other libs crates)

Adding bevy_render causes a circular dependency since bevy_render has bevy_dev_tools in its dependencies. A similar issue happens when adding bevy_gizmos, since it depends on bevy_render.

@pablo-lua
Copy link
Contributor

pablo-lua commented Mar 7, 2024

bevy_render has bevy_dev_tools

Understood. Looks like we are using it in render to add tests when dev_tools feature is on, and even use some types from it, what a pain

Copy link
Contributor

github-actions bot commented Mar 7, 2024

You added a new example but didn't add metadata for it. Please update the root Cargo.toml file.

@mgi388
Copy link
Contributor

mgi388 commented Mar 7, 2024

In the spirit of Cart's comment on the main bevy_dev_tools PR, why is bevy_dev_tools a better place for this than bevy_gizmos? There's probably more context in the main PR and Discord, but it seems like at very least this PR and #12351 could do with a strong written reason that it's a good idea.

As an example for why this jumps out to me as not clear cut: Couldn't you build a game that has AabbGizmoPlugin shipped to end users i.e. the plugin is not just for "dev tools"? Maybe it's enough to say that gizmos are 99% of the time only for devs and not end users, just a thought. In light of Cart's comment, and looking at the current list in #12351, could the same argument hold for a bunch of camera stuff? i.e. you could ship them to end users, and not just use them as "dev tools". Maybe the 1% here isn't enough to justify my thinking, and if so, all good!

I'd say you all know what you're doing and this is a good idea :), but it could be useful to document strong reason this is a good move in separate PRs that do the change, for posterity, etc. It would also help make sure each PR is directly thinking back to Cart's comment to make sure it's not a grab bag 🤔

@alice-i-cecile
Copy link
Member

Great question! My stance on "what is bevy_dev_tools":

  1. I'd like a unified set of tools that can be used to ease the development of Bevy games, primarily by displaying information about the state of the app.
  2. These tools should be easy to discover.
  3. These tools should have a unified style and use shared idioms and code.
  4. These tools should be able to be toggled on and off by end users in a number of convenient ways using a consistent pattern (e.g. editor command palette, dev console, and so on).

The fact that users could use these tools in a final product doesn't negate the fact that they're primarily going to be used for development: there's nothing stopping you from enabling dev tools in your final build but only turning on the key features you need :)

@mgi388
Copy link
Contributor

mgi388 commented Mar 7, 2024

I guess in my hypothetical example, if the real game needs the Aabb gizmo for an end-user mechanic (again, hypothetical, maybe too contrived), then the game dev can still use the AabbGizmoPlugin: it's just that it comes off bevy_dev_tools, not bevy_gizmos. And if solving for the 99% case makes more sense for it to be in the first one, not the second, then I suppose that's fine isn't it, and there's no problem here.

Out of interest, is there any reason that stuff has to live inside bevy_dev_tools and bevy_dev_tools can't just be a place for all the dev tools to come together in a unified fashion, toggleable, etc.? I note the first 3 items in your "stance" above seem to be to do with the goal of creating a crate to serve a specific purpose (dev tools) rather than gluing stuff together from around the engine. Perhaps "gluing stuff together" (as I'm devil's advocating for) has just historically not worked for Bevy, and then this circles back to why this whole move/push is happening.

@afonsolage
Copy link
Contributor

Isn't bevy_dev_tools the new bevy_utils? I mean, the whole bevy is a dev tool. I'm worried we are increasing the scope of bevy_dev_tools to later on have to reduce its scope, like bevy_utils right now.

I would prefer a bevy_gizmos crate rather than moving to something which is generic, like bevy_dev_tools.

@pablo-lua
Copy link
Contributor

pablo-lua commented Mar 7, 2024

Well, the initial proposal was to have a canonical place to put the tools that we we'll for sure do in our way to make the editor happen. I think thebevy_dev_tools meaning is to be used internally by bevy_editor in the future. At the same time, there is some things that are placed here and there in the engine and could use of a better place to put them, like Aabb gizmos itself, which is a dev tool for sure (which doesn't block you from shipping if you really want, of course). This aabb thing doesn't change anything in gizmos: it's just a tool, meaning to be used as it, and IMO aabb gizmos and light gizmos are flashy because they are the only ones that add plugins to the App instead of methods to the gizmos: ArrowBuilder is a example here that can explain how them are different.

@alice-i-cecile alice-i-cecile added the X-Controversial There is active debate or serious implications around merging this PR label Mar 7, 2024
@@ -35,10 +38,12 @@ pub mod ci_testing;
pub struct DevToolsPlugin;

impl Plugin for DevToolsPlugin {
fn build(&self, _app: &mut App) {
fn build(&self, app: &mut App) {
app.add_plugins(AabbGizmoPlugin);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think, for now, we should hide it behind a feature and change it later when we create some other abstraction for toggling specific tools

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree. We have to create this abstraction soon, though, the amount of created features can be very high if we don't handle with care.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can do DevToolsPlugin into a PluginGroup, to make it easy disable and enable things, but I'm not sure how this will play with DefaultPlugins though

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've thought about it too, but wouldn't it then be impossible to change dev tools during runtime? I also thought about making a config as a resource but there might be a better way

Copy link
Contributor

@pablo-lua pablo-lua Mar 7, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think there is a way at disabling this at runtime right now, at least not in the way we are currently doing
we add plugins to the app after all, is there a way to currently disabling a plugin, adding a plugin, or even adding systems at runtime right now? If we want, we can at least make it into a group now and mitigate the effects I guess, at least while we don't have the runtime tool manager

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes we can't add/remove plugins at runtime (see #11083), but maybe we could achieve something without plugins? I think we should create an issue regarding this to decide which direction we want to go for now

@chompaa
Copy link
Member Author

chompaa commented Mar 11, 2024

Once some dev tool abstraction is added (i.e. #12427) I'll update this PR.

@pablo-lua pablo-lua added the S-Blocked This cannot move forward until something else changes label Apr 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Dev-Tools Tools used to debug Bevy applications. A-Gizmos Visual editor and debug gizmos M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide S-Blocked This cannot move forward until something else changes X-Controversial There is active debate or serious implications around merging this PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants