World cell queries

[Frizi]

Rendered
Implementation PR

Add query and command API on WorldCell instances with shared access.

[bjorn3]

It will still alias. All items returned by an iterator are allowed to exist at the same time. For example when doing .collect().

[Frizi]

Collect call would panic if it ends up introducing an invalid alias. Thing about it as an iterator calling RefCell::borrow() in its next method.

[bjorn3]

Then the iterator has to return std::cell::Ref or std::cell::RefMut. You can't return the inner reference as once the Ref or RefMut is dropped, you are no longet allowed to access the inner value, as someone else can now borrow it.

[BoxyUwU]

I was confused by this too, RFC should explicitly mention that the iterators return a Ref/RefMut/some other lock guard

[Frizi]

It is mentioned, but in the implementation details. Maybe that's not enough.

Instead of usual component references, queries return all component data through wrappers types. Similar to already existing Mut wrapper for &mut X world fetches. The existance of those wrappers allows for both runtime checks and "world snapshots" with all commands applied - the wrapper can reference either world or a command queue data.

[Davier]

I see two goals in this RFC:

1. fine-grained runtime checked queries in WorldCell
2. make "overlayed" changes to a WorldCell

Goal 1 seems reasonable, and if I remember correctly it wasn't implemented in ECSv2 mainly due to a lack of use cases.
Quoting that PR: "The api is currently limited to resource access, but it can and should be extended to queries / entity component access."

Goal 2 looks like a complex solution to a problem that I'm not yet convinced exist.
All of the examples given can be tweaked to work with the current World API, usually by collecting the queries when they are iterated.

#[derive(Component, Clone, Copy)]
struct A;
#[derive(Component, Clone, Copy)]
struct B;

fn test() {
    let mut world = World::default();

    let mut query = world.query_filtered::<Entity, With<A>>();
    let entities = query.iter(&world).collect::<Vec<_>>();
    for entity in entities {
        world.entity_mut(entity).insert(B);
    }
}

fn test2() {
    // create world with two entities
    let mut world = World::default();
    let e1 = world.spawn().insert(A).insert(B).id();
    let e2 = world.spawn().insert(A).id();

    let mut query = world.query_filtered::<Entity, (With<A>, Without<B>)>();

    // verify query output before any modifications
    assert_eq!(query.iter(&world).collect::<Vec<_>>(), vec![e2]);

    // remove C from one entity, insert on another
    world.entity_mut(e1).remove::<B>();
    world.entity_mut(e2).insert(B);

    // new query results reflect the changes immediately
    assert_eq!(query.iter(&world).collect::<Vec<_>>(), vec![e1]);
}

fn test3() {
    let mut world = World::default();
    let mut query = world.query::<(Entity, &A)>();

    let result1 = query
        .iter(&world)
        .map(|(entity, a)| (entity, *a))
        .collect::<Vec<_>>();
    let inserted = world.spawn().insert(A).id();
    let result2 = query.iter(&world).collect::<Vec<_>>();

    // iterator created before insert doesn't include new entity
    assert!(!result1.iter().any(|(id, _)| *id == inserted));
    // iterator created after insert does include new entity
    assert!(result2.iter().any(|(id, _)| *id == inserted));
}

Could you provide examples that require the world overlays?
Meanwhile I would recommend splitting Goal 1 into its own PR (not sure an RFC is needed since the API should be the same as World::query)

[alice-i-cecile]

I can understand the justification for this, but I'm very nervous about encouraging users to do this, even for prototype code. It introduces a lot of potential for spooky-action-at-a-distance bugs, where the game will just panic at runtime in strange edge cases. Archetype invariants would help move those panics to the point of forbidden insertion, which would help, but getting those working in a performant fashion is a major endeavor.

If this feature was added to Bevy, I would write a lint forbidding its use in any of our company code bases due to the risk involved.

[alice-i-cecile]

I understand and emphasize with the pain here: we don't have great solutions for rapid prototyping and complex one-off logic. We absolutely need better tools for "fine-grained runtime checked" access to the World: a number of very common situations are needlessly painful.

However, I don't think this is the way. "Working" prototype code has a nasty habit of turning into production code, and this feature will be a large, hard-to-manage source of technical debt and bugs.

The API uses too big of a gun in the name of usability-when-first-writing: users are encouraged to just toss messy, arbitrary code in these exclusive systems, which will panic in unpredictable ways at runtime based on the world's state. The performance issues are fine: I agree with you that performance vs. ease of development should be a choice left to the end user. I just genuinely don't think this will make writing complex gameplay code easier when the full lifecycle is accounted.

[kabergstrom]

The goal of this RFC is to be able to write code with looser restrictions than the existing APIs, while sacrificing performance in a few ways. Parallelism will not be possible, and there will be more runtime overhead. Borrowing semantics will be upheld at runtime. Queries must follow visibility rules when adding/removing components/entities during iteration, which is not free.

The changes should enable writing familiar code that closely resembles what can be written in popular game engines where gameplay code is single-threaded-only. The key benefit here compared to other engines is that code can access the same data as the existing parallel APIs, meaning users can migrate code between the APIs depending on the evolving needs of their project.

On Discord, there has been a lot of discussion around whether this API enables "bad code", or whether the usecase that has lead to this work could be implemented in some way with existing APIs. I am not interested in further discussing either topics, since it derails from the goals of this API proposal: to be able to write gameplay code with fewer restrictions. Just let me write shit code.

I understand there to be two separate conversations:

1. Should bevy_ecs support an API that is minimally restrictive by trading off performance and runtime panics?
2. What is the best way to implement the goals I've described in bevy_ecs?

[kabergstrom]

Could you provide examples that require the world overlays?

Arbitrary callbacks which add/remove entities or components, even while borrows to disjoint components are held elsewhere.

[hymm]

I'm against the idea of instant commands. Not having to confront the complexities of deferred commands at all will lead to code that is impossible to pull apart and the prototype code will end up in production. Commands should be as similar as possible to what you can do in normal systems and then applied by an explicit call like command_queue.apply(world).

[alice-i-cecile]

Coming back to this, I have a hard time following the logical flow of this proposal and agree with Davier above. Why do these changes belong together?

I can get on board with "maximally granular" WorldCell at the expense of performance and runtime panics, provided we have very strong and clear docs for it. Escape hatches are important, and while we can gradually reduce the necessity of this tool, we're not there yet and will probably never be able to predict all possibly useful access patterns. Hopefully we can even provide a nice set of errors for it and make some of the errors recoverable!

We need to solve bevyengine/bevy#3096 for exclusive world access to feel usable for arbitrary game logic. But "instant commands" in the fashion proposed is too error-prone and the behavior is too different from the behavior of the commands used in ordinary systems.

[Davier]

Could you provide examples that require the world overlays?

Arbitrary callbacks which add/remove entities or components, even while borrows to disjoint components are held elsewhere.

My point is that you don't need to hold borrows elsewhere, that is what entity ids are for. I think this represents what you describe:

#[derive(Component)]
struct A;
#[derive(Component)]
struct B;

fn test() {
    // Create world
    let mut world = World::default();
    let e1 = world.spawn().insert(A).insert(B).id();
    let e2 = world.spawn().insert(A).id();
    
    // Borrow stuff
    let mut cell = world.cell();
    let e1_a = cell.entity(e1).get_mut::<A>().unwrap();
    let e2_a = cell.entity(e2).get_mut::<A>();
    let query = cell.query::<A>();
    
    // Access borrowed things
    for a in query.iter() {}
    
    // Callbacks
    f1(&mut cell, query, e1_a, e2_a);
    
    // Access borrowed things
    for a in query.iter() {}
}

fn f1(cell: &mut WorldCell, query: _, e1_a: _, e2_a: _) {
    // Access borrowed things
    *e1_a = *e2_a;
    for a in query.iter() {}
    // Use commands
    cell.spawn().insert(A);
}

And this is what I suggest:

fn test() {
    // Create world
    let mut world = World::default();
    let e1 = world.spawn().insert(A).insert(B).id();
    let e2 = world.spawn().insert(A).id();
    let query = world.query::<A>();
    
    // Borrow only what is needed, when needed
    for a in query.iter(&world) {}
    
    // Callbacks using `&mut World` and entity ids
    f1(&mut world, query, e1, e2);
    
    // Borrow again
    for a in query.iter(&world) {}
}

fn f1(world: &mut World, query: QueryState<A>, e1: Entity, e2: Entity) {
    // Borrow right before use, possibly using WorldCell
    let cell = world.cell();
    let e1_a = cell.entity(e1).get_mut::<A>().unwrap();
    let e2_a = cell.entity(e2).get::<A>().unwrap();
    // Access borrowed things
    *e1_a = *e2_a;
    for a in query.iter(&world) {}
    // Use commands
    world.spawn().insert(A);
}

Does that solve your use case? If you're worried about the verbosity of calling things like cell.entity(e1).get_mut::<A>().unwrap(); in multiple places, I think there is a simpler solution. Notice how the query is created once, passed around, and iterated using world. Maybe we could have a similar wrapper type for component:

// Nothing is borrowed here
let e1_a = world.get_wrapper::<A>(e1);
let e2_a = world.get_wrapper::<A>(e2);

// In a callback
*e1_a.get_mut(world) = *e2_a.get(world);

[alice-i-cecile]

As mentioned in #12549, @james7132 and I are looking to remove this feature entirely.

If anyone reading this needs advice / features to enable a third-party equivalent, please reach out and we'll do our best to accomodate.