Bug in QueryBuilder?

[livid zealot]

Wrote about this in #1064581837055348857, but nobody had an answer so figured I'd make a larger post about it too.
The following system works fine:

using Unity.Collections;
using Unity.Entities;

public partial class TempSystem : SystemBase
{
    public struct SomeComponent : IComponentData
    {
    }

    public struct OtherComponent : IComponentData
    {
        public int SomeInt;
    }

    private EntityQuery _someQuery, _someAndOtherQuery, _someAndOtherQuery2;

    protected override void OnCreate()
    {
        var builder = new EntityQueryBuilder(Allocator.Temp);

        builder.WithAll<SomeComponent>();
        _someQuery = GetEntityQuery(builder);
        builder.WithAll<OtherComponent>();
        _someAndOtherQuery = GetEntityQuery(builder);
        builder.Reset();


        builder.WithAll<SomeComponent, OtherComponent>();
        _someAndOtherQuery2 = GetEntityQuery(builder);
        builder.Reset();
    }

    protected override void OnUpdate()
    {
        // var queryToUse = _someAndOtherQuery;
        var queryToUse = _someAndOtherQuery2;

        if (queryToUse.IsEmpty)
        {
            EntityManager.CreateEntity(typeof(SomeComponent), typeof(OtherComponent));
        }

        Dependency = new TestJob().Schedule(queryToUse, Dependency);
    }

    private partial struct TestJob : IJobEntity
    {
        private void Execute(OtherComponent other)
        {
        }
    }
}

But if I were to swap which query is being used, TestJob().Schedule will complain that the query doesn't cover all the components used in the Job. I think it's fine to enforce calling builder.Reset() after calling GetEntityQuery, but in that case it should error while making the query instead of creating a hard to debug error when using the query.

I've tested it with Entities 1.0.16 on Unity 2022.3.15f1 LTS, and Unity 2023.2.0b17

[sweet depot]

Yeah having a look at code

GetEntityQuery calls finalize on the builder

so it implies it shouldn't be used again but i definitely agree using it again without a reset should error

[median dawn]

Thanks for the report; I've filed a bug internally. I don't believe our intent is that query builders be used to incrementally build multiple queries, in which case we should make that more clear with errors at query-building time as suggested. But I'll confirm and report back if that's not the case.

[median dawn]

...reporting back to confirm that that is the case. So, the code above should throw an error from one or the other of these two lines:

builder.WithAll<OtherComponent>();
_someAndOtherQuery = GetEntityQuery(builder);

[livid zealot]

Oh, that's strange because there seems to be code explicitly for that purpose inside query builder. I also made a post about this later on TMS' discord where I ended up researching some more and found this: (copypasted from my post there)

When you call GetEntityQuery it calls FinalizeQueryInternal: https://github.com/needle-mirror/com.unity.entities/blob/ce54688e0429e00007af61026ea40f1c143e5912/Unity.Entities/SystemState.cs#L779
Which sets _isFinalized: https://github.com/needle-mirror/com.unity.entities/blob/ce54688e0429e00007af61026ea40f1c143e5912/Unity.Entities/Iterators/EntityQueryBuilder.cs#L1779
A little higher up it tries to just return itself if _isFinalized is true: https://github.com/needle-mirror/com.unity.entities/blob/ce54688e0429e00007af61026ea40f1c143e5912/Unity.Entities/Iterators/EntityQueryBuilder.cs#L1760
And functions like WithAll does try to unset it: https://github.com/needle-mirror/com.unity.entities/blob/ce54688e0429e00007af61026ea40f1c143e5912/Unity.Entities/Iterators/EntityQueryBuilder.cs#L255C19-L255C20
But somehow it still creates a corrupt query.
There does seem to be support for subqueries? https://github.com/needle-mirror/com.unity.entities/blob/ce54688e0429e00007af61026ea40f1c143e5912/Unity.Entities/Iterators/EntityQueryBuilder.cs#L1777, so I guess it's just bugged instead?

I should also mention that blob builder has the same issue, where it creates a blob asset with arbritrary (but strangely consistent) values if you reuse it. That should probably error too

[sweet depot]

blobbuilder doesn't even have a reset from memory

i would love a reset for it so i can reuse the memory and not have to re-allocate