bevy icon indicating copy to clipboard operation
bevy copied to clipboard

`Option` and `Has` ignore sparse components in transmuted dense queries

Open chescock opened this issue 1 year ago • 2 comments

Bevy version

e155fe1d86

What you did

Transmuted a QueryState<EntityMut> to either QueryState<Option<&S>> or QueryState<Has<S>>, where S is a sparse set component.

#[derive(Component)]
#[component(storage = "SparseSet")]
struct S;

let mut world = World::new();
world.spawn(S);

let mut query = world
    .query::<EntityMut>()
    .transmute::<(Option<&S>, Has<S>)>(&world);
let (option, has) = query.single(&world);

assert!(option.is_some());
assert!(has);

What went wrong

The matched entity has an S component, but Option<&S> yields None and Has<S> yields false.

Additional information

Option and Has cache whether the component exists in set_table or set_archetype. To make this work for sparse set components, they set IS_DENSE = false and require sparse iteration. But EntityMut performs dense iteration, so the transmuted query is dense and calls set_table. Option and Has don't find the component in the table, since sparse set components are not stored in tables, and return None or false for the entire table.

#16396 would make this also occur in cases involving FilteredEntityMut.

chescock avatar Nov 16 '24 01:11 chescock

I see two basic ways to fix this: either prevent dense to sparse transmutes, or make Option and Has always work on dense queries.

Preventing dense to sparse transmutes could be done by having QueryState::transmute_filtered do assert!(!self.is_dense || (NewD::IS_DENSE && NewF::IS_DENSE)) alongside the other assert. To support cases where those transmutes are needed, we could provide ways to force the original query to be sparse, like a QueryBuilder::force_sparse() method and a ForceSparse QueryFilter struct. That would also resolve #14628.

Making Has work on dense queries isn't too bad. You can store the &ComponentSparseSet in the Fetch when working with a sparse set component and call contains() on each entity. You could even store an Option<bool> alongside it to cache the result when the query winds up being sparse anyway. Making Option work is similar, but requires a lot more code since you have to add plumbing to QueryData.

Making Has and Option always be dense might even improve performance, if the better iteration order offsets the per-entity lookups!

I'm happy to make PRs for any of these ideas if those designs sound reasonable!

chescock avatar Nov 16 '24 01:11 chescock

Looking at this again, I have no idea why I thought #14628 had the same root cause! That one doesn't involve sparse set components at all. I'll remove that from the description.

I also missed a few other types with the same issue: AnyOf and Or. A dense query with AnyOf<(&S)> will always yield None, just like Option<&S>. And a dense query with Or<(Changed<S>)> or Or<(Added<S>)> will never find rows with a changed or added S.

Those could be made to work with dense queries using the same approach as Option.

chescock avatar Nov 24 '24 16:11 chescock