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

Implement Reflect for Box<dyn Reflect> #3392

Open
alice-i-cecile opened this issue Dec 19, 2021 · 10 comments · May be fixed by #3400 or #14776
Open

Implement Reflect for Box<dyn Reflect> #3392

alice-i-cecile opened this issue Dec 19, 2021 · 10 comments · May be fixed by #3400 or #14776
Labels
A-Reflection Runtime information about types C-Usability A simple quality-of-life change that makes Bevy easier to use S-Ready-For-Implementation This issue is ready for an implementation PR. Go for it!
Milestone

Comments

@alice-i-cecile
Copy link
Member

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()
    }
}
@alice-i-cecile alice-i-cecile added C-Usability A simple quality-of-life change that makes Bevy easier to use A-Reflection Runtime information about types S-Ready-For-Implementation This issue is ready for an implementation PR. Go for it! D-Trivial Nice and easy! A great choice to get started with Bevy labels Dec 19, 2021
@franciscoaguirre
Copy link

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

@alice-i-cecile
Copy link
Member Author

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).

@franciscoaguirre franciscoaguirre linked a pull request Dec 20, 2021 that will close this issue
@cart
Copy link
Member

cart commented Feb 4, 2022

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

@alice-i-cecile
Copy link
Member Author

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

@Davier
Copy link
Contributor

Davier commented Feb 4, 2022

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
Copy link
Contributor

Shatur commented Jun 29, 2022

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
Copy link
Member Author

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
Copy link
Contributor

Shatur commented Jun 29, 2022

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.

@alice-i-cecile alice-i-cecile removed the D-Trivial Nice and easy! A great choice to get started with Bevy label Feb 27, 2023
@micycle8778
Copy link

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
Copy link
Contributor

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.

@MrGVSV MrGVSV linked a pull request Aug 16, 2024 that will close this issue
@MrGVSV MrGVSV added this to the 0.15 milestone Aug 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Reflection Runtime information about types C-Usability A simple quality-of-life change that makes Bevy easier to use S-Ready-For-Implementation This issue is ready for an implementation PR. Go for it!
Projects
Status: In Progress
8 participants