Skip to content

[Merged by Bors] - Add FromReflect for Visibility#6410

Closed
Shatur wants to merge 1 commit intobevyengine:mainfrom
simgine:from-reflect
Closed

[Merged by Bors] - Add FromReflect for Visibility#6410
Shatur wants to merge 1 commit intobevyengine:mainfrom
simgine:from-reflect

Conversation

@Shatur
Copy link
Contributor

@Shatur Shatur commented Oct 29, 2022

Objective

  • Visibility don't have FromReflect derive.

Solution

  • Add it.

Changelog

Added

  • FromReflect for Visibility.

@alice-i-cecile alice-i-cecile added C-Usability A targeted quality-of-life change that makes Bevy easier to use A-Reflection Runtime information about types A-Hierarchy labels Oct 29, 2022
Copy link
Member

@alice-i-cecile alice-i-cecile left a comment

Choose a reason for hiding this comment

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

How does FromReflect work for Parent/Children? Doesn't it assume a default value? What does this open up?

I'm also a bit reluctant to add this for those because it allows a way to construct those types (even if it's obviously kinda overwrought), which people could abuse to create a malformed hierarchy.

No comments about the Visibility impl; I would merge that as trivial.

@Shatur
Copy link
Contributor Author

Shatur commented Oct 29, 2022

Doesn't it assume a default value?

If I understand correctly, It creates a "default" values using FromWorld impl as Reflect do.
I also not sure if having FromReflect for Parent and Children is necessary. Should I remove it then?

@alice-i-cecile
Copy link
Member

Yeah, remove those for now, we can add them later if needed.

@Shatur
Copy link
Contributor Author

Shatur commented Oct 29, 2022

Okay, done!

@Shatur Shatur changed the title Add FromReflect imls Add FromReflect for Visibility Oct 29, 2022
Copy link
Member

@alice-i-cecile alice-i-cecile left a comment

Choose a reason for hiding this comment

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

bors r+

@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 Oct 29, 2022
bors bot pushed a commit that referenced this pull request Oct 29, 2022
# Objective

- `Visibility` don't have `FromReflect` derive.

## Solution

- Add it.

---

## Changelog

### Added

- `FromReflect` for `Visibility`.
@bors bors bot changed the title Add FromReflect for Visibility [Merged by Bors] - Add FromReflect for Visibility Oct 29, 2022
@bors bors bot closed this Oct 29, 2022
@Shatur Shatur deleted the from-reflect branch October 29, 2022 23:16
ItsDoot pushed a commit to ItsDoot/bevy that referenced this pull request Feb 1, 2023
# Objective

- `Visibility` don't have `FromReflect` derive.

## Solution

- Add it.

---

## Changelog

### Added

- `FromReflect` for `Visibility`.
@alice-i-cecile alice-i-cecile added A-ECS Entities, components, systems, and events and removed A-Hierarchy labels Jan 28, 2025
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

No open projects
Status: Done

Development

Successfully merging this pull request may close these issues.

4 participants