search

Bluefinger / bevy_turborand

A plugin to enable random number generation for the Bevy game engine.
Apache License 2.0
35 stars 5 forks source link

Deref implementation #1

Closed ManevilleF closed 2 years ago

ManevilleF commented 2 years ago

Adding Deref and DerefMut derive macro attributes on both the resource and component allow direct usage instead of calling get_mut every time.

I also added some must_use attributes and extra methods

Bluefinger commented 2 years ago

Unfortunately with the derives, it allows non-mutable queries to access the RngComponent, so in effect, you could write Query<&RngComponent> and that would be valid, because the Deref would apply to all the &self methods. It would potentially mean that systems accessing RngComponent can try doing so at the same time and cause all sorts of havoc as the underlying Rng state is a Cell, which is not threadsafe. The get_mut() was part of enforcing the contract that only one system at a time could access a given set of RngComponents.

So I don't really want to allow for Query<&RngComponent> situations. What do you think?

ManevilleF commented 2 years ago

Good point, I changed the code to keep Deref and DerefMut (though DerefMut is no longer useful) and use AtomicState instead of CellState. Using atomics should resolve the issue

Bluefinger commented 2 years ago

Atomics is one answer, but it does come with an overhead and some trade-offs. On my machine, the gen_u64 method performance is around 0.85ns for CellState, but 2.1ns for AtomicState. So with this PR, we trade some performance for some convenience, and at the same time, makes enabling deterministic ordering of systems trickier to do (as the absence of mut Queries will make it harder for Bevy's ambiguous ordering tool to pick up on that). Not that it can't be done (I've done so with initial versions of this crate before I released it). So nicer for queries and usage inside systems, but not as nice for ensuring determinism and ordering systems.

That said, for components and for queries, I'm leaning towards keeping things behind mut. Thinking about it though, I don't mind if the global rng instance has less performance. It does mean seeding RngComponent will be slower on a single thread, but we could scale with more threads for spawning things (more systems in parallel when access GlobalRng). So we perhaps could move GlobalRng to use AtomicState. That way, it can be accessed with just Res<GlobalRng> and have the Deref trait on it too.

EDIT: Also, to note, cloning is pretty cheap. It is more or less the same 0.8ns to clone an Rng<Cellstate>, and 2.1ns Rng<AtomicState>

ManevilleF commented 2 years ago

I didn't know there was such a perf gap between atomics and cells. This MR is then not relevant, I'm closing it