Skip to content

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;

@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