Skip to content
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

bevy_reflect: Implement Reflect support for various rand_* crates #9512

Closed
wants to merge 1 commit into from

Conversation

Bluefinger
Copy link
Contributor

Objective

As a follow-up to #9140, implementing Reflect on various rand PRNG crates will allow bevy_rand to do away with the TypePath override and have fully stable TypePath without any std::any:type_name fallback for the generic wrapper component/resource.

Solution

Provide opt-in reflection implementations for several rand crates. The features for each crate are optional and require explicit opt-in in bevy_reflect, so this will not add any additional dependencies on a default bevy install.

A number of crates were chosen on providing a selection of PRNG algorithm choices (ChaCha for cryptographically secure PRNG, PCG + Xoshiro + Wyrand for non-cryptographic PRNGs) so users can select the best ones for their needs. rand types like StdRng and SmallRng are not provided as they are not portable/stable. They have no guarantees that the underlying algorithms will be stable between different versions of rand, and for SmallRng, it has a different algorithm depending if you are on a 64-bit or 32-bit platform. As such, those who are prioritising usage of those types are not going to care about determinism nor about serialising/deserialising these types, and will be using them directly (via thread_rng et al) instead of trying to integrate them into the ECS. OsRng is based on hardware/OS sources, so it is not a serialisable type anyway. rand itself recommends from their docs that if one wants to prioritise stability/portability of PRNGs, to use the algorithm crates like rand_chacha directly.

This fixes issues in bevy_rand with it having non-stable TypePath.


Changelog

Added

  • Implement Reflect support for various rand_* crates

@alice-i-cecile alice-i-cecile added C-Usability A targeted quality-of-life change that makes Bevy easier to use A-Reflection Runtime information about types labels Aug 20, 2023
@mockersf
Copy link
Member

mockersf commented Aug 20, 2023

I'm a little reluctant to add this to Bevy directly. Some of those crates haven't been updated in the last two years, the one that was updated recently has very little users, and most are not applicable / interesting for most users.

I know the orphan rule is a pain around that, but I don't think bevy_reflect should end up with optional features for every case existing.

@mockersf mockersf added the X-Controversial There is active debate or serious implications around merging this PR label Aug 20, 2023
@Bluefinger
Copy link
Contributor Author

Bluefinger commented Aug 21, 2023

These are algorithm crates, which are by definition not really going to update that often once you write the algorithm itself (bar changes to traits like RngCore, which would then necessitate updating if there are breaking changes).

rand_chacha might have been last updated two years ago, but it is literally used and maintained by the rand team. StdRng in rand is pretty much ChaCha12Rng underneath (and what drives thread_rng).

The last one is just the WyRand algorithm, same as used in fastrand, turborand, but just the algorithm itself for the rand ecosystem. I actually did try to get rand-wyrand updated with the traits needed for this, but the author never published a new version/tag, so I had to spin off what I did to its own crate (as I myself wanted to use this algorithm given its performance, properties and wide usage already within fastrand).

So, these crates pretty much cover like 95% of PRNG usages/needs. Plus, there is a need to be able to serialise/deserialise PRNG states for things like bevyengine/rfcs#55, and reflection support is going to go a long way in helping make dealing with RNG in a way I can integrate well with the ECS.

Now, I could remove some of the listed crates, but then that's bevy blessing certain things over others, and I would presume that would be something that would need good reasoning/rationale. bevy_rand for example is written to not be opinionated on that matter.

The point of rand is that it is generic and can just plug in whatever algorithm/source you want for your PRNG needs. But to have that same 'plug-in'-ability, I need to extend Reflect support to these types, which I then can use to have proper reflect support for bevy_rand and resolve TypePath stability issues that arise from std::any::type_name. It also makes it far easier for others to wrap these types into Components/Resources with reflection support without quirky workarounds which I'm not even sure are kosher with their usage.

We already have the precedent of adding crates to bevy_reflect because of orphan rule restrictions. I honestly would prefer not to have to do this for optional use-cases, but the reality is that Rust lacks reflection support natively, bevy has its own reflection thing going, and orphan rules means this is what needs to occur. If you are going to bar adding reflection support to various external crates because of them not being entirely applicable for all use-cases, then a lot of the rust ecosystem is going to be barred from being able to be integrated into bevy by virtue of a lot of crates not wanting to add extra features (and thus dependencies) on their end.

Copy link
Contributor

@soqb soqb left a comment

Choose a reason for hiding this comment

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

as much as the implementation here is rock solid, and i empathise with and share your frustration at Rust's orphan rules, i also do harbour my own concerns.

this feels sort of arbitrary to me, admittedly i would say the same thing about #8771 but we don't even have a bevy_entropy sub-crate or similar.

while the root of the problem is definitely Rust's orphan rules, i wonder if there's another approach than to submit to them.

i'm most definitely spitballing here, but here's a potential solution i'd be willing to turn into a PR:

// in `bevy_rand`

local_type_path!(TypePathRand); // the same definition as `TypePath`.
impl_type_path!(TypePathRand for ::rand_chacha::ChaCha8Rng); // etc.

#[derive(TypePath)]
#[type_path(bounds(T: TypePathRand))]
struct EntropyFoo<T>(..);

#[derive(TypePath)]
#[type_path(local = TypePathRand)]
struct LocallyDefinedRand;
  • unfortunately due to language limitations, we can't satisfy both of the following, so this just moves the limitations of orphan rules to the downstream crate:
    • impl<T> TypePath for EntropyFoo<T> where T: TypePath
    • impl<T> TypePath for EntropyFoo<T> where T: TypePathRand
  • lexically this is also sort of clunky, and further complicates the impl_type_path macro. can we alleviate this?

@Bluefinger
Copy link
Contributor Author

@soqb I'm already moving away from this and doing something on my crate end (actually, I'm splitting that effort into a new, separate crate), but it is effectively newtypes for a bunch of rand_core, PRNG crates.

What motivated me to at least try here is because I kinda view newtypes as poor Dev experience and a bit counter to Bevy's goals for good DX. Newtyping also complicates serialization formats, so where possible, I try to avoid them and keep things as "flat" as possible. But for now, it seems like that is unavoidable, and if unavoidable, I'd rather create something that is reusable for the ecosystem than necessarily lock users down to bevy_rand. Your idea is a nice one, but in the end, I think it is just simpler to go with newtyping that I can at least make things work with reflection (reflect_value ftw) and not so clunky to the users. And by putting into a crate, I remove the boilerplate that would be foisted to users.

@Bluefinger
Copy link
Contributor Author

Closing this as I've gone and released a different approach in the mean time with Bluefinger/bevy_rand#9

@Bluefinger Bluefinger closed this Aug 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Reflection Runtime information about types C-Usability A targeted quality-of-life change that makes Bevy easier to use X-Controversial There is active debate or serious implications around merging this PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants