bevyengine / bevy

A refreshingly simple data-driven game engine built in Rust
https://bevyengine.org
Apache License 2.0
35.92k stars 3.55k forks source link

Implement Reflect for Box<dyn Reflect> #3392

Open alice-i-cecile opened 2 years ago

alice-i-cecile commented 2 years ago

What problem does this solve or what need does it fill?

Boxed Reflect trait objects should themselves be reflectable

What solution would you like?

Implement Reflect for Box<dyn Reflect>.

What alternative(s) have you considered?

None.

Additional context

Working snippet from @Davier:

unsafe impl Reflect for Box<dyn Reflect> {
    fn type_name(&self) -> &str {
        self.deref().type_name()
    }

    fn any(&self) -> &dyn Any {
        self.deref().any()
    }

    fn any_mut(&mut self) -> &mut dyn Any {
        self.deref_mut().any_mut()
    }

    fn apply(&mut self, value: &dyn Reflect) {
        self.deref_mut().apply(value)
    }

    fn set(&mut self, value: Box<dyn Reflect>) -> Result<(), Box<dyn Reflect>> {
        self.deref_mut().set(value)
    }

    fn reflect_ref(&self) -> ReflectRef {
        self.deref().reflect_ref()
    }

    fn reflect_mut(&mut self) -> ReflectMut {
        self.deref_mut().reflect_mut()
    }

    fn clone_value(&self) -> Box<dyn Reflect> {
        self.deref().clone_value()
    }

    fn reflect_hash(&self) -> Option<u64> {
        self.deref().reflect_hash()
    }

    fn reflect_partial_eq(&self, value: &dyn Reflect) -> Option<bool> {
        self.deref().reflect_partial_eq(value)
    }

    fn serializable(&self) -> Option<Serializable> {
        self.deref().serializable()
    }
}
franciscoaguirre commented 2 years ago

Hi there! Never contributed before but want to give it a try. Can I help on this issue?

alice-i-cecile commented 2 years ago

Hi @franciscoaguirre, that sounds great :) Are you comfortable with the process of making PRs in general? See https://github.com/bevyengine/bevy/blob/main/CONTRIBUTING.md#contributing-code for a refresher if you need it.

This should be a straightforward PR: just drop the snippet into place, and write a simple test to verify that it works as expected :) Feel free to open a draft PR (that says "Closes #3392" in its body) and then ping me for questions in that thread (or ask around in #scenes-dev on Discord).

cart commented 2 years ago

What is the actual use case here? This enables nested boxing, which i'd like to avoid unless absolutely necessary.

alice-i-cecile commented 2 years ago

I don't immediately remember the end user use case right now. I suspect that #3694 will help.

Davier commented 2 years ago

IIRC someone wanted to store Box<dyn Reflect>s in a component (possibly a Vec of them), and still be able to reflect that component.

Shatur commented 2 years ago

What is the actual use case here? This enables nested boxing, which i'd like to avoid unless absolutely necessary.

@cart this will allow to serialize structs with dyn Reflect fields. For example, I have a struct that contains modified components from the last network tick (and components represented as dyn Reflect) that I need send over the network. Is this a valid use case or is there a better way to achieve this?

Currently I creating a newtype with dyn Reflect, clone TypeRegistry into a global variable and implement Serialize / Deserialize on this newtype using the global TypeRegistry.

alice-i-cecile commented 2 years ago

For example, I have a struct that contains modified components from the last network tick (and components represented as dyn Reflect) that I need send over the network. Is this a valid use case or is there a better way to achieve this?

For this, it feels like what we really want is better DynamicScene support, and the ability to use those for that sort of diff-ing operation. We already have a tool for the serde of collections of dynamic components, but it's not clearly documented and the use case hasn't been fleshed out.

Shatur commented 2 years ago

For this, it feels like what we really want is better DynamicScene support,

It looks like it can only be serialized to RON, which is not suitable for network transmission. Also DynamicEntity contains only u32. When sending over the network I need all Entity bits from server and map it into the corresponding client entity using EntityMap. But it would be great to improve it.

micycle8778 commented 10 months ago

What is the actual use case here? This enables nested boxing, which i'd like to avoid unless absolutely necessary.

I ran into this while trying to write a task system for my game:

#[derive(Component, Reflect)]
struct MoveTask {
    to: Vec3,
    next_task: Box<dyn Reflect>
}

/* more tasks here ... */

#[derive(Component, Reflect)]
struct NoOpTask {
    next_task: Box<dyn Reflect>
}

fn no_op_task_tick(
    mut commands: Commands,
    q_tasks: Query<(Entity, &NoOpTask)>
) {
    for (entity, task) in &q_tasks {
        commands.entity(entity).remove::<NoOpTask>();
        if let Some(new_task) = task.next_task {
            commands.entity().insert_reflect(new_task);
        }
    }
}

There might be a better way of doing this, like a command for inserting a Box<dyn Component>.

brandon-reinhart commented 4 months ago

I have found this to be something I would have used - also for a task system. My task steps were components that I pull out of a Vec<Box> and place the active step on the task. This worked very well, but I ran into a problem when implementing save / load using bevy reflect. I had to temporarily switch to every task step being an entity (each with one component). In my case, I was basically using an Entity as a level of indirection to the dyn Reflect component I was interested in.