Skip to content

Implement Reflect for ParsedPath, OffsetAccess, and Access#23357

Closed
tauanbinato wants to merge 2 commits intobevyengine:mainfrom
tauanbinato:fix/issue-22974
Closed

Implement Reflect for ParsedPath, OffsetAccess, and Access#23357
tauanbinato wants to merge 2 commits intobevyengine:mainfrom
tauanbinato:fix/issue-22974

Conversation

@tauanbinato
Copy link
Copy Markdown
Contributor

Objective

Solution

  • Implemented Reflect for ParsedPath, OffsetAccess, and Access<'static>. Access<'static> is reflected as an opaque type via impl_reflect_opaque! since the lifetime parameter prevents using #[derive(Reflect)] directly. OffsetAccess and ParsedPath use #[derive(Reflect)] since their fields (including Access<'static>) all implement Reflect.

Testing

  • All existing bevy_reflect tests pass. Path-related doctests pass. Verified the types compile with Reflect derived.


impl_reflect_opaque!(
(in crate::path::access) Access<'a: 'static>(Clone, Debug, Hash, PartialEq)
);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it possible to reflect it as an Enum instead of an Opaque? Without the Enum implementation, using reflect to serialize/deserialize this will be more awkward. With the Enum implementation, everything should line up since Cow<'static, T> implements Reflect and the Serialize/Deserialize traits.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point, reflecting as an Enum would be better for serialization. I went with Opaque because of the lifetime on Access<'a>, but since we already constrain it to 'static and OffsetAccess only uses Access<'static>, it should be doable. Do you think it's worth trying to derive Reflect as an enum here?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for starting on this. Since you closed this PR, I created my own attempt at #23371. It looks like everything just worked correctly using the Reflect derive macro even with the lifetime parameter. Let me know if I missed something though.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Implement Reflect for ParsedPath

2 participants