Abstraction dev tools

[matiqo15]

Objective

• Fixes Abstraction of bevy_dev_tools #12368

Solution

• Create DevTool that holds unique id and state.
• Create DevToolsStore resource that stores new DevTools.
• Make existing tool use new abstraction

[matiqo15]

I don't like this. This will run every time any tool has been changed, but I can't think of a solution to only run if a specific tool has been changed

[pablo-lua]

I think we can relax on that somehow by just making a global system that manages this, based on one-shot systems, I'm not sure how effective this can be do though. At the same time, that would be useless without some kind o cache to store what was changed and what was not.

[matiqo15]

If we had some kind of cache storing what was changed, I don't think there would be any reason for complicating it with the global system. I'd just let the user interact with the cache and use it in a run_if condition.

[alice-i-cecile]

I think this is a fine level of granularity for now: I wouldn't expect DevToolsStore to change regularly at all.

If it becomes a problem we can also cache a Local within the system to short-circuit.

[pablo-lua]

I'm wondering: Why not use a TypeId map instead of Uuid? I think its much more simple.

[matiqo15]

Do we want to identify each dev tool with a different type? We could use plugin struct for it, but what if plugin adds more than one dev tool?

[pablo-lua]

I was expecting to identify every tool with a single config type registered to it, but I see that's a similar approach taken by the gizmos multiple configs, and we aren't storing configs in this map by now, so, with what we have now on this PR, the TypeId doesn't make sense.
I can see number of reasons why offering a config type though. For example, we might want to do more than enable and disable things, we might want this DevToolsStore to store entire configurations in the future, like storing the FpsOverlayConfig here, and let the user refer to it from the TypeId (which can be easily created and so).

[alice-i-cecile]

I finally have the energy to review this: sorry for keeping you waiting! Generally impressed with the polish and thought here, but I think we should try harder to remove the need for UUIDs and try to be more consistent with other parts of the engine.

I like:

• .register_dev_tool
• a central registry
• a way to iterate over all of the devtools
• pulling shared config like "is_enabled" out into the struct

I don't like:

• the need to use UUIDs
• the fact that it's unclear where we should store tool-specific config

I think we should:

• borrow from the [Asset] trait heavily, and create a matching DevTool trait
• use the type that implements DevTool to store tool-specific config
• add a DevTool: Reflect bound: this will be essential for editor use cases and we need to make sure it works now
• use TypeId keys

In summary:

1. Every dev tool has its own config type, which may be a unit struct.
2. The DevTool trait is implemented for these types.
3. DevToolConfig stores both common settings (e.g. is_enabled) and tool specific settings, using a Box<dyn DevTool>.
4. DevToolsStore is a wrapper over HashMap<TypeId, DevToolConfig>.
5. We add dev tools using app.init_dev_tool::<D: DevTool>, with a matching app.insert_dev_tool(d: D) method for when you want to supply config.

[pablo-lua]

Looks good! Just a few comments and a request: Can you change the new ui debug overlay to use this abstraction? see #10420

[pablo-lua]

Is there a reason why those two impl Blocks aren't together?

[pablo-lua]

Also, I think we might want helper methods to return the inner config as &dyn Reflect and &mut dyn Reflect.

[pablo-lua]

Suggested change

	fn insert_dev_tool<D: DevTool>(&mut self, value: D) -> &mut Self {
	let dev_tool = DevToolConfig::new(TypeId::of::<D>(), value);
	fn insert_dev_tool<D: DevTool>(&mut self, tool_config: D) -> &mut Self {
	let dev_tool = DevToolConfig::new(TypeId::of::<D>(), tool_config);

[pablo-lua]

Just a better naming here, but can be a opinion question.

[cart]

I'm not convinced we should have a "dev tool" abstraction like this:

1. What constitutes a "dev tool" is extremely nebulous. I do not think there is a good clear line between a "dev tool" and a "bevy engine feature".
2. This moves configuration outside of the standard "ECS resource" pattern and into a consolidated DevToolStore resource. This increases the clunkiness of accessing dev tool config significantly and makes it impossible to use ECS change detection to determine when the config has changed.
3. If we want to expose an existing Bevy feature in some "dev tool debug interface", we shouldn't need to refactor how it is configured.
4. The motivation appears to be "we want a queryable catalog of dev tools and their configuration, with a common on/off switch". I would argue that "Dev tool" is too general of a concept to be a useful generic "category of thing". Instead, consumers of Bevy features (such as a "dev tool debug interface") should be responsible for choosing what Bevy features to expose and how to expose them (including what it means to enable and disable a feature). The "dev tool debug interface" can be responsible for being the catalog of features to expose.
5. If we really need a "common on/off switch", that is something that should be considered holistically. However without consideration for the lifecycle of adding/removing features (systems, config, etc), I don't think this would provide significant value. Instead, just add enabled: bool to whatever feature-config resource makes sense. It should be easy to bind specific bool values to whatever debug interface we are building.

I think, if we were to embrace a generic pattern like this, it should be done at the Plugin level and after significant consideration about "plugin/feature configuration and lifecycles".

[pablo-lua]

1

I agree that's a nebulous concept, but at the same time, this crate was created with the goal of more than offering users a simple tool interface: it was meant for helping with the editor development, and IMO this kind of abstraction can help the engine in the future.

2

Agree with the point of this making change detection looking weird, but at the same time, this resource can help with creating and managing tools at runtime and helping with a global access that maybe can use by the crate itself.

3

Agreed, we shouldn't change any behavior of any feature we want to show. For example, it wouldn't be acceptable to refactor frame count just to do the fps overlay. But, in this field, if we need a refactoring of some kind, this should be considered more a question of "do the user have enough freedom so they can build something like this from their own code?". So some refactor can maybe be considered more a question of freedom than changing existent behavior.

4

I think one of the reasons to have this is helping with runtime config: there are tools that are just added and nothing more, so we create this DevTool crate just so we say: "hey, that's what we are expecting from a user defined DevTool, if you don't think this can be in, so maybe you are working with something more complex and/or more specific."

5

In fact, in the future, we can maybe add and remove plugins at runtime, which maybe defeats the purpose of this abstraction (not in the sense of a global dev tool store, though), you can just remove plugins added here or even remove systems and there you have disabled the tool.

These are very good points about this abstraction, and I think we really need to clarify what a dev tool really is. Mainly things that are meant to debug and we expect to have in the editor? And Users can even add their own, and the editor will handle the rest in some way, I really think this abstraction can help in that kind of sense

[alice-i-cecile]

@cart I understand your concerns. I'll aim to write up a proper response and design vision in an RFC, and link it here (and the linked issue). I think there's very good reasons to want a cohesive method of control (even if this might not be the best possible design), but those aren't clearly articulated anywhere.

[pablo-lua]

Blocked on the RFC
See bevyengine/rfcs#77