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

Add FromReflect where Reflect is used #8776

Merged
merged 9 commits into from
Jun 19, 2023
Merged

Add FromReflect where Reflect is used #8776

merged 9 commits into from
Jun 19, 2023

Conversation

raffaeleragni
Copy link
Contributor

@raffaeleragni raffaeleragni commented Jun 7, 2023

Objective

Discovered that PointLight did not implement FromReflect. Adding FromReflect where Reflect is used. I overreached and applied this rule everywhere there was a Reflect without a FromReflect, except from where the compiler wouldn't allow me.

Based from question: #8774

Solution

  • Adding FromReflect where Reflect was already derived

Notes

First PR I do in this ecosystem, so not sure if this is the usual approach, that is, to touch many files at once.

@github-actions
Copy link
Contributor

github-actions bot commented Jun 7, 2023

Welcome, new contributor!

Please make sure you've read our contributing guide and we look forward to reviewing your pull request shortly ✨

@raffaeleragni raffaeleragni marked this pull request as ready for review June 7, 2023 17:57
@MrGVSV MrGVSV added C-Usability A targeted quality-of-life change that makes Bevy easier to use A-Reflection Runtime information about types labels Jun 7, 2023
Copy link
Member

@MrGVSV MrGVSV left a comment

Choose a reason for hiding this comment

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

Thanks for doing this!

One thing I think needs to be added are ReflectFromReflect registrations for all of these types. This is added the same way as ReflectComponent and other type data:

#[derive(Reflect, FromReflect)]
#[reflect(FromReflect)]
struct MyType;

crates/bevy_reflect/src/lib.rs Outdated Show resolved Hide resolved
crates/bevy_reflect/src/lib.rs Outdated Show resolved Hide resolved
@raffaeleragni
Copy link
Contributor Author

Now also added the ReflectFromReflect

@raffaeleragni
Copy link
Contributor Author

Now it seems to error, unclear why tho.

@raffaeleragni
Copy link
Contributor Author

Is this normal?

query stack during panic:
#0 [mir_built] building MIR for `event::<impl at crates\bevy_window\src\event.rs:163:46: 163:53>::apply`
#1 [unsafety_check_result] unsafety-checking `event::<impl at crates\bevy_window\src\event.rs:163:46: 163:53>::apply`
#2 [analysis] running analysis passes on this crate
end of query stack
there was a panic while trying to force a dep node
try_mark_green dep node stack:
#0 mir_const(b9aa1f027d561b53-4af66effe17d0313)
#1 mir_promoted(b9aa1f027d561b53-4af66effe17d0313)
#2 mir_borrowck(bevy_window[aa47]::event::{impl#124}::apply)
end of try_mark_green dep node stack
error: internal compiler error: re-entrant incremental verify failure, suppressing message

@raffaeleragni
Copy link
Contributor Author

Ok, should be fixed now.

Copy link
Member

@MrGVSV MrGVSV left a comment

Choose a reason for hiding this comment

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

LGTM! Glad to see FromReflect in more places 😄

@MrGVSV MrGVSV added this to the 0.11 milestone Jun 8, 2023
@raffaeleragni
Copy link
Contributor Author

How do we know that these are all the needed ones?
All I could do was finding them via search, but it's a long list.

A couple of ones also could not derive FromReflect, anything to do with those?

@MrGVSV
Copy link
Member

MrGVSV commented Jun 9, 2023

How do we know that these are all the needed ones? All I could do was finding them via search, but it's a long list.

A couple of ones also could not derive FromReflect, anything to do with those?

Hm, I'm not sure. We should get a couple more reviews to make sure we're not being overzealous in certain places or possibly even breaking/missing something.

@MrGVSV
Copy link
Member

MrGVSV commented Jun 9, 2023

Also, I should make it clear that this PR would be a temporary solution until #6056 is merged (which, honestly, could come before or after 0.11).

@raffaeleragni
Copy link
Contributor Author

Sure, all I need right now is that some components are not deriving FromReflect, not necessarily ReflectFromReflect, because it's blocking something I am writing. If that comes first I can still continue either way.

@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 Jun 19, 2023
@alice-i-cecile alice-i-cecile enabled auto-merge June 19, 2023 16:05
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Jun 19, 2023
Merged via the queue into bevyengine:main with commit 7fc6db3 Jun 19, 2023
@raffaeleragni raffaeleragni deleted the from_reflect branch June 19, 2023 17:20
james7132 pushed a commit to james7132/bevy that referenced this pull request Jun 19, 2023
# Objective

Discovered that PointLight did not implement FromReflect. Adding
FromReflect where Reflect is used. I overreached and applied this rule
everywhere there was a Reflect without a FromReflect, except from where
the compiler wouldn't allow me.

Based from question: bevyengine#8774

## Solution

- Adding FromReflect where Reflect was already derived

## Notes

First PR I do in this ecosystem, so not sure if this is the usual
approach, that is, to touch many files at once.

---------

Co-authored-by: Alice Cecile <alice.i.cecile@gmail.com>
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 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
Status: In Progress
Development

Successfully merging this pull request may close these issues.

3 participants