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

Multiple Configurations for Gizmos #10342

Merged
merged 38 commits into from
Jan 18, 2024

Conversation

jeliag
Copy link
Contributor

@jeliag jeliag commented Nov 2, 2023

Objective

This PR aims to implement multiple configs for gizmos as discussed in #9187.

Solution

Configs for the new GizmoConfigGroups are stored in a GizmoConfigStore resource and can be accesses using a type based key or iterated over. This type based key doubles as a standardized location where plugin authors can put their own configuration not covered by the standard GizmoConfig struct. For example the AabbGizmoGroup has a default color and toggle to show all AABBs. New configs can be registered using app.init_gizmo_group::<T>() during startup.

When requesting the Gizmos<T> system parameter the generic type determines which config is used. The config structs are available through the Gizmos system parameter allowing for easy access while drawing your gizmos.

Internally, resources and systems used for rendering (up to an including the extract system) are generic over the type based key and inserted on registering a new config.

Alternatives

The configs could be stored as components on entities with markers which would make better use of the ECS. I also implemented this approach (here) and believe that the ergonomic benefits of a central config store outweigh the decreased use of the ECS.

Unsafe Code

Implementing system parameter by hand is unsafe but seems to be required to access the config store once and not on every gizmo draw function call. This is critical for performance. Is there a better way to do this?

Future Work

New gizmos (such as #10038, and ideas from #9400) will require custom configuration structs. Should there be a new custom config for every gizmo type, or should we group them together in a common configuration? (for example EditorGizmoConfig, or something more fine-grained)

Changelog

  • Added GizmoConfigStore resource and GizmoConfigGroup trait
  • Added init_gizmo_group to App
  • Added early returns to gizmo drawing increasing performance when gizmos are disabled
  • Changed GizmoConfig and aabb gizmos to use new GizmoConfigStore
  • Changed Gizmos system parameter to use type based key to retrieve config
  • Changed resources and systems used for gizmo rendering to be generic over type based key
  • Changed examples (3d_gizmos, 2d_gizmos) to showcase new API

Migration Guide

  • GizmoConfig is no longer a resource and has to be accessed through GizmoConfigStore resource. The default config group is DefaultGizmoGroup, but consider using your own custom config group if applicable.

Copy link
Contributor

github-actions bot commented Nov 2, 2023

Welcome, new contributor!

Please make sure you've read our contributing guide and we look forward to reviewing your pull request shortly ✨

@alice-i-cecile alice-i-cecile added C-Usability A targeted quality-of-life change that makes Bevy easier to use A-Gizmos Visual editor and debug gizmos labels Nov 2, 2023
@nicopap nicopap added this to the 0.13 milestone Nov 2, 2023
Copy link
Contributor

@nicopap nicopap left a comment

Choose a reason for hiding this comment

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

This is excellent! I'm excited by this change. I've a few comments.

crates/bevy_gizmos/src/config.rs Outdated Show resolved Hide resolved
crates/bevy_gizmos/src/config.rs Outdated Show resolved Hide resolved
crates/bevy_gizmos/src/config.rs Outdated Show resolved Hide resolved
crates/bevy_gizmos/src/gizmos.rs Outdated Show resolved Hide resolved
crates/bevy_gizmos/src/gizmos.rs Outdated Show resolved Hide resolved
crates/bevy_gizmos/src/gizmos.rs Outdated Show resolved Hide resolved
crates/bevy_gizmos/src/config.rs Outdated Show resolved Hide resolved
crates/bevy_gizmos/src/config.rs Outdated Show resolved Hide resolved
crates/bevy_gizmos/src/config.rs Outdated Show resolved Hide resolved
///
/// If `line_perspective` is `true` then this is the size in pixels at the camera's near plane.
/// A trait adding `init_gizmo_config<T>()` to the app
pub trait AppGizmoBuilder {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: Could this be moved to its own module?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm quite new to rust and big projects and would love to understand why this should be moved to its own module. For me, it seems to be related to the plugin build code above. Thanks!

Copy link
Contributor

Choose a reason for hiding this comment

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

The general pattern in rust is to have a tinny lib.rs. In bevy, it usually only consists of re-exports (such as the prelude) and the root impl Plugin for CratePlugin definition.

The whole bevy_gizmo crate started small enough that it could fit in a single file. But as we add features, it is good to split up. I think it's time to split it, just generally.

In the case of AppGizmoBuilder, some crate authors define extension traits in a separate module, so it would be fit to move to its own module.

Copy link
Contributor Author

@jeliag jeliag Nov 19, 2023

Choose a reason for hiding this comment

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

Thank you for the explanation! I moved the aabb code into a separate module as I changed how additional gizmo configs should be added. Opening another PR to split the rest of lib.rs seems to be a better fit. Or should I do everything in this one?

@nicopap
Copy link
Contributor

nicopap commented Nov 7, 2023

In #9187, I make the point that the API chosen here is similar to the one used by the diagnostics (with a diagnostics store).

This API uses reflection so that we can add additional non-homogeneous configuration options. To me, it seems an asset based solution would not solve any of the problems the reflection-based approach solves, while introducing new ones.

The gizmo API is very different from any other bevy API, as it is immediate, which is completely different from interacting with the ECS, which by definition is retained. I suspect that any ECS-based approach would give up all the ergonomics of the gizmo API.

But if you have an idea in mind, feel free to implement it. I'd be glad to be proven wrong.

@nicopap nicopap added the M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide label Nov 22, 2023
Copy link
Contributor

@nicopap nicopap left a comment

Choose a reason for hiding this comment

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

this is already good for me.

}

/// Returns [`GizmoConfig`] and [`GizmoConfigGroup`] associated with [`GizmoConfigGroup`] `T`
pub fn get<T: GizmoConfigGroup>(&self) -> (&GizmoConfig, &T) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I would have this return an Option<(&GizmoConfig, &T)>. Can always let the end-user do the unwrap.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought about returning Option<(&GizmoConfig, &T)> but decided to use a panicking API for the generic getter methods. Contrary to the get_dyn methods, the generic ones will only fail if the config isn't correctly registered as the type is already checked to be a gizmo config. The possibility of a plugin author failing to register the config or the user forgetting to add the plugin doesn't feel like something we need to be aware of every time the get method is used.

Even if these return an option, the Gizmos<T> SystemParam would still fail to be built when the config isn't registered. I believe both generic access patterns should react the same way.

But I'm open to changing it. I just wanted to share my thoughts :)

Copy link
Contributor

Choose a reason for hiding this comment

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

I just don't like the panicking API on a get method. In the std, get is for non-panicking indexing. My beef is more with the name than the fact it panics. What about config? In fact, it doesn't really make sense to return an Option, panicking is the correct behavior here…

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are right, get is an unfortunate name. I'm changing it to config for now, but maybe we will find a better name in future.

crates/bevy_gizmos/src/aabb.rs Outdated Show resolved Hide resolved
crates/bevy_gizmos/src/gizmos.rs Outdated Show resolved Hide resolved
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.

Really liked those changes, With this, its possible to use gizmos in tools context without blocking the user from using it.

One question, though: Should the Aabb tool go in #11341 when it get merged? (That can probably be a follow up PR, just to be sure that this is indeed a debug tool and not something that we use regularly)

@alice-i-cecile alice-i-cecile added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Jan 18, 2024
@alice-i-cecile
Copy link
Member

alice-i-cecile commented Jan 18, 2024

@jeliag if you resolve the merge conflicts here I can merge this in :) Sorry about missing this; it didn't get the Ready-For-Final-Review label.

@alice-i-cecile alice-i-cecile added this pull request to the merge queue Jan 18, 2024
Merged via the queue into bevyengine:main with commit f6b40a6 Jan 18, 2024
26 checks passed
@jeliag
Copy link
Contributor Author

jeliag commented Jan 18, 2024

Really liked those changes, With this, its possible to use gizmos in tools context without blocking the user from using it.

One question, though: Should the Aabb tool go in #11341 when it get merged? (That can probably be a follow up PR, just to be sure that this is indeed a debug tool and not something that we use regularly)

I think moving AABB gizmos (or any other special debug gizmos) out of the gizmos crate is a good idea. This way the gizmos crate can focus on providing a useful API for drawing gizmos.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Gizmos Visual editor and debug gizmos C-Usability A targeted quality-of-life change that makes Bevy easier to use M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants