-
-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
Migrate Quat
reflection strategy from "value" to "struct"
#10068
Merged
alice-i-cecile
merged 6 commits into
bevyengine:main
from
Trashtalk217:quat-reflect-struct
Oct 9, 2023
Merged
Migrate Quat
reflection strategy from "value" to "struct"
#10068
alice-i-cecile
merged 6 commits into
bevyengine:main
from
Trashtalk217:quat-reflect-struct
Oct 9, 2023
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
alice-i-cecile
added
A-Reflection
Runtime information about types
C-Usability
A targeted quality-of-life change that makes Bevy easier to use
labels
Oct 9, 2023
alice-i-cecile
approved these changes
Oct 9, 2023
alice-i-cecile
added
S-Ready-For-Final-Review
This PR has been approved by the community. It's ready for a maintainer to consider merging it
and removed
S-Ready-For-Final-Review
This PR has been approved by the community. It's ready for a maintainer to consider merging it
labels
Oct 9, 2023
@Trashtalk217 CI has a failing test: I think you need to update a serialization test string to match the new impl :) |
Something strange happened where on my end the serialization was returning |
MrGVSV
approved these changes
Oct 9, 2023
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 9, 2023
regnarock
pushed a commit
to regnarock/bevy
that referenced
this pull request
Oct 13, 2023
…ne#10068) Adopted from bevyengine#8954, co-authored by @pyrotechnick # Objective The Bevy ecosystem currently reflects `Quat` via "value" rather than the more appropriate "struct" strategy. This behaviour is inconsistent to that of similar types, i.e. `Vec3`. Additionally, employing the "value" strategy causes instances of `Quat` to be serialised as a sequence `[x, y, z, w]` rather than structures of shape `{ x, y, z, w }`. The [comments surrounding the applicable code](https://github.com/bevyengine/bevy/blob/bec299fa6e727a59d973fc8ca8f468732d40cb14/crates/bevy_reflect/src/impls/glam.rs#L254) give context and historical reasons for this discrepancy: ``` // Quat fields are read-only (as of now), and reflection is currently missing // mechanisms for read-only fields. I doubt those mechanisms would be added, // so for now quaternions will remain as values. They are represented identically // to Vec4 and DVec4, so you may use those instead and convert between. ``` This limitation has [since been lifted by the upstream crate](bitshifter/glam-rs@3746251), glam. ## Solution Migrating the reflect strategy of Quat from "value" to "struct" via replacing `impl_reflect_value` with `impl_reflect_struct` resolves the issue. ## Changelog Migrated `Quat` reflection strategy to "struct" from "value" Migration Guide Changed Quat serialization/deserialization from sequences `[x, y, z, w]` to structures `{ x, y, z, w }`. --------- Co-authored-by: pyrotechnick <13998+pyrotechnick@users.noreply.github.com> Co-authored-by: Alice Cecile <alice.i.cecile@gmail.com>
ameknite
pushed a commit
to ameknite/bevy
that referenced
this pull request
Nov 6, 2023
…ne#10068) Adopted from bevyengine#8954, co-authored by @pyrotechnick # Objective The Bevy ecosystem currently reflects `Quat` via "value" rather than the more appropriate "struct" strategy. This behaviour is inconsistent to that of similar types, i.e. `Vec3`. Additionally, employing the "value" strategy causes instances of `Quat` to be serialised as a sequence `[x, y, z, w]` rather than structures of shape `{ x, y, z, w }`. The [comments surrounding the applicable code](https://github.com/bevyengine/bevy/blob/bec299fa6e727a59d973fc8ca8f468732d40cb14/crates/bevy_reflect/src/impls/glam.rs#L254) give context and historical reasons for this discrepancy: ``` // Quat fields are read-only (as of now), and reflection is currently missing // mechanisms for read-only fields. I doubt those mechanisms would be added, // so for now quaternions will remain as values. They are represented identically // to Vec4 and DVec4, so you may use those instead and convert between. ``` This limitation has [since been lifted by the upstream crate](bitshifter/glam-rs@3746251), glam. ## Solution Migrating the reflect strategy of Quat from "value" to "struct" via replacing `impl_reflect_value` with `impl_reflect_struct` resolves the issue. ## Changelog Migrated `Quat` reflection strategy to "struct" from "value" Migration Guide Changed Quat serialization/deserialization from sequences `[x, y, z, w]` to structures `{ x, y, z, w }`. --------- Co-authored-by: pyrotechnick <13998+pyrotechnick@users.noreply.github.com> Co-authored-by: Alice Cecile <alice.i.cecile@gmail.com>
rdrpenguin04
pushed a commit
to rdrpenguin04/bevy
that referenced
this pull request
Jan 9, 2024
…ne#10068) Adopted from bevyengine#8954, co-authored by @pyrotechnick # Objective The Bevy ecosystem currently reflects `Quat` via "value" rather than the more appropriate "struct" strategy. This behaviour is inconsistent to that of similar types, i.e. `Vec3`. Additionally, employing the "value" strategy causes instances of `Quat` to be serialised as a sequence `[x, y, z, w]` rather than structures of shape `{ x, y, z, w }`. The [comments surrounding the applicable code](https://github.com/bevyengine/bevy/blob/bec299fa6e727a59d973fc8ca8f468732d40cb14/crates/bevy_reflect/src/impls/glam.rs#L254) give context and historical reasons for this discrepancy: ``` // Quat fields are read-only (as of now), and reflection is currently missing // mechanisms for read-only fields. I doubt those mechanisms would be added, // so for now quaternions will remain as values. They are represented identically // to Vec4 and DVec4, so you may use those instead and convert between. ``` This limitation has [since been lifted by the upstream crate](bitshifter/glam-rs@3746251), glam. ## Solution Migrating the reflect strategy of Quat from "value" to "struct" via replacing `impl_reflect_value` with `impl_reflect_struct` resolves the issue. ## Changelog Migrated `Quat` reflection strategy to "struct" from "value" Migration Guide Changed Quat serialization/deserialization from sequences `[x, y, z, w]` to structures `{ x, y, z, w }`. --------- Co-authored-by: pyrotechnick <13998+pyrotechnick@users.noreply.github.com> 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
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.
Adopted from #8954, co-authored by @pyrotechnick
Objective
The Bevy ecosystem currently reflects
Quat
via "value" rather than the more appropriate "struct" strategy. This behaviour is inconsistent to that of similar types, i.e.Vec3
. Additionally, employing the "value" strategy causes instances ofQuat
to be serialised as a sequence[x, y, z, w]
rather than structures of shape{ x, y, z, w }
.The comments surrounding the applicable code give context and historical reasons for this discrepancy:
This limitation has since been lifted by the upstream crate, glam.
Solution
Migrating the reflect strategy of Quat from "value" to "struct" via replacing
impl_reflect_value
withimpl_reflect_struct
resolves the issue.Changelog
Migrated
Quat
reflection strategy to "struct" from "value"Migration Guide
Changed Quat serialization/deserialization from sequences
[x, y, z, w]
to structures{ x, y, z, w }
.