Add DeferredMut, and supporting methods on WorldQuery to apply deferred mutations

[ecoskey]

See discord convo: https://discord.com/channels/691052431525675048/749335865876021248/1373035857522725079

Added apply and queue methods to WorldQuery to mirror SystemParam, and piped the calls through Query as needed.

While manually passing &mut Commands around is probably sufficient for most cases, sometimes custom QueryData implementations would benefit from being able to directly wrap (and queue/apply) some form of deferred state. A hypothetical example would be an Entry<T> QueryData, but anything that needs to update external state while accessing component data would fit the bill.

I'm not sure if this is controversial or not. Maybe it'd be good to enforce a strong separation between directly accessing components through queries and applying deferred mutations with commands, but IMO this opens the door to some pretty nice ergonomics.

Possible Followups:

• separate fetch.rs into a bunch of smaller modules to make the cfg::parallel guard cleaner
• add something like StorageSwitch but for component mutability, and only do the clone/deferred mutations when T is actually immutable?
• add methods on QueryState to allow users to manually flush state. Currently using DeferredMut outside of a system is a footgun.

[ItsDoot]

The QueryData derive macro should be updated too.

[ItsDoot]

Hmm, why not have these functions on just QueryData? QueryFilter being able to queue commands feels like a footgun (unless you have a use case in mind?).

[ecoskey]

Hmm, why not have these functions on just QueryData? QueryFilter being able to queue commands feels like a footgun (unless you have a use case in mind?).

Mostly it just seemed nicer to put methods taking &mut State on WorldQuery where State is defined, but I don't care too much either way if it seems like a hazard.

[ecoskey]

derive macros should be fixed

[alice-i-cecile]

Hmm. I think we should make a call on the use of this for deferred mutations for the immutable components pattern now, and probably merge that as one PR. This is currently very simple, which means I'm comfortable adding the first use case in one PR.

[ecoskey]

sounds good, will try in the morning :)

[github-actions]

It looks like your PR has been selected for a highlight in the next release blog post, but you didn't provide a release note.

Please review the instructions for writing release notes, then expand or revise the content in the release notes directory to showcase your changes.

[ecoskey]

turns out this is super cursed actually, I ran into a bunch of roadblocks trying to get any kind of state from QueryData::Item back to WorldQuery::State. Will try again later probably but might need some actual design work lol

[ecoskey]

Well, it works ™️ now, but it feels extremely jank to clone an Arc<Parallel<>> to each query item lol. If we could pass extended borrows from State to Fetch to Item we could get rid of the Arc at least, but last I tried lots of things broke.

Is this dead in the water/how should this proceed?

[chescock]

Oh, wow, I was worried this would be complex, but it's very readable!

The IS_DENSE thing is unsound and should be fixed before merging, and you probably want to delegate the new methods for Option so that Option<DeferredMut<T>> doesn't compile but ignore all changes, but none of the rest of my comments should be blocking.

[alice-i-cecile]

Needs a release note still :)

[chescock]

Oh, I just thought of a missing piece: We need to call SystemMeta::set_has_deferred in Query::init_access if (and only if) the query has one of these. Otherwise apply_deferred systems won't automatically be inserted to apply the commands.

[urben1680]

As a bevy user I love this, I also wanted to have something like this in an Entry-like query data.

But I am not so happy with the Parallel approach, the possibly surprising limitations such as "you won't see the new value on the second iteration" or "will not work with manual QueryState".

It really shows queries are not built to support this and this implementation here only works roughly. But I leave it to others to decide if that is okay. Personally I think a proper way to do this comes with a larger refactoring.

What this PR is missing is tests which is why I did not yet approve it here.

[ecoskey]

But I am not so happy with the Parallel approach

In hindsight, me neither lol. Since DeferredMut is readonly I'd assumed we'd need syncing of some sort, since multiple items could exist from the same entity at once.

But I guess we could just make it not readonly?? Seems like storing a bunch of UnsafeCells in State would be safe since mutable queries guarantee disjoint access 🤔

The one downside there is we'd need to also add a read-only version, which should be Ref<T> but we can't do that since the States have to match :(

[chescock]

But I guess we could just make it not readonly?? Seems like storing a bunch of UnsafeCells in State would be safe since mutable queries guarantee disjoint access 🤔

Wouldn't that require a separate cell for each entity? At that point, you might as well store it in the ECS like #[derive(Component)] struct Staged<T>(Option<T>); and then have a system that copies them over.

But I think an approach like that requires always iterating every entity in order to apply the updates. The current approach keeps a dense list of updated entities, which is going to perform better when updates are rare. I don't see how to do better than Parallel<T> for appending to a list with only &State.

[alice-i-cecile]

I like this, and the implementation looks solid. Before we merge this though, it needs:

a) to update the example where we demonstrate the immutable components pattern
b) needs a robust set of tests, including one where we validate that automatic sync points are inferred correctly

Removing from the 0.17 milestone as this is not a release blocker and is not currently ready to merge.