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

Feature request: Add/remove component buttons #9

Open
PROMETHIA-27 opened this issue Jan 20, 2022 · 11 comments
Open

Feature request: Add/remove component buttons #9

PROMETHIA-27 opened this issue Jan 20, 2022 · 11 comments

Comments

@PROMETHIA-27
Copy link
Contributor

PROMETHIA-27 commented Jan 20, 2022

I think that the editor being able to manipulate the components of entities- and display all available ones- is a vital feature which is missing currently. I'd be happy to make a PR for this, I just wanted to make an issue first in case you were working on this already. The UI for it should be easy, the biggest problem I encountered was being able to iterate all the component types available. I've found out that any type that has #[reflect(Component)] on it and has been registered (both necessary for being serialized as a scene) can be accessed like this:

let reg = app.world.get_resource::<bevy::reflect::TypeRegistry>().unwrap();
for ty in reg.read().iter() {
    if let Some(_) = ty.data::<ReflectComponent>() {
        println!("{}", ty.name());
    }
}

I'd be working on new-main rather than main, since I assume main is not going to be further developed until new-main is merged into it.

Should I go ahead and make a PR for this?

@jakobhellermann
Copy link
Owner

I think the reflection API isn't quite enough yet, because it only allows calling add_component with a &dyn Reflect, and there is no way to just insert the default value.

@PROMETHIA-27
Copy link
Contributor Author

PROMETHIA-27 commented Jan 20, 2022

I think I've solved it. To insert default value just do:

// reflect is a ReflectComponent
reflect.add_component(world, entity, &bevy::reflect::DynamicStruct::default());

I was able to add a component with an i32 to an entity with almost this exact code, and the i32 defaulted to 0. This also works for empty struct components, but crashes for tuplestruct components. DynamicTupleStruct also does not work for tuplestruct components, which is really confusing. I think there may be a bug somewhere but I don't understand how. Still looking into that.

EDIT: Nvm, it works perfectly fine on tuple struct components with DynamicTupleStruct. The problem is that loading a scene file with a tuple struct component in it crashes, as it invariably uses DynamicStruct.

EDIT 2: Nvm again, I'm just bad at writing the .scn.ron files. If you use "tuple_struct" instead of "struct" it works fine. Feel like that should be a more obvious error than a panic with 'Attempted to apply non-TupleStruct type to TupleStruct type.' though.

EDIT 3: It's also extremely easy to not do default values:

reflect.insert("value", 12i32);

and now the i32 on the struct is 12. I think should work very well, as you only need a type id to access everything necessary to do this. Would be easy enough to store all components' names and ids in a resource I think.

@jakobhellermann
Copy link
Owner

The problem with that trick is that there is no way to find out if DynamicStruct or DynamicTupleStruct (or DynamicEnum in the future) are the correct ones to use just from a type id.

Another solution could be to add a #[reflect(Default)] to bevy which will store a way to get a Box<dyn Reflect> for the type. Then you could pass that to add_component.

Would be easy enough to store all components' names and ids in a resource I think.

Both names and type ids are already in the TypeRegistryArc resource so that should be enough.

@jakobhellermann
Copy link
Owner

I opened a PR for #[reflect(Default)]: bevyengine/bevy#3733

@PROMETHIA-27
Copy link
Contributor Author

I don't like that solution much as it's even more things to annotate types with, especially when reflect already has a trait bound for default. I think it would be better to just expose an enum of Struct | TupleStruct | Enum on the TypeRegistration, or maybe on the ReflectComponent itself. Plus that would increase compatibility with any crates that forget to add Default to their reflect tag, on top of deriving reflect + reflecting component + registering the type.

@jakobhellermann
Copy link
Owner

I don't know if we can rely on the FromWorld bound always being on a component, I believe it was only added as a necessity as there is no other way of creating a component from a &dyn Reflect.

Someone on discord said

I strongly believe Default should not be required for component (and I'd love to get rid of the FromWorld requirement)

so having an explicit #[reflect(Default)] - while boilerplatey - is probably the way to go for now IMO.

@jakobhellermann
Copy link
Owner

Also I finished up and pushed the code for an Add menu that lets you spawn new bundles into the world: b35bcc2

It doesn't use reflection and will only work for manually registered bundles but I'm linking this here since there's some overlap.

@PROMETHIA-27
Copy link
Contributor Author

Ah, that's a good point, fair enough. I suppose anything that doesn't fulfill everything on its own could probably be newtyped. I'll continue with an add component button branch- I'll just default to DynamicStruct and update it to the proper reflection code when that's available. Could also work on transform gizmos after, those just need that one crate and link its selection to inspector selection, both use the same picking crate underneath I believe.

@jakobhellermann
Copy link
Owner

Yeah I think the DynamicStruct workaround is definitely worth merging and a good solution until bevy supports something else

@jakobhellermann
Copy link
Owner

The gizmo crate uses bevy_mod_picking while this crate uses bevy_mod_raycast, which the picking crate is built on.

I don't know how flexible bevy_mod_picking and the gizmo crate is and I used the raycast one since I'm doing my own selection logic anyways, so that's something to investigate.

@asdfsdfsdfzhjzu
Copy link

could an add/remove Component button now be added that the changes have been merged ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants