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

#[derive(Clone)] on Component{Info,Descriptor} #9812

Merged
merged 1 commit into from
Sep 20, 2023

Conversation

SludgePhD
Copy link
Contributor

Objective

Occasionally, it is useful to pull ComponentInfo or ComponentDescriptor out of the Components collection so that they can be inspected without borrowing the whole World.

Solution

Make ComponentInfo and ComponentDescriptor Clone, so that reflection-heavy code can store them in a side table.


Changelog

  • Implement Clone for ComponentInfo and ComponentDescriptor

@alice-i-cecile alice-i-cecile added A-ECS Entities, components, systems, and events C-Usability A targeted quality-of-life change that makes Bevy easier to use A-Reflection Runtime information about types S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it labels Sep 14, 2023
Copy link
Member

@james7132 james7132 left a comment

Choose a reason for hiding this comment

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

This likely isn't too big of an issue, but a lot of the type safety and soundness of the ECS is heavily reliant on these being correct. We currently do not expose a way to mutably accesss these from Components, but a lot of care should be taken to ensure that the ability to clone them doesn't cause any potential safety footguns, both internally and externally.

I'll give this a more thorough sweep later.

@alice-i-cecile alice-i-cecile removed 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 Sep 18, 2023
@hymm
Copy link
Contributor

hymm commented Sep 18, 2023

My one worry is the drop function stored on ComponentDescriptor. But you'd need to have an owned OwningPtr to even call it and it's already unsafe, so it's probably ok to give access to it.

@SludgePhD
Copy link
Contributor Author

Yeah, and you already had access before, this just lets you have owned access without having to borrow the Components.

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.

As mentioned, the only worry is the drop function, otherwise this is perfectly fine, and I personally thought I needed this a couple times.

@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 Sep 19, 2023
Copy link
Member

@james7132 james7132 left a comment

Choose a reason for hiding this comment

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

We don't expose a &mut Component{Info,Descriptor} of any kind, and this does not change the status quo.

This does re-raise the old concerns I had with @maniwani's "components as entities" idea, however. Not sure if they have any comments on this change.

@maniwani
Copy link
Contributor

This does re-raise the old concerns I had with @maniwani's "components as entities" idea, however.
Not sure if they have any comments on this change.

I don't see an issue. The only thing that would raise safety concerns is the user being able to modify the data in Components, but that isn't the case here (it's also not the case in making components entities btw).

@james7132
Copy link
Member

OK sounds good to me then. Merging.

@james7132 james7132 added this pull request to the merge queue Sep 20, 2023
Merged via the queue into bevyengine:main with commit e07c427 Sep 20, 2023
27 checks passed
rdrpenguin04 pushed a commit to rdrpenguin04/bevy that referenced this pull request Jan 9, 2024
# Objective

Occasionally, it is useful to pull `ComponentInfo` or
`ComponentDescriptor` out of the `Components` collection so that they
can be inspected without borrowing the whole `World`.

## Solution

Make `ComponentInfo` and `ComponentDescriptor` `Clone`, so that
reflection-heavy code can store them in a side table.

---

## Changelog

- Implement `Clone` for `ComponentInfo` and `ComponentDescriptor`
@SludgePhD SludgePhD deleted the clone-component-info branch March 4, 2024 02:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-ECS Entities, components, systems, and events A-Reflection Runtime information about types C-Usability A targeted quality-of-life change that makes Bevy easier to use 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.

6 participants