[Merged by Bors] - implement Reflect for Input<T>, some misc improvements to reflect value derive
#5676
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jakobhellermann
added
C-Enhancement
jakobhellermann
force-pushed
the
reflect-for-input
branch
from
2179b9b
to
29674f5
Compare
NiklasEi
approved these changes
Aug 13, 2022
irate-devil
reviewed
Aug 13, 2022
jakobhellermann
force-pushed
the
reflect-for-input
branch
2 times, most recently
from
2662ef5
to
9ee07f3
Compare
irate-devil
approved these changes
Aug 20, 2022
MrGVSV
approved these changes
Aug 29, 2022
|
bors try |
alice-i-cecile
added
the
S-Ready-For-Final-Review
jakobhellermann
force-pushed
the
reflect-for-input
branch
from
e6e553d
to
65c713b
Compare
|
bors r+ |
bors bot
pushed a commit
that referenced
this pull request
Sep 7, 2022
… value derive (#5676) # Objective - I'm currently working on being able to call methods on reflect types (https://github.com/jakobhellermann/bevy_reflect_fns) - for that, I'd like to add methods to the `Input<KeyCode>` resource (which I'm doing by registering type data) - implementing `Reflect` is currently a requirement for having type data in the `TypeRegistry` ## Solution - derive `Reflect` for `KeyCode` and `Input` - uses `#[reflect_value]` for `Input`, since it's fields aren't supposed to be observable - using reflect_value would need `Clone` bounds on `T`, but since all the methods (`.pressed` etc) already require `T: Copy`, I unified everything to requiring `Copy` - add `Send + Sync + 'static` bounds, also required by reflect derive ## Unrelated improvements I can extract into a separate PR if needed. - the `Reflect` derive would previously ignore `#[reflect_value]` and only accept `#[reflect_value()]` which was a bit confusing - the generated code used `val.clone()` on a reference, which is fine if `val` impls `Clone`, but otherwise also compiles with a worse error message. Change to `std::clone::Clone::clone(val)` instead which gives a neat `T does not implement Clone` error
|
Pull request successfully merged into main. Build succeeded: |
bors
bot
changed the title
Reflect for Input<T>, some misc improvements to reflect value deriveReflect for Input<T>, some misc improvements to reflect value derive
nicopap
pushed a commit
to nicopap/bevy
that referenced
this pull request
Sep 12, 2022
… value derive (bevyengine#5676) # Objective - I'm currently working on being able to call methods on reflect types (https://github.com/jakobhellermann/bevy_reflect_fns) - for that, I'd like to add methods to the `Input<KeyCode>` resource (which I'm doing by registering type data) - implementing `Reflect` is currently a requirement for having type data in the `TypeRegistry` ## Solution - derive `Reflect` for `KeyCode` and `Input` - uses `#[reflect_value]` for `Input`, since it's fields aren't supposed to be observable - using reflect_value would need `Clone` bounds on `T`, but since all the methods (`.pressed` etc) already require `T: Copy`, I unified everything to requiring `Copy` - add `Send + Sync + 'static` bounds, also required by reflect derive ## Unrelated improvements I can extract into a separate PR if needed. - the `Reflect` derive would previously ignore `#[reflect_value]` and only accept `#[reflect_value()]` which was a bit confusing - the generated code used `val.clone()` on a reference, which is fine if `val` impls `Clone`, but otherwise also compiles with a worse error message. Change to `std::clone::Clone::clone(val)` instead which gives a neat `T does not implement Clone` error
james7132
pushed a commit
to james7132/bevy
that referenced
this pull request
Oct 28, 2022
… value derive (bevyengine#5676) # Objective - I'm currently working on being able to call methods on reflect types (https://github.com/jakobhellermann/bevy_reflect_fns) - for that, I'd like to add methods to the `Input<KeyCode>` resource (which I'm doing by registering type data) - implementing `Reflect` is currently a requirement for having type data in the `TypeRegistry` ## Solution - derive `Reflect` for `KeyCode` and `Input` - uses `#[reflect_value]` for `Input`, since it's fields aren't supposed to be observable - using reflect_value would need `Clone` bounds on `T`, but since all the methods (`.pressed` etc) already require `T: Copy`, I unified everything to requiring `Copy` - add `Send + Sync + 'static` bounds, also required by reflect derive ## Unrelated improvements I can extract into a separate PR if needed. - the `Reflect` derive would previously ignore `#[reflect_value]` and only accept `#[reflect_value()]` which was a bit confusing - the generated code used `val.clone()` on a reference, which is fine if `val` impls `Clone`, but otherwise also compiles with a worse error message. Change to `std::clone::Clone::clone(val)` instead which gives a neat `T does not implement Clone` error
ItsDoot
pushed a commit
to ItsDoot/bevy
that referenced
this pull request
Feb 1, 2023
… value derive (bevyengine#5676) # Objective - I'm currently working on being able to call methods on reflect types (https://github.com/jakobhellermann/bevy_reflect_fns) - for that, I'd like to add methods to the `Input<KeyCode>` resource (which I'm doing by registering type data) - implementing `Reflect` is currently a requirement for having type data in the `TypeRegistry` ## Solution - derive `Reflect` for `KeyCode` and `Input` - uses `#[reflect_value]` for `Input`, since it's fields aren't supposed to be observable - using reflect_value would need `Clone` bounds on `T`, but since all the methods (`.pressed` etc) already require `T: Copy`, I unified everything to requiring `Copy` - add `Send + Sync + 'static` bounds, also required by reflect derive ## Unrelated improvements I can extract into a separate PR if needed. - the `Reflect` derive would previously ignore `#[reflect_value]` and only accept `#[reflect_value()]` which was a bit confusing - the generated code used `val.clone()` on a reference, which is fine if `val` impls `Clone`, but otherwise also compiles with a worse error message. Change to `std::clone::Clone::clone(val)` instead which gives a neat `T does not implement Clone` error
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Objective
Input<KeyCode>resource (which I'm doing by registering type data)Reflectis currently a requirement for having type data in theTypeRegistrySolution
ReflectforKeyCodeandInput#[reflect_value]forInput, since it's fields aren't supposed to be observableClonebounds onT, but since all the methods (.pressedetc) already requireT: Copy, I unified everything to requiringCopySend + Sync + 'staticbounds, also required by reflect deriveUnrelated improvements
I can extract into a separate PR if needed.
Reflectderive would previously ignore#[reflect_value]and only accept#[reflect_value()]which was a bit confusingval.clone()on a reference, which is fine ifvalimplsClone, but otherwise also compiles with a worse error message. Change tostd::clone::Clone::clone(val)instead which gives a neatT does not implement Cloneerror