[Bug?] Multi EntityQueryDesc and enable components

[fast bridge]

Failing test I expect to pass

        [Test]
        public void QueryTestEnablable()
        {
            var entity = this.Manager.CreateEntity(typeof(Test1), typeof(Test2));
            var entity2 = this.Manager.CreateEntity(typeof(Test1), typeof(Test2));

            this.Manager.SetComponentEnabled<Test2>(entity2, false);

            var desc1 = new EntityQueryDesc
            {
                All = new[] { ComponentType.ReadOnly<Test1>(), ComponentType.ReadOnly<Test2>(), },
            };

            var desc2 = new EntityQueryDesc
            {
                All = new[] { ComponentType.ReadOnly<Test1>() },
                Disabled = new[] { ComponentType.ReadOnly<Test2>(), },
            };

            var query1 = this.Manager.CreateEntityQuery(desc1);
            var query2 = this.Manager.CreateEntityQuery(desc2);
            var query3 = this.Manager.CreateEntityQuery(desc1, desc2);

            Assert.AreEqual(1, query1.CalculateEntityCount());
            Assert.AreEqual(1, query2.CalculateEntityCount());
            Assert.AreEqual(2, query3.CalculateEntityCount()); // fail 1
        }

If I replicate it with normal components, works as expected

        [Test]
        public void QueryTestComponents()
        {
            var entity = this.Manager.CreateEntity(typeof(Test3), typeof(Test4));
            var entity2 = this.Manager.CreateEntity(typeof(Test3));

            var desc1 = new EntityQueryDesc
            {
                All = new[] { ComponentType.ReadOnly<Test3>(), ComponentType.ReadOnly<Test4>(), },
            };

            var desc2 = new EntityQueryDesc
            {
                All = new[] { ComponentType.ReadOnly<Test3>(), },
                None = new[] { ComponentType.ReadOnly<Test4>(),  },
            };

            var query1 = this.Manager.CreateEntityQuery(desc1);
            var query2 = this.Manager.CreateEntityQuery(desc2);
            var query3 = this.Manager.CreateEntityQuery(desc1, desc2);

            Assert.AreEqual(1, query1.CalculateEntityCount());
            Assert.AreEqual(1, query2.CalculateEntityCount());
            Assert.AreEqual(2, query3.CalculateEntityCount()); // pass
        }```
```cs
    public struct Test1 : IComponentData
    {
    }

    public struct Test2 : IComponentData, IEnableableComponent
    {
    }

    public struct Test3 : IComponentData
    {
    }

    public struct Test4 : IComponentData
    {
    }```

[Bug] Multi EntityQueryDesc and enable components

[fast bridge]

There's also an internal assertion failing on
var query3 = this.Manager.CreateEntityQuery(desc1, desc2);
if I change Test3 and Test4 to IEnableableComponent

    public struct Test3 : IComponentData, IEnableableComponent
    {
    }

    public struct Test4 : IComponentData, IEnableableComponent
    {
    }```

The internal error

[fast bridge]

well that assert error is probably known since it's marked with
// TODO(DOTS-5638): this assumes that query only contains one ArchetypeQuery

[fast bridge]

hmmmmmmmmmm ok well

i can see why in code this doesn't work this way; basically all filtering needs to be apply to all queries

that's a shame

[Bug?] Multi EntityQueryDesc and enable components

[mellow lintel]

Yeah, this multi-EntityQueryDesc feature was clearly added for a handful of internal use cases in the very early days of DOTS, and was clearly never properly tested. It fails to work at all in surprisingly many cases even in the best of times (lots of code just blindly assumes there's only one query desc), and the addition of enableable components makes it unclear how it could ever work as originally envisioned. I can't promise anything, but I suspect we'll end up deprecating and removing this feature; I wouldn't suggest building anything important on top of it in the meantime.

[mellow lintel]

The workaround depends on exactly what you're trying to achieve. In many cases you can express the multi-desc query as a single-desc query; lots of our internal use cases can be trivially replaced with a .WithAny<T1,T2>() clause instead of two descs requiring T1 and T2. In a few cases, you just have to use multiple queries instead.

Where things get tricky is where the queries/query descs can overlap (especially if that overlap is conditional upon the state of enableable components). If you're not careful, chunks can get processed multiple times.

[fast bridge]

I basically have a case where I have 2 enable components but one of them I want the enable state to be ignored while I want the other one to care about it's enable state (therefore IgnoreComponentEnabledState isn't useful as it makes both not care)
to quote myself from general the other day

we have these excludes

/// - WithDisabled<T>() matches chunks that must have T, but only matches entities where T is disabled.
/// - WithNone<T>() matches both of the above cases: either the component isn't present at all, or it is present but disabled.```
Then we have this include
```/// WithAll<T> matches all entities that have every component in this set```
Which is not really true, should say 
``` matches all entities that the component is present and it is enabled.```
imo what seem to be missing which is just 
```/// WithExist<T> matches all entities that have every component in this set, even if it's disabled```
the opposite of Absent

so I thought a clevery workaround would just be to have 2 queries (which is actually a feature I don't use frequently, I have found it very useful 1-2 times over the past 5 years to solve a problem so I'm a bit sad it might go away)

            SystemAPI.QueryBuilder()
                .WithNone<EntityDestroy>()
                .WithAll<EffectActive>()
                .AddAdditionalQuery()
                .WithNone<EntityDestroy>()
                .WithDisabled<EffectActive>()
                .Build();```
but alas, it does not work (hence this thread)

[mellow lintel]

If the absence of a "with present, whether enabled or disabled" component list is an argument for keeping AddAdditionalQuery, then I'd definitely rather add the former than keep the latter.
The naming of these lists is getting confusing; if you don't already know the semantics, what's the obvious difference between WithAll and WithExists? But that's a question for the internal bikeshedding department.

[fast bridge]

I totally agree, it took quite a while for me to wrap my head around none, absent and disabled

i wonder if simplifying it down in the query builder to again only All, Any, None but with optional settings for enablable (and other things) would be easier to use, for example

SystemAPI.QueryBuilder()
    .WithNone<EntityDestroy>(disabled:true)
    .WithNone<Initialized>(absent:true)
    .WithAll<EffectActive>(includeDisabled:true)
    .WithAll<EffectActivePrevious, EffectSomethingElse>()
    .Build();```

don't know if that makes it better or worse

[surreal kindle]

I feel like things got pretty confusing with the enablable components. It would be easier to infer if they were treated like a matrix of options:
(With/Without)(<nothing>/Enabled/Disabled)(<nothing>/RW)
With<T>
WithEnabled<T>
WithDisabled<T>
Without<T>
WithoutEnabled<T>
WithoutDisabled<T>

This would also make it clearer when you're using a query that checks enabled as that's more expensive. And maybe the plain functions shouldn't allow enablable components so it's explicit.

[mellow lintel]

These are both interesting ideas; thanks! I'm now confident we'll address this somehow; stay tuned for details.

One question is what the interface should look like; the other is how to represent the different categories internally. It's already pretty messy on the inside having the existing five component categories to wrangle & iterate over; it seems like we'd need as many as eight, assuming we keep the current constraint that a component can't be in more than one category?

• HasAll
• HasAll (enabled only)
• HasAll (disabled only)
• HasAny
• HasAny (enabled only)
• HasAny (disabled only)
• DoesntHave
• DoesntHave or Has+Disabled

[fast bridge]

assuming we keep the current constraint that a component can't be in more than one category
I think with what your propose there it wouldn't actually solve my original problem

There is still no way to query a component while ignoring the actual enable state

Just to make sure it's not lost

[BurstCompile]
[WithNone(typeof(EntityDestroy))] // IEnableable
private partial struct Job : IJobEntity
{
    private void Execute(EnabledRefRO<EffectActive> effectActive)
    {
        if(effectActive.ValueRO)
        {
            // Do something
        }
        else
        {
            // Do something else
        };
    }
}```
this is basically the problem I ran into

I can't use IgnoreComponentEnabledState because then the EntityDestroy won't be respected

using double queries was just the only workaround i thought of, but didn't work either
it wasn't the ideal solution at all

[mellow lintel]

Right, we need to make sure all possible constraints are possible to express, and right now they're not. IgnoreComponentEnabledState is probably the best you can do, but you'd have to manually check whether EntityDestroy is enabled or disabled on every entity, and that's definitely less than ideal.

[random ingot]

+1 for adding in the missing options here. This is something I found myself wanting back when enableable components were first added (https://forum.unity.com/threads/pain-points-with-aspects-and-ienableable-components.1355639/). In addition to the ideas already suggested, another one from that forum thread was to add an enum:

public enum EnableableOptions
{
    OnlyEnabled,
    OnlyDisabled,
    EnabledOrDisabled,
}

And then use it like so:

WithEnableable<T1, T2>(EnableableOptions.OnlyDisabled)

This would only work on IEnableableComponents, so there would be less confusion on what it would do with normal non-enableable components.

Additionally, it would be nice to be able to control this in an aspect for EnabledRefRW fields as well. Specifically, some kind of attribute to control whether the current enabled state of the component matters, and if so if it needs to enabled or disabled. This avoids having to add IgnoreComponentEnabledState at all the callsites, which is very easy to forget. Also, IgnoreComponentEnabledState affects ALL enableable components so it is quite prone to bugs, especially when components are hidden away in aspects.

Here is a simple example of what I mean:

[Serializable]
public struct MoveTo : IComponentData, IEnableableComponent
{
    public float3 Destination;
}
 
public readonly partial struct MoveToAspect : IAspect
{
    // A new attribute similar to Optional, but just to control the enabled bit in the query.
    [WithEnableable(EnableableOptions.EnabledOrDisabled)]
    private readonly EnabledRefRW<MoveTo> _moveTo;
 
    public void GoTo(float3 destination)
    {
        MoveTo.Destination = destination;
        Enable();
    }
 
    private void Enable()
    {
        // A new property on EnabledRefRW for accessing the enabled bit.
        _moveTo.EnabledRW = true;
    }
 
    // The ValueRW property would return the data for the IEnableComponent, not the enabled bit.
    private ref MoveTo MoveTo => ref _moveTo.ValueRW;
}
 
foreach (MoveToAspect moveTo in SystemAPI.Query<MoveToAspect>())
{
    moveTo.GoTo(new float3(1, 2, 3));
}

[fierce flame]

Hopefully relevant to the discussion: I'd like to be able to declare the ability to ignore enabled state on the parameters of IJobEntity so I can be easily selective about it

[random ingot]

Like an attribute? That would be really nice. Reminds me of how you can use [WithChangeFilter] inline next to the parameter. I didn't know about this for a long time but I find it much nicer to read compared to adding a WithChangeFilter attribute on the job struct

I guess it would look something like this:

private partial struct Job : IJobEntity
{
    private void Execute(Entity entity, [WithEnableable(EnableableOptions.EnabledOrDisabled)] EnabledRefRW<MoveTo> moveTo) {
        moveTo.ValueRW = true;
    }
}

[fast bridge]

forums will skin you

they don't like things longer than 3 charactesr 😄

[random ingot]

well I actually got some good replies on that thread. Although I think I had to link to it in discord during one of those Q&A blitz before I got any unity eyeballs on it haha

Not sure if I like this idea or how feasible it is, but what if there were more types like EnabledRefRW? Something like:

• EnabledRefRW: enabled only
• DisabledRefRW: disabled only
• EnabledOrDisabledRefRW: either

[fast bridge]

HasAll
HasAll (enabled only)
i was thinking about this the other day, would these not always be the same @mellow lintel
its not the same as WithNone, WithDisabled where the component not existing is a separate state
for All the component has to exist and be enabled
Unless HasAll in this case is ignoring enable state?

[random ingot]

I assumed the default HasAll would ignore, but I guess that be a breaking change for how it works today

[mellow lintel]

Yeah, these are all breaking changes unfortunately. The only bandaid we could add in a minor release would be a WithPresent<T> that means "has T, whether it's enabled or disabled", and just accept that we're further muddying the waters. But at least we'd fill this functionality gap.