- 
          
 - 
                Notifications
    
You must be signed in to change notification settings  - Fork 4.2k
 
          Migrate Quat reflection strategy from "value" to "struct"
          #10068
        
          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
          
     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 hidden or 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
  
              
              approved these changes
              
                  
                    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 
                  
              
              
            
            
    
  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 <[email protected]> Co-authored-by: Alice Cecile <[email protected]>
    
  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 <[email protected]> Co-authored-by: Alice Cecile <[email protected]>
    
  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 <[email protected]> Co-authored-by: Alice Cecile <[email protected]>
  
    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
Quatvia "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 ofQuatto 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_valuewithimpl_reflect_structresolves the issue.Changelog
Migrated
Quatreflection strategy to "struct" from "value"Migration Guide
Changed Quat serialization/deserialization from sequences
[x, y, z, w]to structures{ x, y, z, w }.