[Merged by Bors] - Add macro to implement reflect for struct types and migrate glam types #4540

PROMETHIA-27 · 2022-04-19T18:55:36Z

Objective

Relevant issue: #4474

Currently glam types implement Reflect as a value, which is problematic for reflection, making scripting/editor work much more difficult. This PR re-implements them as structs.

Solution

Added a new proc macro, impl_reflect_struct, which replaces impl_reflect_value and impl_from_reflect_value for glam types. This macro could also be used for other types, but I don't know of any that would require it. It's specifically useful for foreign types that cannot derive Reflect normally.

Changelog

Added

impl_reflect_struct proc macro

Changed

Glam reflect impls have been replaced with impl_reflect_struct
from_reflect's impl_struct altered to take an optional custom constructor, allowing non-default non-constructible foreign types to use it
Calls to impl_struct altered to conform to new signature
Altered glam types (All vec/mat combinations) have a different serialization structure, as they are reflected differently now.

Migration Guide

This will break altered glam types serialized to RON scenes, as they will expect to be serialized/deserialized as structs rather than values now. A future PR to add custom serialization for non-value types is likely on the way to restore previous behavior. Additionally, calls to impl_struct must add a None parameter to the end of the call to restore previous behavior.

To be compatible with non-constructible foreign types

for compatibility with non-default non-constructible foreign types

To address issues where `use bevy_reflect::Struct` is ambiguous when called from bevy_reflect

crates/bevy_reflect/src/impls/glam.rs

MrGVSV · 2022-04-22T15:25:56Z

I could be wrong, but I believe this would break current scene formats which expect a tuple-like value:

{
  "type": "glam::vec3::Vec3",
  "value": (1.0, 1.0, 1.0),
},

This would then become:

{
  "type": "glam::vec3::Vec3",
  "struct": {
    "x": {
      "type": "f32",
      "value": 1.0
    },
    // ...
  },
},

This is probably okay, but worth mentioning in the migration guide at least.

PROMETHIA-27 · 2022-04-23T00:15:12Z

That makes sense. I'm having trouble confirming it from looking at bevy_serde, but I will try to test this soon. Will add it to the description.

MrGVSV

Overall, I think this is a good change. I was torn at first because I like the one-line tuples defined in the scene format, but I realize now that we can just define custom serde implementations if we really wanted to keep them like that. So I'm on board!

Left a few suggestions and some points for discussion.

One last thing I think this PR really needs is a set of tests. Could you also add some of those?

crates/bevy_reflect/bevy_reflect_derive/src/lib.rs

crates/bevy_reflect/src/impls/glam.rs

PROMETHIA-27 · 2022-04-23T18:46:00Z

I've verified that this change alters how glam types are serialized like you mentioned. If they're serialized directly, they work as normal (e.g. using serialize_ron() from bevy_scene::dynamic_scene) but when using ReflectSerializer they are unfolded as complex struct definitions. The reflect serializer/deserializer likely only respects custom serializers when that type is a value, not when it's a struct. Not sure where to proceed with that from here; maybe letting struct reflect types check for a custom serialization scheme and use that if present? I feel like that deserves its own PR though.

MrGVSV · 2022-04-23T21:31:29Z

The reflect serializer/deserializer likely only respects custom serializers when that type is a value, not when it's a struct. Not sure where to proceed with that from here; maybe letting struct reflect types check for a custom serialization scheme and use that if present? I feel like that deserves its own PR though.

Yep this is currently the case. I addressed this in #4561 to hopefully allow custom implementations to be used. But either way I think you’re okay not worrying about serialization 🙂

PROMETHIA-27 · 2022-04-23T22:04:52Z

Nice! I'll keep working on the other suggestions in the meantime then.

Co-authored-by: MrGVSV <[email protected]>

…METHIA-27/bevy into glam-vec_struct_reflect_type

crates/bevy_reflect/bevy_reflect_derive/src/from_reflect.rs

alice-i-cecile

This looks good. One nit about naming conventions; I didn't recognize ctor at a glance.

crates/bevy_reflect/bevy_reflect_derive/src/from_reflect.rs

Co-authored-by: Alice Cecile <[email protected]>

crates/bevy_reflect/bevy_reflect_derive/src/lib.rs

crates/bevy_reflect/bevy_reflect_derive/src/from_reflect.rs

crates/bevy_reflect/bevy_reflect_derive/src/lib.rs

Co-authored-by: Alice Cecile <[email protected]>

alice-i-cecile · 2022-05-09T16:31:58Z

bors r+

#4540) # Objective Relevant issue: #4474 Currently glam types implement Reflect as a value, which is problematic for reflection, making scripting/editor work much more difficult. This PR re-implements them as structs. ## Solution Added a new proc macro, `impl_reflect_struct`, which replaces `impl_reflect_value` and `impl_from_reflect_value` for glam types. This macro could also be used for other types, but I don't know of any that would require it. It's specifically useful for foreign types that cannot derive Reflect normally. --- ## Changelog ### Added - `impl_reflect_struct` proc macro ### Changed - Glam reflect impls have been replaced with `impl_reflect_struct` - from_reflect's `impl_struct` altered to take an optional custom constructor, allowing non-default non-constructible foreign types to use it - Calls to `impl_struct` altered to conform to new signature - Altered glam types (All vec/mat combinations) have a different serialization structure, as they are reflected differently now. ## Migration Guide This will break altered glam types serialized to RON scenes, as they will expect to be serialized/deserialized as structs rather than values now. A future PR to add custom serialization for non-value types is likely on the way to restore previous behavior. Additionally, calls to `impl_struct` must add a `None` parameter to the end of the call to restore previous behavior. Co-authored-by: PROMETHIA-27 <[email protected]>

bors · 2022-05-09T16:47:21Z

Pull request successfully merged into main.

Build succeeded:

bevyengine#4540) # Objective Relevant issue: bevyengine#4474 Currently glam types implement Reflect as a value, which is problematic for reflection, making scripting/editor work much more difficult. This PR re-implements them as structs. ## Solution Added a new proc macro, `impl_reflect_struct`, which replaces `impl_reflect_value` and `impl_from_reflect_value` for glam types. This macro could also be used for other types, but I don't know of any that would require it. It's specifically useful for foreign types that cannot derive Reflect normally. --- ## Changelog ### Added - `impl_reflect_struct` proc macro ### Changed - Glam reflect impls have been replaced with `impl_reflect_struct` - from_reflect's `impl_struct` altered to take an optional custom constructor, allowing non-default non-constructible foreign types to use it - Calls to `impl_struct` altered to conform to new signature - Altered glam types (All vec/mat combinations) have a different serialization structure, as they are reflected differently now. ## Migration Guide This will break altered glam types serialized to RON scenes, as they will expect to be serialized/deserialized as structs rather than values now. A future PR to add custom serialization for non-value types is likely on the way to restore previous behavior. Additionally, calls to `impl_struct` must add a `None` parameter to the end of the call to restore previous behavior. Co-authored-by: PROMETHIA-27 <[email protected]>

PROMETHIA-27 added 8 commits April 19, 2022 11:01

Implemented impl_reflect_struct

71a770f

Testing impl_reflect_struct

5860496

Rephrased impl_struct for from_reflect

478d9ce

To be compatible with non-constructible foreign types

Added from_reflect to impl_reflect_struct

650e7a7

Altered impl_struct to accept a custom constructor

fd4f58d

for compatibility with non-default non-constructible foreign types

Verified trait reflection

9a9aaf6

Switched all (less quats) impls to structs

401fe37

Removed messy temp test

6592abe

github-actions bot added the S-Needs-Triage This issue needs to be labelled label Apr 19, 2022

Ran cargo fmt

ac61dd2

alice-i-cecile added A-Reflection Runtime information about types A-Math Fundamental domain-agnostic mathematical operations and removed S-Needs-Triage This issue needs to be labelled labels Apr 19, 2022

Added bevy_reflect path override

44b06ba

To address issues where `use bevy_reflect::Struct` is ambiguous when called from bevy_reflect

ItsDoot reviewed Apr 21, 2022

View reviewed changes

crates/bevy_reflect/src/impls/glam.rs Show resolved Hide resolved

PROMETHIA-27 added 3 commits April 21, 2022 12:55

Restored impl_from_reflect_value for Quat/DQuat

b584f14

Added use for impl_from_reflect_value

fb4c12e

Ran cargo fmt again

1bdefbf

MrGVSV suggested changes Apr 23, 2022

View reviewed changes

PROMETHIA-27 added 2 commits April 23, 2022 16:19

Added tests for Vec3 serialization behavior

c5d8ce1

Ran cargo fmt --all

ed5372f

Split parsing into a separate Parse struct

44f66c9

alice-i-cecile added the C-Code-Quality A section of code that is hard to understand or change label Apr 25, 2022

PROMETHIA-27 and others added 2 commits April 25, 2022 15:08

Added tests for Vec3 reflection behavior

b6c196e

Merge branch 'main' into glam-vec_struct_reflect_type

cbbae2b

PROMETHIA-27 and others added 6 commits May 8, 2022 14:59

Explain type must be in scope

9a73dfe

Explain example

68fef52

Show Vec3 in scope

467a39a

.into -> TokenStream::from

2f63312

Co-authored-by: MrGVSV <[email protected]>

Merge branch 'glam-vec_struct_reflect_type' of https://github.com/PRO…

c262d94

…METHIA-27/bevy into glam-vec_struct_reflect_type

Cargo fmt --all

7aa58db

MrGVSV approved these changes May 8, 2022

View reviewed changes

alice-i-cecile reviewed May 9, 2022

View reviewed changes

crates/bevy_reflect/bevy_reflect_derive/src/from_reflect.rs Outdated Show resolved Hide resolved

alice-i-cecile requested changes May 9, 2022

View reviewed changes

crates/bevy_reflect/bevy_reflect_derive/src/from_reflect.rs Outdated Show resolved Hide resolved

crates/bevy_reflect/bevy_reflect_derive/src/from_reflect.rs Outdated Show resolved Hide resolved

PROMETHIA-27 and others added 3 commits May 9, 2022 12:11

ctor -> constructor

05f4f0b

Co-authored-by: Alice Cecile <[email protected]>

ctor -> constructor

7683aa9

Co-authored-by: Alice Cecile <[email protected]>

ctor -> constructor

58e127c

Co-authored-by: Alice Cecile <[email protected]>

alice-i-cecile reviewed May 9, 2022

View reviewed changes

crates/bevy_reflect/bevy_reflect_derive/src/lib.rs Outdated Show resolved Hide resolved

alice-i-cecile reviewed May 9, 2022

View reviewed changes

crates/bevy_reflect/bevy_reflect_derive/src/from_reflect.rs Outdated Show resolved Hide resolved

alice-i-cecile reviewed May 9, 2022

View reviewed changes

crates/bevy_reflect/bevy_reflect_derive/src/from_reflect.rs Outdated Show resolved Hide resolved

alice-i-cecile reviewed May 9, 2022

View reviewed changes

crates/bevy_reflect/bevy_reflect_derive/src/lib.rs Outdated Show resolved Hide resolved

PROMETHIA-27 and others added 2 commits May 9, 2022 12:17

ctor -> constructor

ca936c8

last ctor -> constructor batch

b7d76fb

Co-authored-by: Alice Cecile <[email protected]>

alice-i-cecile approved these changes May 9, 2022

View reviewed changes

bors bot changed the title ~~Add macro to implement reflect for struct types and migrate glam types~~ [Merged by Bors] - Add macro to implement reflect for struct types and migrate glam types May 9, 2022

bors bot closed this May 9, 2022

MrGVSV mentioned this pull request May 9, 2022

[Merged by Bors] - bevy_reflect: Add #[reflect(default)] attribute for FromReflect #4140

Closed

jakobhellermann mentioned this pull request May 13, 2022

Tracking issue: dynamic APIs and reflection improvements for scripting #4474

Closed

5 tasks

MrGVSV mentioned this pull request May 16, 2022

[Merged by Bors] - bevy_reflect: Reflect enums #4761

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Merged by Bors] - Add macro to implement reflect for struct types and migrate glam types #4540

[Merged by Bors] - Add macro to implement reflect for struct types and migrate glam types #4540

PROMETHIA-27 commented Apr 19, 2022 •

edited

Loading

MrGVSV commented Apr 22, 2022

PROMETHIA-27 commented Apr 23, 2022

MrGVSV left a comment •

edited

Loading

PROMETHIA-27 commented Apr 23, 2022

MrGVSV commented Apr 23, 2022

PROMETHIA-27 commented Apr 23, 2022

alice-i-cecile left a comment

alice-i-cecile commented May 9, 2022

bors bot commented May 9, 2022

[Merged by Bors] - Add macro to implement reflect for struct types and migrate glam types #4540

[Merged by Bors] - Add macro to implement reflect for struct types and migrate glam types #4540

Conversation

PROMETHIA-27 commented Apr 19, 2022 • edited Loading

Objective

Solution

Changelog

Added

Changed

Migration Guide

MrGVSV commented Apr 22, 2022

PROMETHIA-27 commented Apr 23, 2022

MrGVSV left a comment • edited Loading

Choose a reason for hiding this comment

PROMETHIA-27 commented Apr 23, 2022

MrGVSV commented Apr 23, 2022

PROMETHIA-27 commented Apr 23, 2022

alice-i-cecile left a comment

Choose a reason for hiding this comment

alice-i-cecile commented May 9, 2022

bors bot commented May 9, 2022

PROMETHIA-27 commented Apr 19, 2022 •

edited

Loading

MrGVSV left a comment •

edited

Loading