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

Add bevy_entropy plugin and include it in the default set #2504

Closed
wants to merge 12 commits into from

Conversation

LegNeato
Copy link
Contributor

@LegNeato LegNeato commented Jul 20, 2021

Objective

  • Bevy games / applications often need to use random number generators, but they make execution non-deterministic. Currently there is no official way to introduce determinism in a bevy-supported way.

Solution

  • Include a source of entropy to (optionally) enable deterministic random number generators. The plugin can be completely disabled if no source of entropy is required, the default entropy from the OS can be used if randomness is needed but deterministic execution is not, or a world seed can be specified for deterministic random number generation.

Note that I put up a PR previously to add default rngs. While most other engines (godot, unity, etc) provide built-in rngs, this is a lower-level (and more general?) building block we likely want to start with first.

Open items

  • Is the naming ok? Entropy or EntropySource? get()? Naming is ok
  • Should this be a plugin or in core? One can make the argument that entropy is as core as time and thus should always be there. Plugin is better and time might move out of core.
  • Tests
  • Docs (did I miss these in the repo?)
  • Changelog item

@github-actions github-actions bot added S-Needs-Triage This issue needs to be labelled S-Pre-Relicense This PR was made before Bevy added the Apache license. Cannot be merged or used for other work labels Jul 20, 2021
@DJMcNab
Copy link
Member

DJMcNab commented Jul 20, 2021

I don't think this is deterministic where there is system ordering ambiguity - afaik I we don't make a promise about execution order in that case

However, I hadn't realised that a mutable resource would be deterministic if ordering is well defined - although obviously that does have the cost of making all entropy access serial.

One potentially neat thing would be for the rng to be a custom system parameter, which gets a 'system level' rng seed, without blocking the global entropy. We would probably need a pre-run hook which could hold resources for only a short amount of time (alternatively, chained systems could support something similar and those could be used instead)

@alice-i-cecile alice-i-cecile added A-Core Common functionality for all bevy apps C-Feature A new feature, making something new possible and removed S-Needs-Triage This issue needs to be labelled S-Pre-Relicense This PR was made before Bevy added the Apache license. Cannot be merged or used for other work labels Jul 20, 2021
@alice-i-cecile
Copy link
Member

Preface: I think that standardizing on a solution here is extremely important. Without it, each crate in the ecosystem will use their own PRNG solution, bloating dependencies and making a unified approach to determinism pretty much impossible.

I don't think this is deterministic where there is system ordering ambiguity - afaik I we don't make a promise about execution order in that case

We can currently use the single-threaded executor when we want drop-in determinism. There's space in the future for more sophisticated parallel executors as well.

One potentially neat thing would be for the rng to be a custom system parameter, which gets a 'system level' rng seed, without blocking the global entropy

Agreed, this would be extremely useful, and worth baking into Bevy as a whole. RNG use will be quite prevalent in some games, and it would be nice to have a non-blocking solution.

We would probably need a pre-run hook which could hold resources for only a short amount of time (alternatively, chained systems could support something similar and those could be used instead)

I prefer the former solution; I would probably build it using a simple exclusive system that runs at the start of the schedule and initializes the Local data of each RNG dependent system. Could you expand a little on how you think system chaining could work @DJMcNab?

@alice-i-cecile alice-i-cecile added the S-Pre-Relicense This PR was made before Bevy added the Apache license. Cannot be merged or used for other work label Jul 20, 2021
@DJMcNab
Copy link
Member

DJMcNab commented Jul 20, 2021

The basic idea of how the system chaining would work is that the first system in the chain would only block the resources that it uses.
E.g. a system with a resmut parameter could be chained after a relatively long running system without blocking other system's access to that value, such as to update it at the end of the frame, say

Basically change chaining into dependencies with 'message passing'

Also our single threaded executor also doesn't guarantee ordering afaik, it's just a way to run the parallel system graph but only using one thread

@alice-i-cecile
Copy link
Member

Also our single threaded executor also doesn't guarantee ordering afaik, it's just a way to run the parallel system graph but only using one thread

Right... it runs it in topographic order, but that topographic order may vary freely within the constraints between runs.

@alice-i-cecile
Copy link
Member

Is the naming ok? Entropy or EntropySource? get()?

I much prefer Entropy. It's perfectly clear and we're going to need to type this a lot.

Should this be a plugin or in core? One can make the argument that entropy is as core as time and thus should always be there

Hmm. It would force a dependency on rand for every Bevy app, which is not great. I see your point though.

Docs (did I miss these in the repo?)

The doc strings and examples here are quite good so far. You could add module level documentation, but what probably makes more sense is to add a page on RNG as part of chapter 3 of the new book in the bevy-website repo. See https://github.com/bevyengine/rfcs/blob/main/rfcs/23-quick_start_book.md for an overview of the plan there.

@alice-i-cecile
Copy link
Member

@LegNeato we're currently undergoing relicensing to add Apache in addition to MIT. Could you please agree to this relicense over in #2373?

@LegNeato
Copy link
Contributor Author

LegNeato commented Jul 21, 2021

Commented. Will try to clean this up in the next week or so.

@mockersf mockersf removed the S-Pre-Relicense This PR was made before Bevy added the Apache license. Cannot be merged or used for other work label Jul 21, 2021
@cart cart added the S-Pre-Relicense This PR was made before Bevy added the Apache license. Cannot be merged or used for other work label Jul 23, 2021
@mockersf mockersf removed the S-Pre-Relicense This PR was made before Bevy added the Apache license. Cannot be merged or used for other work label Jul 24, 2021
@LegNeato
Copy link
Contributor Author

LegNeato commented Aug 9, 2021

Not sure why the ci job is failing with something related to jni, I didn't touch anything there. Any pointers would be appreciated.

I'll look into automatically making rngs or entropy per-system if this lands.

@LegNeato
Copy link
Contributor Author

LegNeato commented Aug 14, 2021

Ah, the same CI issue exists on main:

https://github.com/bevyengine/bevy/runs/3326099396

@alice-i-cecile alice-i-cecile added the S-Needs-Design-Doc This issue or PR is particularly complex, and needs an approved design doc before it can be merged label Sep 22, 2021
@LegNeato
Copy link
Contributor Author

This is now a part of an RFC: bevyengine/rfcs#55

@rdrpenguin04
Copy link
Contributor

What are the pros/cons of this versus bevy_rand, besides this being intended to be built-in? Are there any design ideas to merge in or anything like that?

@LegNeato
Copy link
Contributor Author

This predates bevy_rand and inspired a bit of it (and the verbage of much of the bevy_rand explanation is reused from the RFC associated with this).

@LegNeato
Copy link
Contributor Author

Closing in favor of #7871 and/or bevy_rand. I still think this needs to be built into bevy.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Core Common functionality for all bevy apps C-Feature A new feature, making something new possible S-Needs-Design-Doc This issue or PR is particularly complex, and needs an approved design doc before it can be merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants