[Bug] IJobEntity WithPresent and EnabledRefRW duplicates component in query

[simple spire]

When using [WithPresent], EnableRefRW and IJobEntity it adds the component twice to the query.

[BurstCompile]
[WithPresent(typeof(ConditionAllActive))]
private partial struct ConditionAllActiveJob : IJobEntity
{
    private static void Execute(EnabledRefRW<ConditionAllActive> conditionAllActive, in ConditionActive conditionActive)
    {
        conditionAllActive.ValueRW = conditionActive.Value.AllTrue;
    }
}

Result

DefaultQuery = 
    entityQueryBuilder
        .WithPresentRW<global::BovineLabs.Reaction.Data.Conditions.ConditionAllActive>()
        .WithAll<global::BovineLabs.Reaction.Data.Conditions.ConditionActive>()
        .WithAllRW<global::BovineLabs.Reaction.Data.Conditions.ConditionAllActive>()
        .Build(ref state);```
However if you do it [WithDisabled]
```cs
[BurstCompile]
[WithDisabled(typeof(ConditionAllActive))]
private partial struct ConditionAllActiveJob : IJobEntity
{
    private static void Execute(EnabledRefRW<ConditionAllActive> conditionAllActive, in ConditionActive conditionActive)
    {
        conditionAllActive.ValueRW = conditionActive.Value.AllTrue;
    }
}```
It correctly produces the right query
```cs
DefaultQuery = 
    entityQueryBuilder
        .WithDisabledRW<global::BovineLabs.Reaction.Data.Conditions.ConditionAllActive>()
        .WithAll<global::BovineLabs.Reaction.Data.Conditions.ConditionActive>()
        .Build(ref state);```

WithPresent just needs the same logic as WithDisabled.
If we can fix this then I think we can finally handle all enable states without the need for arbitrary EntityQueryOptions.IgnoreComponentEnabledState

@cedar sable

[Bug] IJobEntity WithPresent and EnabledRefRW duplicates component in query

changing this seems to fix it

DefaultQuery = 
    entityQueryBuilder
        .WithPresentRW<global::BovineLabs.Reaction.Data.Conditions.ConditionAllActive>()
        .WithAll<global::BovineLabs.Reaction.Data.Conditions.ConditionActive>()
        .Build(ref state);```
though i don't know if i'm breaking some other case

[simple spire]

OK this doesn't 100% fix it

There's still another case that's broken when you use [WithChangeFilter]

[BurstCompile]
[WithChangeFilter(typeof(ConditionAllActive))]
[WithPresent(typeof(ConditionAllActive))]
private partial struct ConditionAllActiveJob1 : IJobEntity
{
    private static void Execute(EnabledRefRW<ConditionAllActive> conditionAllActive, in ConditionActive conditionActive)
    {
        conditionAllActive.ValueRW = conditionActive.Value.AllTrue;
    }
}```

```cs
DefaultQuery = 
                entityQueryBuilder
                    .WithPresentRW<global::BovineLabs.Reaction.Data.Conditions.ConditionAllActive>()
                    .WithAll<global::BovineLabs.Reaction.Data.Conditions.ConditionActive>()
                    .WithAllRW<global::BovineLabs.Reaction.Data.Conditions.ConditionAllActive>()
                    .Build(ref state);```

I don't think [WithChangeFilter] should automatically add to the query, I think the user should be required to specify it separately in the query (which is the same requirement when doing querybuilder) - unfortunately i think this would be classified as a breaking change

this seems to fix that...

actually as per my above statement though, i think I prefer to fix this by not auto including change filter in the All query and instead requiring users to explicitly define it

but i'm doubtful unity will be able to make this change atm, but hey that's what my fork is for

[simple spire]

^ actually probably going to go with this so my fork remains compatible with official

[cedar sable]

Well we do support cases like:

[WithDisabled(typeof(MyComponent))]
partial struct DisableMyComponent : IJobEntity 
{
    void Execute(ref MyComponent health, EnabledRefRW<MyComponent> isAlive)
    {
      if (health.Value > 0)
        isAlive.ValueRW = true;
    }
}

So don't think we can remove the .Except(_archetype.Disabled) 😅

[simple spire]

It still works if you remove that

oh wait no i get you

the true would break it

yeah i went with the Except variation in the end anyway

seems to solve the problem

[simple spire]

hang on the hang on, this still works fine

        [WithDisabled(typeof(IntrinsicDirty))]
        partial struct DisableMyComponent : IJobEntity
        {
            void Execute(ref IntrinsicDirty health, EnabledRefRW<IntrinsicDirty> isAlive)
            {
                if (health.Value > 0)
                    isAlive.ValueRW = true;
            }
        }```

```cs
DefaultQuery = 
                entityQueryBuilder
                    .WithDisabledRW<global::BovineLabs.Stats.Data.IntrinsicDirty>()
                    .Build(ref state);```

Because your

```cs
                        case SimpleNameSyntax { Identifier.ValueText: "WithDisabled" }:
                            if (attribute.ArgumentList != null)
                                foreach (var argument in attribute.ArgumentList.Arguments)
                                    AddQueryInstanceFromAttribute(QueryDisabledTypes, argument, QueryType.Disabled, semanticModel, removeFromQueryAllIfFound: true);
                            break;```

removeFromQueryAllIfFound: true already covers this case as it has already removed it from the AllQuery group

the issue with this
_archetype.All.Concat(_changeFilterTypes)
is that _changeFilterTypes is adding types back to All that have been removed from removeFromQueryAllIfFound already

[cedar sable]

Oh yeah that's where I put it. Wonder if the reason it's also in SingleArchetypeQueryFieldDescription.cs is that someone wanted to solve it for SystemAPI.Query too

[simple spire]

side note, i appreciate how easy it was for me to read through these generators to figure this out

[cedar sable]

Thx! 😄

[simple spire]

but yeah removing
.Concat(_changeFilterTypes).Except(_archetype.Disabled).Except(_archetype.Present);
is breaking anyway as it breaks
SystemAPI.Query<MyComponent>().WithChangeFilter<MyOtherComponent>()

and while I think having to be explicit about the use of MyOtherComponent is probably better (because you have to be when you use changefilter on entityquery so it'd be consistent), i know you can't do breaking changes hence i settled on just removing the present from the list as well

[cedar sable]

BTW did a 1hour deep dive of the internals (and some of the design desciions behind IJE, last friday: https://www.youtube.com/watch?v=GqHjFvDBpBU in case you're curious)

[simple spire]

Sounds like I've found tomorrows while i work background audio

[simple spire]

tbh kind of disappointed this doesn't look fixed in 1.3.2

[cunning sonnet]

@simple spire Just to make sure I understand, the quick one-line fix you shared in #1251658206044754036 message is sufficient to fix the WithPresent query generation, right? If so, we can definitely get that into the next 1.3 patch release.

The WithChangeFilter issue is different enough that it will require some thought, even though the symptom is ultimately very similar.

[simple spire]

Yeah pretty certain, had it changed locally for a couple of months now

[simple spire]

#1064581837055348857 message

When you schedule a job without a query and it automatically infers from the components requested, is there a way to include access to an enablable component regardless if it is enabled or not? I tried adding a WithPresent attribute but comes up with a duplicate query error.
more people keep running into this problem...

[cunning sonnet]

We pushed the fix for WithPresent() this week to 1.3. Not sure which patch release it'll make it into, but one of them for sure.

Thank you for the reminder!