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

Provide bevy_entropy crate for RNG sources within ecs #7871

Closed
wants to merge 28 commits into from

Conversation

Bluefinger
Copy link
Contributor

@Bluefinger Bluefinger commented Mar 2, 2023

Objective

  • rand_core and rand are the defacto standard of handling PRNGs in the rust ecosystem. There's a wide ecosystem behind it and rand continues to be maintained and expanded upon by a whole community behind it.
  • Therefore, it is unlikely that there is much need for many third-party crates for handling RNGs in bevy, unless it is some small, custom crate that does not support RngCore (like with fastrand).
  • Incorporating RNGs into an ECS environment and in a way that allows for both determinism and also parallelism is not trivial. There's been discussions about this in previous RFCs, such as Deterministic RNG rfcs#55.

Solution

  • Create a crate for integrating any PRNG that implements RngCore and SeedableRng, as well as additional traits for supporting reflection (serde Serialize and Deserialize). The crate exposes a plugin for easy integration, or the components/resource for applying/accessing PRNGs in a way that will work well in an ECS environment. Ideas here are all from lessons learned from bevy_turborand.
  • The API provides generic plugin/component/resource that will accept an RngCore PRNG, and the component and resource itself also implements RngCore to allow for easy use with rand traits and features.
  • Using the plugin to incorporate a PRNG will automatically register the types for the resource and component for that particular PRNG, and then initialise the resource with either a random seed sourced from system entropy, or with a seed if provided.
  • RNG usage will require mut access given the RngCore API, so accessing GlobalEntropy resource will greatly limit parallelism of systems querying it. Instead, RNG sources should be moved to entities via EntropyComponent, to allow for increased parallelism for groupings of systems/entities that do not overlap each other with mut accesses of the RNG sources.
  • Determinism still requires careful consideration, but by making use of components/resources, it becomes easier to reason about how an RNG source is accessed and avoids the pitfalls of relying on a global instance and removing all parallelism from one's game.
  • rand on its own brings in a lot of code, as well as additional PRNGs. For focusing on determinism, that also means determinism in algorithm used for seeding entropy, as rand makes no guarantee that the output of thread_rng is deterministic in terms of underlying algorithm. Same with StdRng and SmallRng. rand recommends using PRNG crates directly if determinism of output is important. Therefore, this crate implements everything needed with just rand_core as the base, leaving it to developers to decide on when to import rand and what features they use from rand.

Example setup of bevy_entropy:

use bevy::prelude::*;
use rand_core::RngCore;
use rand_chacha::ChaCha8Rng;

fn main() {
  App::new()
    .add_plugins(DefaultPlugins)
    .add_plugin(EntropyPlugin::<ChaCha8Rng>::default())
    .add_system(print_random_value)
    .run();
}

fn print_random_value(mut rng: ResMut<GlobalEntropy<ChaCha8Rng>>) {
  println!("Random value: {}", rng.next_u32());
}

Todos

  • Initial API implementation, from plugin to component and resource.
  • Documentation. Provide full details/explanations of patterns and usage of the components/resource.
  • Examples. Deterministic output even with parallelised systems would be a nice demonstration of the approaches here.
  • Tests. Ensure PRNG integration is tested and works well.

Open Questions

  • Splitting up the crate further with features. Do we need to pull in rand_chacha by default, or can we fallback to OS entropy sources for components? There's overheads here potentially, but someone might want less deps or other use-cases.
  • Optional reflection support. Is there a way to have optional reflection support with the generics bounds? Being able to remove the need for serialisation support would be a nice to have with this crate.

Also, all naming in this crate is subject to further bikeshedding, and feedback on the initial API is welcome.


Changelog

Added

  • bevy_entropy crate added for integrating RNGs into bevy.

@github-actions
Copy link
Contributor

github-actions bot commented Mar 2, 2023

Welcome, new contributor!

Please make sure you've read our contributing guide and we look forward to reviewing your pull request shortly ✨

@harudagondi
Copy link
Member

Is there a reason why this shouldn't be just a third party crate?

@Bluefinger
Copy link
Contributor Author

Is there a reason why this shouldn't be just a third party crate?

There was interest in a first-party solution, and the defacto standard for RNG in the rust ecosystem is rand. So, it is unlikely there'd be many different crates/approaches to this. I maintain bevy_turborand for example, but that's for supporting turborand primarily. Integrating a defacto standard crate seems like something bevy could offer as an optional but first-party feature.

@github-actions
Copy link
Contributor

github-actions bot commented Mar 2, 2023

Your PR increases Bevy Minimum Supported Rust Version. Please update the rust-version field in the root Cargo.toml file.

3 similar comments
@github-actions
Copy link
Contributor

github-actions bot commented Mar 2, 2023

Your PR increases Bevy Minimum Supported Rust Version. Please update the rust-version field in the root Cargo.toml file.

@github-actions
Copy link
Contributor

github-actions bot commented Mar 2, 2023

Your PR increases Bevy Minimum Supported Rust Version. Please update the rust-version field in the root Cargo.toml file.

@github-actions
Copy link
Contributor

github-actions bot commented Mar 2, 2023

Your PR increases Bevy Minimum Supported Rust Version. Please update the rust-version field in the root Cargo.toml file.

@alice-i-cecile alice-i-cecile added X-Controversial There is active debate or serious implications around merging this PR C-Feature A new feature, making something new possible labels Mar 2, 2023
@alice-i-cecile alice-i-cecile added this to the 0.11 milestone Mar 2, 2023
@LegNeato
Copy link
Contributor

LegNeato commented Mar 4, 2023

Is there a reason why this shouldn't be just a third party crate?

The previously mentioned RFC goes into reasoning: https://github.com/LegNeato/bevy-rfcs/blob/entropy/rfcs/55-deterministic_rng.md#motivation

@Bluefinger Bluefinger marked this pull request as ready for review March 8, 2023 18:36
@LegNeato
Copy link
Contributor

What about using the hasher as a source of entropy?

https://matklad.github.io/2023/01/04/on-random-numbers.html

@LegNeato
Copy link
Contributor

Also, feel free to grab any of the user level guide stuff from the RFC

@Bluefinger
Copy link
Contributor Author

Bluefinger commented Mar 11, 2023

What about using the hasher as a source of entropy?

https://matklad.github.io/2023/01/04/on-random-numbers.html

getrandom is a bit more extensive in the support of platforms than what is on the std with RandomState, though the two do overlap a fair bit. For broader platform support, getrandom is what is used by rand_core and supports passing a byte slice, whereas RandomState will need to be built into a hasher to then output the necessary bytes (per u64) to fill an arbitrary byte slice. I think also RandomState is a bit iffy when it comes to WASM environments, which getrandom has the necessary features to resolve.

@Bluefinger
Copy link
Contributor Author

As a further follow-up, RandomState relies on sys::hashmap_random_keys(), which for unsupported platforms, is not random. And browser WASM is an unsupported platform that will yield non-random state (you need WASI for RandomState to be random again on std platform. So, to better support entropy sourcing for WASM, we kinda need getrandom.

@LegNeato
Copy link
Contributor

Ah yeah, great point

@alice-i-cecile alice-i-cecile added the A-Math Fundamental domain-agnostic mathematical operations label Mar 28, 2023
Copy link
Member

@MrGVSV MrGVSV left a comment

Choose a reason for hiding this comment

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

Just some reflection-related comments

)]
#[cfg_attr(
all(feature = "serialize", feature = "bevy_reflect"),
reflect_value(Debug, PartialEq, Serialize, Deserialize)
Copy link
Member

Choose a reason for hiding this comment

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

This should likely also register the following type data: ReflectFromReflect and (if we want this to be used in scenes) ReflectComponent.

Copy link
Contributor Author

@Bluefinger Bluefinger Apr 13, 2023

Choose a reason for hiding this comment

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

Looks like this basically forces the use of only seedable RNG sources, so SeedableRng only types. That at least removes the possibility of using something like OsRng from rand_core, so ensuring deterministic algorithms only and no hardware/OS RNG sources.

I'm okay with that though, as this then ensures people have to provide a user-land algorithm.

Comment on lines 106 to 109
#[cfg_attr(
all(feature = "serialize", feature = "bevy_reflect"),
reflect_value(Debug, PartialEq, Serialize, Deserialize)
)]
Copy link
Member

Choose a reason for hiding this comment

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

We probably want to split this into two attributes, right? That way we can keep the reflection registrations without needing the serde ones (I know the bevy_reflect feature relies on serialize, but just for clarity and ease of separation in the future).

Comment on lines 36 to 39
#[cfg_attr(
all(feature = "serialize", feature = "bevy_reflect"),
reflect_value(Debug, PartialEq, Serialize, Deserialize)
)]
Copy link
Member

Choose a reason for hiding this comment

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

Same here. I think we should split these.

Also, we can probably register the ReflectFromReflect and ReflectResource type data.

@djeedai
Copy link
Contributor

djeedai commented Apr 13, 2023

Is there a reason why this shouldn't be just a third party crate?

The previously mentioned RFC goes into reasoning: https://github.com/LegNeato/bevy-rfcs/blob/entropy/rfcs/55-deterministic_rng.md#motivation

I don't see where, especially given the RFC seem to justify itself (given its name) on the basis we need a deterministic RNG, then goes:

The default source of world entropy [Entropy::default()] is non-deterministic and seeded from the operating system.

So, what is the rationale for this not being a third-party crate? Everything I saw in the RFC seem to be easily implemented in a 3P crate. The only reason I'd see would be if several crates want to interop with each other via RNG, and so a built-in crate would enforce a common interface, but I'm not sure why they would want to do this (can't think of any example).

@Bluefinger
Copy link
Contributor Author

I don't see where, especially given the RFC seem to justify itself (given its name) on the basis we need a deterministic RNG, then goes:

The rationale being that pretty much the RNG side of the rust ecosystem has converged on rand and its ecosystem, making it the de-facto standard. Referencing the contribution guide, there's also the bit on "Don't reinvent the wheel", which when looking at this implementation, it is mostly an interface to plug in PRNG crates into usage as either a Resource and/or Component. There won't really be a need for many third party crates to write what is effectively the same thing, a way to work in a RNG that works well with the ECS, and can be done so in a way that enables both determinism and parallelism.

It can live as a third-party crate, sure, but bevy also makes use of RNG in various examples, with pitfalls regarding actual randomness if used in unsupported platforms (using DefaultHasher on WASM, for example, which I pointed out the problem here). So, perhaps we can do better on a first-party basis with regards to RNG patterns, deterministic or not. So might as well provide a first-party solution that helps us get there. Determinism is a property that can be enabled, but not always needed, and this solution provides both options with a common interface.

The default source of world entropy [Entropy::default()] is non-deterministic and seeded from the operating system.

So, what is the rationale for this not being a third-party crate? Everything I saw in the RFC seem to be easily implemented in a 3P crate. The only reason I'd see would be if several crates want to interop with each other via RNG, and so a built-in crate would enforce a common interface, but I'm not sure why they would want to do this (can't think of any example).

The common interface already exists with RngCore, SeedableRng traits provided by rand_core. It is the de-facto standard, so why not provide the interface to glue standard traits into Bevy?

@LegNeato
Copy link
Contributor

Is there a reason why this shouldn't be just a third party crate?

The previously mentioned RFC goes into reasoning: https://github.com/LegNeato/bevy-rfcs/blob/entropy/rfcs/55-deterministic_rng.md#motivation

I don't see where, especially given the RFC seem to justify itself (given its name) on the basis we need a deterministic RNG, then goes:

The default source of world entropy [Entropy::default()] is non-deterministic and seeded from the operating system.

So, what is the rationale for this not being a third-party crate? Everything I saw in the RFC seem to be easily implemented in a 3P crate. The only reason I'd see would be if several crates want to interop with each other via RNG, and so a built-in crate would enforce a common interface, but I'm not sure why they would want to do this (can't think of any example).

Your questions are answered in what you link to and it acknowledges it could be a third-party crate with different tradeoffs.

TL;DR: it needs to be in bevy to coordinate between ecosystem crates, enable advanced game features like lockstep, and make unit testing games better in general. Randomness is clearly used a ton in real world use cases...most of bevys examples use it, which points to being in bevy proper. Every other engine has randomness built in as a primitive, so there is prior art that points to including it.

@Bluefinger
Copy link
Contributor Author

TBF, one of the biggest cases for Deterministic RNG is being able to test systems that deal with random output. Being able to enforce the correct patterns for this is a win for the bevy ecosystem. It might not make sense for quick and dirty examples, but for anything approaching a serious project, being able to test your random parts is a huge boon.

@djeedai
Copy link
Contributor

djeedai commented Apr 13, 2023

The rationale being that pretty much the RNG side of the rust ecosystem has converged on rand and its ecosystem, making it the de-facto standard.

This is a rationale for converging on rand, which I don't think anyone questioned. This doesn't address the 1P/3P question.

it is mostly an interface to plug in PRNG crates into usage as either a Resource and/or Component.

Same, probably useful, but also probably doable as a third-party crate.

It can live as a third-party crate, sure, but bevy also makes use of RNG in various examples, with pitfalls regarding actual randomness if used in unsupported platforms

But does Bevy internally uses some PRNG? If not, examples can consume a 3P crate, and there's no argument to make this a first-party one.

So, perhaps we can do better on a first-party basis with regards to RNG patterns, deterministic or not. So might as well provide a first-party solution that helps us get there.

And perhaps also we can do better as a third-party crate? I fail to see any correlation between the problems raised in the RFC and here, and the requirements to be a first-party crate.

The common interface already exists with RngCore, SeedableRng traits provided by rand_core. It is the de-facto standard, so why not provide the interface to glue standard traits into Bevy?

Completely agreed, that sounds very reasonable and useful. And Bevy's policy is to provide such features as third-party crates to iterate over, until proven useful enough and stable enough to merge into core, unless there's a clear argument making it impossible to do so in the first place.

The log crate provides a logging facade for a lot of implementations. And yet it's a third-party crate, unrelated to those implementations, and not a part of the Rust standard library. Same for serde for serialization. Several crates have adopted bevy-inspector-egui for editing. There's a few others "de facto" third-party crates for other Bevy features. So there's precedent for things like this, and unless there's an argument I'm missing where it would be impossible to write that interface as a third-party crate, I'm not sure what justifies making an exception to the Bevy policy.

I think it would be easier to convince reviewer if the following questions where answered:

  1. Is Bevy internally using some PRNG, contributing to non determinism?
  2. What obstacle prevents using a third-party crate?
  3. Which Bevy crates are currently using a PRNG and need this standardizing feature, and why?
  4. Have their authors been contacted to discuss this? Do they have any objection to consuming a third-party dependency?

My personal understanding so far is that:

  1. No
  2. None
  3. Not that many. Most uses are in the final app/game, so is a per-app issue that a third-party crate can solve.
  4. Speaking for myself for Hanabi, which is a library that happens to use some PRNG, there's already a publicly available Resource that can probably help users achieve determinism (or, I'm open to fix anything needed here). And to make things even smoother, I'd be perfectly fine depending on a third-party interface crate to help some ecosystem interop/standardisation so it's even easier to do so.

The determinism raised in the RFC is a very valid concern. I just don't think the RFC answers the 1P/3P question that we ask all crates to answer.

@Bluefinger
Copy link
Contributor Author

In the mean time then, I've published https://github.com/Bluefinger/bevy_rand which is basically this crate, but with some updates. So now it exists as a third-party solution to battle-test what this crate is aiming to provide. I'm leaving bevy_entropy as the first-party name, and this PR to serve as the placeholder in case this is revisited at a later date.

@LegNeato
Copy link
Contributor

LegNeato commented Apr 15, 2023

I'm not doing a great job explaining first party vs third party. It feels a bit like trying to prove a negative but I'll give it a other shot:

What is better for the bevy ecosystem...having every single third party crate using randomness standardize on rand and dependency injection patterns (via components or resources) with potentially differing API, feature names, etc, or putting it in bevy? Randomness/entropy for games is as core as time IMHO. Punting implementation to a third party crate with adhoc coordination fragments the ecosystem--all it takes is one crate in your dependency chain that doesn't know about the 3rd party crate and your test and game determinism is blown out. You can of course go to that crate and fix it, but why play whack-a-mole across the whole ecosystem by default?

There is a reason why every other engine has randomness built in...almost every usage of the engine will need randomness, and almost every third party lib will want to use it in the same way. I'd love to learn about a full-featured engine that doesn't have randomness in core...Unity, Unreal, Godot, Source, etc all have it.

When building distributed systems (which is what bevy is structured as, it just happens to run on one machine), there are certain things you make sure to control to enable scenario testing as well as advanced runtime patterns--time, I/O (mem, disk, net, etc), and entropy are the big ones. Bevy has time (bevy_time) and I/O already (ecs takes care of mem, loaders take care of disk, punts on networking for now) but lacks an abstraction for randomness/entropy.

@IceSentry
Copy link
Contributor

almost every usage of the engine will need randomness

I think that's the part that's not as obvious as you are saying. There's plenty of projects that don't need randomness. Almost all of bevy's examples don't need it and neither does bevy itself except in, I believe, one limited situation.

@LegNeato
Copy link
Contributor

LegNeato commented Apr 15, 2023

I'm not sure you can say the examples do not need it when they almost all currently have it 😂. Sure, one can not use randomness in toy examples or narrow use-cases...but I don't think that is representative. When I get time, I'll look at the bevy jam entries...I bet well over 50% use randomness!

@IceSentry
Copy link
Contributor

There's 165 files with .rs extension in the examples folder and only 15 of those use rand and half of those are for stress tests. In most cases rand is only used as a workaround to spawn a lot of stuff which would generally not be done like that in a lot of games.

I'm not saying randomness is uncommon, just that it's not as common as you are saying.

@LegNeato
Copy link
Contributor

Fair, the number of examples changed since the original RFC was written.

@Bluefinger
Copy link
Contributor Author

When building distributed systems (which is what bevy is structured as, it just happens to run on one machine), there are certain things you make sure to control to enable scenario testing as well as advanced runtime patterns--time, I/O (mem, disk, net, etc), and entropy are the big ones. Bevy has time (bevy_time) and I/O already (ecs takes care of mem, loaders take care of disk, punts on networking for now) but lacks an abstraction for randomness/entropy.

I think the problem currently is that patterns/abstractions for coordinating the ecosystem are not yet established in the context of Bevy. Thus it is hard to argue for first-party basis on that. While this is explored, I think folks are okay with things being fragmented until the dust settles on a good way forward, and exploring that will be easier as a third party crate (which is what is being argued here and is the accepted modus operandi of the bevy crate ecosystem).

So for anyone reading this, please do go over to bevy_rand to help with establishing said patterns and providing use-cases, ideas, abstractions, etc, if you are interested in contributing in this area.

@alice-i-cecile alice-i-cecile removed this from the 0.11 milestone Jun 19, 2023
@LegNeato
Copy link
Contributor

LegNeato commented Oct 12, 2023

There is a chicken and egg here. I'm not sure why entropy / randomness has a higher bar than previous plugins with desirable functionality that were included in core and then iterated on (for example, UI). The abstractions can't be flexed if everyone just uses rand (following the examples!). On the flipside, physics are critical and are not in core.

While 18 /209 of examples use rand, 2/4 of the game examples do. I don't think the examples are representative of use, but as mentioned in the RFC every other major engine includes this functionality!

Shall we port the examples to the third-party crate in hopes that more people pick it up and try the API? If so, how is that better/worse than just including it in bevy as unstable? If not, how do we intend to get people to give feedback on the API?

What are next steps @alice-i-cecile ? Is Bevy even using the RFC process anymore?

@Bluefinger
Copy link
Contributor Author

There is a chicken and egg here. I'm not sure why entropy / randomness has a higher bar than previous plugins with desirable functionality that were included in core and then iterated on (for example, UI). The abstractions can't be flexed if everyone just uses rand (following the examples!). On the flipside, physics are critical and are not in core.

While 18 /209 of examples use rand, 2/4 of the game examples do. I don't think the examples are representative of use, but as mentioned in the RFC every other major engine includes this functionality!

Shall we port the examples to the third-party crate in hopes that more people pick it up and try the API? If so, how is that better/worse than just including it in bevy as unstable? If not, how do we intend to get people to give feedback on the API?

What are next steps @alice-i-cecile ? Is Bevy even using the RFC process anymore?

@LegNeato I think the argument about other engines applies less to Bevy because Bevy is designed to be extremely modular and flexible, with the ability to plug in all sorts of functionality, first-party and third-party and have it just work. And what Bevy exposes through its APIs allows for third-party crates to do a lot in the first place. So there's less need to have things first-party unless absolutely necessary.

So, I think for the moment, it should remain a community based effort until the need arises to make it first party. There's already a case that because of the need for something like bevy_prng, as the reflection story makes it awkward to use foreign types with Components and Resources. Being able to support PRNGs without newtyping them would be nice, but it is not a blocker to third-party.

Again, I'm more than happy to have folks help me maintain and expand bevy_rand, so to help build its visibility and explore the API, etc.

@alice-i-cecile
Copy link
Member

Sounds good <3 The RFC process is still in use, but is likely due for an overhaul. I would like to get you a proper final decision on this, but I can say the same for about 100 other open work items, and can't + shouldn't act unilaterally here. Given that a third-party crate is relatively viable here, I'm in favor of developing and promoting that for now.

@djeedai
Copy link
Contributor

djeedai commented Oct 12, 2023

Seconding @alice-i-cecile, reading the RFC I don't really see a clear advantage to a built-in solution over a 3P crate. Maybe we should move the discussion there to clarify the value first before proposing an actual PR?

@NthTensor
Copy link
Contributor

I recommend we close this. The third party crate has been successful, and the changes are stale.

@Bluefinger Bluefinger closed this Aug 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Math Fundamental domain-agnostic mathematical operations C-Feature A new feature, making something new possible 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.

8 participants