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.

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 by Bors] - implement reflection for more glam types #5194

Closed
wants to merge 8 commits into from

Conversation

makspll
Copy link
Contributor

@makspll makspll commented Jul 4, 2022

Objective

  • To implement Reflect for more glam types.

Solution

insert impl_reflect_struct invocations for more glam types. I am not sure about the boolean vectors, since none of them implement Serde::Serialize/Deserialize, and the SIMD versions don't have public fields.
I do still think implementing reflection is useful for BVec's since then they can be incorporated into Reflect'ed components and set dynamically even if as a whole + it's more consistent.

Changelog

Implemented Reflect for the following types

  • BVec2
  • BVec3
  • BVec3A (on simd supported platforms only)
  • BVec4
  • BVec4A (on simd supported platforms only)
  • Mat2
  • Mat3A
  • DMat2
  • Affine2
  • Affine3A
  • DAffine2
  • DAffine3
  • EulerRot

@alice-i-cecile alice-i-cecile added C-Usability A targeted quality-of-life change that makes Bevy easier to use A-Reflection Runtime information about types labels Jul 4, 2022
@alice-i-cecile alice-i-cecile added the A-Math Fundamental domain-agnostic mathematical operations label Jul 4, 2022
@makspll
Copy link
Contributor Author

makspll commented Jul 4, 2022

Added an implementation for EulerRot too, but that one on top of no Serialize and Deserialize also doesn't implement PartialEq:

 impl_reflect_value!(EulerRot(Debug, Default));

I think that's all of glam ?
EDIT: there's also XY, XYZ, XYZW but I doubt those need reflection

@makspll
Copy link
Contributor Author

makspll commented Jul 4, 2022

Hmm not sure what's happening with the wasm builds,
I think BVec3/4A end up as type aliases to BVec3/4 on wasm due to no SIMD support,

Should I conditionally disable those macros on wasm, or not have the SIMD versions at all ?

@makspll
Copy link
Contributor Author

makspll commented Jul 5, 2022

I've looked into this and the relevant glam LOC are here: https://github.com/bitshifter/glam-rs/blob/58285f7b127cb93b314e22344bac678b9ba4298f/src/bool.rs#L25-L35.

There is a glam issue about this behaviour here: bitshifter/glam-rs#306,
It do be annoying.

For now these reflect impls are behind SIMD features consistent with the glam type aliases, but this might be surprising for people building for multiple platforms.

@mockersf
Copy link
Member

mockersf commented Jul 5, 2022

please mention the glam issue in the comment 👍

Co-authored-by: François <mockersf@gmail.com>
@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 Jul 5, 2022
@alice-i-cecile
Copy link
Member

bors r+

bors bot pushed a commit that referenced this pull request Jul 5, 2022
# Objective

- To implement `Reflect` for more glam types.  

## Solution

insert `impl_reflect_struct` invocations for more glam types. I am not sure about the boolean vectors, since none of them implement `Serde::Serialize/Deserialize`, and the SIMD versions don't have public fields. 
I do still think implementing reflection is useful for BVec's since then they can be incorporated into `Reflect`'ed components and set dynamically even if as a whole + it's more consistent.

## Changelog
Implemented `Reflect` for the following types
 - BVec2
 - BVec3
 - **BVec3A** (on simd supported platforms only)
 - BVec4
 - **BVec4A** (on simd supported platforms only)
 - Mat2
 - Mat3A
 - DMat2
 - Affine2
 - Affine3A
 - DAffine2
 - DAffine3
 - EulerRot
@bors bors bot changed the title implement reflection for more glam types [Merged by Bors] - implement reflection for more glam types Jul 5, 2022
@bors bors bot closed this Jul 5, 2022
@bitshifter
Copy link
Contributor

What is the use case for serializing or reflecting the BVec* types? The intention of these types in glam is as a temporary value typically created as outputs from the cmp* methods for input into Vec*::select. These are optimised for SIMD where everything can live in vector registers and branches can be avoided. For scalar types there is a bool fall-back. I didn't anticipate people wanting to serialize them, so it would be good to understand why this is desired.

I'm also curious about the use case for reflecting EulerRot. glam is coordinate system agnostic, so it aims to support any possible coordinate system. AFAIK Bevy has a defined 2D and 3D coordinate systems and I would have thought it would consistently use a single EulerRot order everywhere?

@makspll
Copy link
Contributor Author

makspll commented Jul 5, 2022

So I think the main argument for reflecting those is consistency, as an example, I am writing bevy_mod_scripting and in order to support the bevy API I am wrapping all of the bevy types in Lua supporting wrappers.
Now with the way bevy reflect is heading, I am assuming that anything which is to be available to scripts will implement Reflect. Things which don't must be treated differently since they cannot be part of components of resources which are in fact Reflect'able, If all of glam implements Reflect, I don't need to worry about this. But even if these types implement Reflect this doesn't mean they won't just be temporaries, it's more of a convenience for treating types uniformly.

But that's just why I personally prefer this approach. Other than that, I don't have anything specific in mind but I also don't see why these shouldn't be supported as subfields of components or resources. There may be a neat use case for boolean vectors or serializing them, why should we limit the non-rust user?

Same reasoning applies for EulerRot, while bevy has an established preference for coordinates, someone might prefer not to use this and roll their own transforms.

So my argument here is basically, why not?

Now Serialization is not really that big of a deal here, the API difference between platforms is however. I guess if performance would be much worse with the alternative consistent API though, then platform-consistent-reflecting has lower priority than performance of the other glam operations.

@bitshifter
Copy link
Contributor

bitshifter commented Jul 5, 2022

Thanks for the explanation, exposing to script does make a lot of sense. To be clear I'm not opposing this but I did want to understand the why a bit better. The performance question issue is a bit theoretical at the moment, it would require a bit of work to determine if it was a real problem. For example, BVec* could go back to containing u32 components of 0 or 0xffffffff like they used to in the early glam days, so the type is the same between SIMD and scalar code. But it might be a performance hit when you aren't using SIMD, which for bevy which mostly uses Vec3 could be an issue. The simpler thing to do would be to have a scalar implementations of BVec3A and BVec4A instead of aliasing them when SIMD isn't available. Then any perf hit will only be on platforms that don't have a SIMD implementation yet, like aarch64. It might be an issue for SPIR-V (rust-gpu) as well, I'm not sure.

I probably should have started this discussion on the glam issue but I was interested in the bevy context :)

@makspll
Copy link
Contributor Author

makspll commented Jul 5, 2022

Yeah no worries, I get why you'd want context!

The scalar implementation probably sounds like the best solution in the context of bevy, but I have no expertise in this area

inodentry pushed a commit to IyesGames/bevy that referenced this pull request Aug 8, 2022
# Objective

- To implement `Reflect` for more glam types.  

## Solution

insert `impl_reflect_struct` invocations for more glam types. I am not sure about the boolean vectors, since none of them implement `Serde::Serialize/Deserialize`, and the SIMD versions don't have public fields. 
I do still think implementing reflection is useful for BVec's since then they can be incorporated into `Reflect`'ed components and set dynamically even if as a whole + it's more consistent.

## Changelog
Implemented `Reflect` for the following types
 - BVec2
 - BVec3
 - **BVec3A** (on simd supported platforms only)
 - BVec4
 - **BVec4A** (on simd supported platforms only)
 - Mat2
 - Mat3A
 - DMat2
 - Affine2
 - Affine3A
 - DAffine2
 - DAffine3
 - EulerRot
james7132 pushed a commit to james7132/bevy that referenced this pull request Oct 28, 2022
# Objective

- To implement `Reflect` for more glam types.  

## Solution

insert `impl_reflect_struct` invocations for more glam types. I am not sure about the boolean vectors, since none of them implement `Serde::Serialize/Deserialize`, and the SIMD versions don't have public fields. 
I do still think implementing reflection is useful for BVec's since then they can be incorporated into `Reflect`'ed components and set dynamically even if as a whole + it's more consistent.

## Changelog
Implemented `Reflect` for the following types
 - BVec2
 - BVec3
 - **BVec3A** (on simd supported platforms only)
 - BVec4
 - **BVec4A** (on simd supported platforms only)
 - Mat2
 - Mat3A
 - DMat2
 - Affine2
 - Affine3A
 - DAffine2
 - DAffine3
 - EulerRot
ItsDoot pushed a commit to ItsDoot/bevy that referenced this pull request Feb 1, 2023
# Objective

- To implement `Reflect` for more glam types.  

## Solution

insert `impl_reflect_struct` invocations for more glam types. I am not sure about the boolean vectors, since none of them implement `Serde::Serialize/Deserialize`, and the SIMD versions don't have public fields. 
I do still think implementing reflection is useful for BVec's since then they can be incorporated into `Reflect`'ed components and set dynamically even if as a whole + it's more consistent.

## Changelog
Implemented `Reflect` for the following types
 - BVec2
 - BVec3
 - **BVec3A** (on simd supported platforms only)
 - BVec4
 - **BVec4A** (on simd supported platforms only)
 - Mat2
 - Mat3A
 - DMat2
 - Affine2
 - Affine3A
 - DAffine2
 - DAffine3
 - EulerRot
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Math Fundamental domain-agnostic mathematical operations 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
Archived in project
Development

Successfully merging this pull request may close these issues.

4 participants