-
-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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
reflect: remove manual Reflect
impls which could be handled by macros
#12025
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Amazing! Just one comment (and one non-blocking question)
@@ -52,7 +47,8 @@ use thiserror::Error; | |||
/// This means that the common case of `asset_server.load("my_scene.scn")` when it creates and | |||
/// clones internal owned [`AssetPaths`](AssetPath). | |||
/// This also means that you should use [`AssetPath::parse`] in cases where `&str` is the explicit type. | |||
#[derive(Eq, PartialEq, Hash, Clone, Default)] | |||
#[derive(Eq, PartialEq, Hash, Clone, Default, Reflect)] | |||
#[reflect_value(Debug, PartialEq, Hash, Serialize, Deserialize)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't have to update it in this PR, but any reason why this is a reflect_value
type? I would have assumed CowArc
would be reflectable, but maybe not?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM sans @MrGVSV's comments.
Co-authored-by: James Liu <contact@jamessliu.com>
@@ -67,7 +63,9 @@ bitflags::bitflags! { | |||
/// If you have an asset that doesn't actually need to end up in the render world, like an Image | |||
/// that will be decoded into another Image asset, use `MAIN_WORLD` only. | |||
#[repr(transparent)] | |||
#[derive(Serialize, TypePath, Deserialize, Hash, Clone, Copy, PartialEq, Eq, Debug)] | |||
#[derive(Serialize, Deserialize, Hash, Clone, Copy, PartialEq, Eq, Debug, Reflect)] | |||
#[reflect(Serialize, Deserialize, Hash, PartialEq, Debug, Reflect)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How is CI passing if we never import ReflectSerialize
et al? 🤔
@soqb can you resolve merge conflicts? They're not trivial so I'm not going to tackle them in the web client. |
not this week i'm afraid, but i do plan on it when there's time. |
I'm interested in adopting this PR, I think I can fix the merge conflicts at least |
…#12596) # Objective * Adopted #12025 to fix merge conflicts * In some cases we used manual impls for certain types, though they are (at least, now) unnecessary. ## Solution * Use macros and reflecting-by-value to avoid this clutter. * Though there were linker issues with Reflect and the CowArc in AssetPath (see #9747), I checked these are resolved by using #[reflect_value]. --------- Co-authored-by: soqb <cb.setho@gmail.com> Co-authored-by: Alice Cecile <alice.i.cecile@gmail.com> Co-authored-by: James Liu <contact@jamessliu.com>
Objective
Solution
Reflect
and theCowArc
inAssetPath
(see Build failure whendynamic_linking
feature is used #9747), I checked these are resolved by using#[reflect_value]
.