- 
          
- 
                Notifications
    You must be signed in to change notification settings 
- Fork 4.2k
Generic composite primitive gizmos / rework #21480
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
base: main
Are you sure you want to change the base?
Conversation
e9aeffc    to
    3fa6fb2      
    Compare
  
    | @lynn-lumen do you mind having a look at this? | 
| There is also #20246. All of these PRs take a similar approach, and I agree with the direction. However, using Gizmos as structs should probably be a separate PR if that approach is blessed. I think @alice-i-cecile 's input on this may be valuable. | 
3fa6fb2    to
    f3a9af4      
    Compare
  
    66d3cc4    to
    06beda3      
    Compare
  
    | 
 Regarding #20246, so it turns out that with a simple blanket  impl<B: GizmoBlueprint2d> From<B> for GizmoAsset {
    fn from(mut value: B) -> Self {
        let mut asset = GizmoAsset::new();
        value.build_2d(&mut asset);
        asset
    }
}... we already get something very close to what  fn setup(
    mut commands: Commands,
    mut gizmo_assets: ResMut<Assets<GizmoAsset>>,
) {
    let mut gizmo = GizmoAsset::new();
    commands.spawn((
        Gizmo {
            handle: gizmo_assets.add(Circle::new(0.5).to_blueprint_2d(Vec2::ZERO, PURPLE).resolution(20)),
            line_config: GizmoLineConfig {
                width: 5.,
                ..default()
            },
            ..default()
        },
        Transform::from_xyz(4., 1., 0.),
    ));
}and most of the way towards the design provided in that issue. The only real ergonomic pain is all the fields in  Assuming we moved forward with my PR (bikeshedding on naming welcome), the next PR would be to decompose  #[derive(Component, Clone, Debug, Default, Deref, DerefMut, Reflect, PartialEq, Eq, From)]
#[reflect(Component, Default, Clone, PartialEq)]
#[require(GizmoLineConfig, GizmoDepthBias, Transform)]
pub struct Gizmo2d(pub Handle<GizmoAsset>);
fn setup(
    mut commands: Commands,
    mut gizmo_assets: ResMut<Assets<GizmoAsset>>,
) {
    commands.spawn((
        Gizmo2d(gizmo_assets.add(Circle::new(0.5).to_blueprint_2d(Vec2::ZERO, PURPLE).resolution(20))),
        Transform::from_xyz(4., 1., 0.),
    ));
}After that, BSN might bring us closer to literally  | 
Objective
I was trying to implement gizmos for
Rings - but ran into the fact that builders (GizmoPrimitive2d::Output) contain a mutable borrow of the gizmos, so I couldn't construct "composites" of primitives that were also configurable i.e. I need to be able to generate two sub-gizmos from a single composite primitive:Solution
The solution is to remove the borrow from each of the builders and defer the use of the GizmoBuffer (i.e. the builder is now just data).
Essentially this mirrors the
Meshableimplementation:ToGizmoBlueprint2dto take a primitive and generate a blueprint / descriptor / builder from it (where the data to actually generate the gizmo is sourced from)primitive -> builder (data only, no borrow)GizmoBlueprintthat uses the data in the blueprint to generate a gizmobuilder -> gizmo (GizmoBuffer borrowed on demand)GizmoBuilder2dthat is a wrapper around the builder and the GizmoBuffer-borrow, and provides easy access to the inner builders (via Deref/DerefMut)All the gizmo drawing instructions that were previously placed inside the Drop implementation for every builder or directly inside the
GizmoPrimitive2d::primitive_2dimplementation now lives in theBlueprint2d::build_2dimplementation. Now there is only a single Drop implementation, that is not user-facing, inGizmoBuilder2d; another convenient benefit is thegizmos.enabledcheck can be done in this one place, and it won't require users to implement it themselves.GizmoBuilder2dcould probably also be simplified, so it always returnsGizmoBuilder2d<'a, Self::Output<'a>, Config, Clear>or even turned into a blanket implementation.Generic composites, non-generic composites and custom primitives should be fairly trivial with this design.
I'm far from done with this but I'm putting this out just as a draft to see whether this is the right path forward or not before I continue working on it.
One thing that may need to be resolved is that the builder/blueprint is owned, so something like a Polyline has to be cloned at some point. I also suspect it's possible to have a single pair of the Blueprint traits (
ToGizmoBlueprintandGizmoBlueprint) instead of dividing them into separate 2d and 3d traits (not that I've implemented the 3d one yet).I hadn't seen #20049 until after I got this far. It seems like it goes down a somewhat similar path in some ways, at least as far as deferring the use of the
GizmoBuffergoes, but I'm not sure whether it solves the builder-reuse problem.Testing
So far, the 2d_gizmos examples continues to work the same way it used to, as far as I'm aware.