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.

Sign up for GitHub

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

Jump to bottom

[Merged by Bors] - implement Reflect for Input<T>, some misc improvements to reflect value derive #5676

Closed
wants to merge 5 commits into from
Closed

[Merged by Bors] - implement Reflect for Input<T>, some misc improvements to reflect value derive #5676

wants to merge 5 commits into from

Conversation

jakobhellermann
Copy link
Contributor

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

@jakobhellermann jakobhellermann added C-Feature A new feature, making something new possible A-Reflection Runtime information about types labels Aug 13, 2022
tim-blackbird
tim-blackbird reviewed Aug 13, 2022
View reviewed changes
Copy link
Contributor

@tim-blackbird tim-blackbird left a comment

Choose a reason for hiding this comment

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

It would be nice if ScanCode was also Reflect : )

@jakobhellermann jakobhellermann force-pushed the reflect-for-input branch 2 times, most recently from 2662ef5 to 9ee07f3 Compare August 16, 2022 11:39
tim-blackbird
tim-blackbird approved these changes Aug 20, 2022
View reviewed changes
Copy link
Contributor

@tim-blackbird tim-blackbird left a comment

Choose a reason for hiding this comment

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

Not familiar with the derive but LGTM.

@alice-i-cecile
Copy link
Member

bors try

@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 Sep 2, 2022
bors bot added a commit that referenced this pull request Sep 2, 2022
@bors
Try #5676:
a278b8a
@bors
Copy link
Contributor

bors bot commented Sep 2, 2022

try

Build succeeded:

@alice-i-cecile
Copy link
Member

bors r+

bors bot pushed a commit that referenced this pull request Sep 7, 2022
@jakobhellermann
implement Reflect for Input<T>, some misc improvements to reflect…
7d9e864 
… 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
@bors
Copy link
Contributor

bors bot commented Sep 7, 2022

Pull request successfully merged into main.

Build succeeded:

@bors bors bot changed the title implement Reflect for Input<T>, some misc improvements to reflect value derive [Merged by Bors] - implement Reflect for Input<T>, some misc improvements to reflect value derive Sep 7, 2022
@bors bors bot closed this Sep 7, 2022
nicopap pushed a commit to nicopap/bevy that referenced this pull request Sep 12, 2022
@jakobhellermann @nicopap
implement Reflect for Input<T>, some misc improvements to reflect…
60a271e 
… 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
@jakobhellermann @james7132
implement Reflect for Input<T>, some misc improvements to reflect…
28e13d8 
… 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
@jakobhellermann @ItsDoot
implement Reflect for Input<T>, some misc improvements to reflect…
c0dda2e 
… 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
Reviewers

@alice-i-cecile alice-i-cecile alice-i-cecile left review comments

@NiklasEi NiklasEi NiklasEi approved these changes

@tim-blackbird tim-blackbird tim-blackbird approved these changes

@MrGVSV MrGVSV MrGVSV approved these changes

Assignees
No one assigned
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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@jakobhellermann @alice-i-cecile @NiklasEi @tim-blackbird @MrGVSV