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

bevy_scene: Add ReflectBundle #9165

Merged
merged 5 commits into from
Aug 28, 2023

Conversation

Shatur
Copy link
Contributor

@Shatur Shatur commented Jul 15, 2023

Objective

Similar to #6344, but contains only ReflectBundle changes. Useful for scripting. The implementation has also been updated to look exactly like ReflectComponent.


Changelog

Added

  • Reflection for bundles.

@Shatur Shatur requested a review from MrGVSV July 15, 2023 11:33
@Shatur Shatur added A-Reflection Runtime information about types C-Feature A new feature, making something new possible labels Jul 15, 2023
Co-authored-by: Gino Valente <[email protected]>
@MrGVSV
Copy link
Member

MrGVSV commented Jul 17, 2023

Note: My approval should probably be taken with a grain of salt as this code is based on #6344 (not trying to intentionally dupe the reviewer system lol)

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.

There is a few issues with this code:

  1. how much the implementation duplicates ReflectComponent
  2. how many failure cases we have. A lot of panic!.
  3. The indentation is all over the place.

Performance

I did take a look at alternative implementation. Notably: could we use DynamicBundle::get_components to avoid having to recursively navigate the struct? I think yes, but it's not relevant for this PR.

Things to do for approval

The fn defined in ReflectBundleFns are very large, so it's going to bloat binaries, pretty bad for wasm.

All said. I think it's fine as of this PR, since you have to explicitly register the ReflectBundle, so no one is paying for things they don't use, at the cost of some convenience (the need to add a register_data::<Bundle, ReflectBundle>()). So I'm approving if the following are done:

  1. Replace the if let … else x with let else x; … to avoid this rightward drift
  2. Remove the documentation, replace them with reference to doc items in reflect/component.rs

crates/bevy_ecs/src/reflect/bundle.rs Outdated Show resolved Hide resolved
crates/bevy_ecs/src/reflect/bundle.rs Outdated Show resolved Hide resolved
crates/bevy_ecs/src/reflect/bundle.rs Outdated Show resolved Hide resolved
@Shatur
Copy link
Contributor Author

Shatur commented Aug 6, 2023

@nicopap done!
Should I also link fn_pointers and new to the one from ReflectComponent?

crates/bevy_ecs/src/reflect/bundle.rs Outdated Show resolved Hide resolved
crates/bevy_ecs/src/reflect/bundle.rs Outdated Show resolved Hide resolved
@nicopap
Copy link
Contributor

nicopap commented Aug 6, 2023

I think the docs are pretty good at this point.

@Shatur Shatur requested a review from nicopap August 15, 2023 20:58
@github-actions
Copy link
Contributor

Example alien_cake_addict failed to run, please try running it locally and check the result.

1 similar comment
@github-actions
Copy link
Contributor

Example alien_cake_addict failed to run, please try running it locally and check the result.

@Shatur Shatur 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 Aug 16, 2023
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Aug 28, 2023
Merged via the queue into bevyengine:main with commit fb9a65f Aug 28, 2023
20 of 21 checks passed
@Shatur Shatur deleted the reflect-bundle branch August 28, 2023 17:11
Shatur added a commit to projectharmonia/bevy that referenced this pull request Aug 30, 2023
# Objective

Similar to bevyengine#6344, but contains only `ReflectBundle` changes. Useful for
scripting. The implementation has also been updated to look exactly like
`ReflectComponent`.

---

## Changelog

### Added

- Reflection for bundles.

---------

Co-authored-by: Gino Valente <[email protected]>
@cart cart mentioned this pull request Oct 13, 2023
43 tasks
rdrpenguin04 pushed a commit to rdrpenguin04/bevy that referenced this pull request Jan 9, 2024
# Objective

Similar to bevyengine#6344, but contains only `ReflectBundle` changes. Useful for
scripting. The implementation has also been updated to look exactly like
`ReflectComponent`.

---

## Changelog

### Added

- Reflection for bundles.

---------

Co-authored-by: Gino Valente <[email protected]>
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-Feature A new feature, making something new possible 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.

4 participants