[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

Uh oh!

[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

Uh oh!

Conversation

PROMETHIA-27 commented Apr 19, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Objective

Solution

Changelog

Added

Changed

Migration Guide

Uh oh!

Uh oh!

MrGVSV commented Apr 22, 2022

Uh oh!

PROMETHIA-27 commented Apr 23, 2022

Uh oh!

MrGVSV left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

PROMETHIA-27 commented Apr 23, 2022

Uh oh!

MrGVSV commented Apr 23, 2022

Uh oh!

PROMETHIA-27 commented Apr 23, 2022

Uh oh!

Uh oh!

alice-i-cecile left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

alice-i-cecile commented May 9, 2022

Uh oh!

bors bot commented May 9, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

PROMETHIA-27 commented Apr 19, 2022 •

edited

Loading

MrGVSV left a comment •

edited

Loading