-
-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
Add no_std
support to bevy_reflect
#16256
base: main
Are you sure you want to change the base?
Conversation
Tested with my demo UEFI application and confirmed it works as expected, letting me derive |
IIUC it's possible to make |
I'll give this another look to see if |
Actually, reflection uses a lot of |
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.
Overall looks great! A lot less involved than I thought it'd be.
Just a few questions (mainly for my own clarity and understanding).
@@ -4,6 +4,9 @@ use core::{ | |||
slice::Iter, | |||
}; | |||
|
|||
#[cfg(not(feature = "std"))] |
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.
Why do we use this attribute here but not on any of the other alloc
imports?
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.
So this is an interesting one. Basically I added these imports without the feature gate to ensure no_std
compiled, and then checked compilation with std
. In the cases were Clippy said these imports weren't used I added the feature gate. So in short, I don't have a good explanation as to why sometimes it's needed and sometimes it's not, but the process used to arrive here was very mechanical ("clippy said so")
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.
Thank you Clippy, very helpful
…entation Co-Authored-By: Gino Valente <49806985+MrGVSV@users.noreply.github.com>
Instead of embedding an `extern crate alloc;` Co-Authored-By: Gino Valente <49806985+MrGVSV@users.noreply.github.com>
Moved `std` feature gate inside `impl_reflect_for_atomic` explicitly for `ReflectDe/Serialize` Co-Authored-By: Gino Valente <49806985+MrGVSV@users.noreply.github.com>
Co-Authored-By: Gino Valente <49806985+MrGVSV@users.noreply.github.com>
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.
Awesome work! Can't wait to have reflection on my smart mirror!
Hey, I've never touched Bevy before but I do have a strong opinion against spin locks. Would it be better to use |
That looks perfect honestly, thanks for the advice! I'll update this PR to use |
Ok @GnomedDev I've spent a good couple of hours working with
Point 1 could be addressed by just accepting that we lose the concurrency boost an
The problem being if a write lock is requested between steps 1 and 3 (remembering that this time-frame can be large, since multiple readers are valid), the CS isn't actually held, so the writing thread would acquire the CS, see that the lock is in use, and then have no way to park execution, other than just spinning. To confirm this belief, I attempted to find a pre-made So, where to from here? In my opinion, a |
Objective
no_std
Bevy #15460Solution
std
feature (enabled by default)Testing
cargo check -p bevy_reflect --no-default-features --target "x86_64-unknown-none"
bevy_reflect
, allowingderive(Reflect)
Notes
spin
crate has been included to provideRwLock
andOnce
(as an alternative toOnceLock
) when thestd
feature is not enabled. Another alternative may be more desirable, please provide feedback if you have a strong opinion here!Box
,String
,ToString
) provided byalloc
have been added to__macro_exports
as a way to avoidalloc
vsstd
namespacing. I'm personally quite annoyed that we can't rely onalloc
as a crate name instd
environments within macros. I'd love an alternative to my approach here, but I suspect it's the least-bad option.alloc
feature (for allocation-freebevy_reflect
), unfortunately,erased_serde
unconditionally requires access toBox
. Maybe one day we could design around this, but for now it just meansbevy_reflect
requiresalloc
.