#[BUG] WithNone<IEnableable> doesn't add dependency to system

1 messages · Page 1 of 1 (latest)

high crag
#

@obsidian flume me again sorry 😅
IN-47419
When using EntityQuery.WithNone<T> where T : IEnableableComponent it does not add a dependency to the system so if another system is writing to the enable component safety exceptions are thrown on schedule:
InvalidOperationException: The previously scheduled job TestSystem1:TestJob writes to the ComponentTypeHandle<TestComponent2> TestJob.JobData.__TypeHandle.__TestComponent2_RW_ComponentTypeHandle. You are trying to schedule a new job TestSystem2:TestJob, which reads from the same ComponentTypeHandle<TestComponent2> (via TestJob.safety).

#

[BUG] WithNone<IEnableable> doesn't add dependency to system

#

You can work around the issue by adding the component dependency manually

public partial struct TestSystem2 : ISystem
{
    private EntityQuery query;

    public void OnCreate(ref SystemState state)
    {
        // state.GetComponentTypeHandle<TestComponent2>(); // UNCOMMENT TO WORK AROUND BUG

        this.query = SystemAPI.QueryBuilder()
            .WithAll<TestComponent1>()
            .WithNone<TestComponent2>()
            .Build();
    }

    [BurstCompile]
    public void OnUpdate(ref SystemState state)
    {
        state.Dependency = new TestJob
            {
                TestComponent1 = SystemAPI.GetComponentTypeHandle<TestComponent1>(true),
            }
            .ScheduleParallel(this.query, state.Dependency);
    }```
#

screenshot of the lack of dependency added to the system

image.png
#

so when the query tries to filter the enableable, problems =S

obsidian flume
#

Thank you, we'll take a look shortly!

#

Really I think the issue here is "WithNone" existing at all, since sometimes it means "I don't want this component" and other times it means "I want this component, but it should be disabled". In the latter case, you definitely need a dependency on the component; in the former, you actively don't want one. And we have no way of knowing at query time what your intent is.
In your case, would replacing WithNone with WithAbsent or WithDisabled have been possible, to more clearly express your code's intended use for this component?

high crag
#

For my specific case I can definitely use WithDisabled as the component has to exist on the entity in this case.

I think WithNone is useful though because sometimes I imagine you want to handle both cases?
I assume you'd only add the dependency when TypeInfo.EnableableType is true or something? Least bad solution.

obsidian flume
#

That's reasonable for now, but quietly becomes less reasonable as enableable components become more common. Some day they may even be the default... hmm.

high crag
#

hmm does using both .WithAbsent<Component>().WithDisabled<Component>() work?
or does it complain about adding same component twice
because that'd be the same as WithNone<Component> right if you wanted to remove it

obsidian flume
#

It does not work, for exactly the reason you give: we don't allow a component in more than one list. Arguably we could; some combinations are nonsensical (WithAll<T>().WithAbsent<T>()), but they should at least be well-defined. I'll look into it.

twin quiver
#

Just dredging up this old post as I am having issues with the WithAny attribute seeming to not do this either?

How would I go about writing an extension to the attribute to add it to the dependency? Or is there a better way / work around I should be doing?

obsidian flume
#

Can you be more specific as to what incorrect behavior you're seeing? The intended Semantics of WithAny are "at least one of the listed components is present and enabled". All types in the query's "Any" list should be added to the system's read or read/write dependency lists.

#

...no wait, I think I see it now. We only add required types to the system's reader/writer lists, meaning types that must be present (from the WithAll, WithDisabled, or WithPresent lists). Types in the WithAny list have never been included in the required types, so I suspect this bug has always been present, and WithAny just doesn't get wide enough usage that anybody noticed. Good catch!

obsidian flume
#

I've added a ticket for this bug. I suspect it's not a quick-and-easy fix, as it will change the package behavior in a not-backwards-compatible way.

The best workaround I can think of for both this issue and the original WithNone<T> bug is to call GetComponentTypeHandle<T>() (or its buffer equivalent) at system creation time for each component type passed to WithNone or WithAny, as this operation also adds the type to the system's list of dependencies. You don't need to touch the resulting type handle again (or even store it), unless you need to pass it into a job later in the system. Note that GetComponentLookup<T>() also adds T as a read or read/write dependency on the system, so if you're already using this method you can skip the dummy type handle creation workaround.

high crag
# obsidian flume ...no wait, I think I see it now. We only add _required_ types to the system's r...

(Nimps is a former colleague who asked for some help on this so I've already looked into this)

I don't think this was a problem pre IEnableable because any structural changes could only happen on the main thread and to access the any data you would need to use Lookup or Handle so it always brought in a dependency into the system anyway.

It's only an issue with IEnableable because the data not just the existence needs to be accessed when doing the chunk iteration part as well, which the user hasn't necessarily brought in the dependency separately.

dapper vale
#

yeah I also ran into this(and using workaround thanks to tertle), its just not expected behaviour with enableable components. i think tertle mentioned his original report was closed?

high crag
#

oh yeah, not sure what was up with that

image.png
obsidian flume
#

I'm the one who closed that ticket. We have some new bug workflows internally that we're still getting the hang of; hopefully my resolution notes were propagated to the public ticket?
The intent of the "Won't Fix" resolution was "We can't fix this core problem without a substantial EntityQuery redesign w/breaking behavior changes, and the work for that is already tracked in a different (non-public) ticket. For this specific symptom, there's a workaround available that we can point users to (manually creating TypeHandle/Lookup objects, as described above)."

twin quiver
#

Awesome thanks for the help!

high crag
#

@obsidian flume out of interest, what problem would this fix I've patched locally cause?

image.png
#

because i noticed looking at code the old path wasn't broken (with ComponentType[] instead of EntityQueryBuilder)

# 
        public EntityQuery CreateEntityQuery(params ComponentType[] requiredComponents)
        {
            var access = GetCheckedEntityDataAccess();
            fixed(ComponentType* requiredComponentsPtr = requiredComponents)
            {
                var query = access->EntityQueryManager->CreateEntityQuery(access, requiredComponentsPtr, requiredComponents.Length);
                return query;
            }
        }```
# 
        public EntityQuery CreateEntityQuery(EntityDataAccess* access, ComponentType* inRequiredComponents, int inRequiredComponentsCount)
        {
            var buffer = stackalloc byte[1024];
            var scratchAllocator = new UnsafeScratchAllocator(buffer, 1024);
            var archetypeQueries = CreateArchetypeQueries(ref scratchAllocator, inRequiredComponents, inRequiredComponentsCount);
            var outRequiredComponents = (ComponentType*)scratchAllocator.Allocate<ComponentType>(inRequiredComponentsCount + 1);
            outRequiredComponents[0] = ComponentType.ReadWrite<Entity>();
            for (int i = 0; i != inRequiredComponentsCount; i++)
                SortingUtilities.InsertSorted(outRequiredComponents + 1, i, inRequiredComponents[i]);
            var outRequiredComponentsCount = inRequiredComponentsCount + 1;
            return CreateEntityQuery(access, archetypeQueries, 1, outRequiredComponents, outRequiredComponentsCount);
        }```
as it passed all components, include ComponentType.Exclude, to requiredComponents
obsidian flume
#

In theory you're right; so long as T isn't enableable, adding ComponentType.Exclude<T>() to the required types should have the same effect as specifying .WithAbsent<T>(). But from a quick browse through uses of requiredComponents in the package, I see several cases that look like they implicitly assume these components must be present. (the TODO on the EntityQueryData.RequiredComponents field itself seems to acknowledge this ambiguity and suggest that we may want to remove the concept entirely).

#

Broadly speaking, my concern is that Exclude is an AccessMode, and we already have a long history of not actually respecting/preserving a ComponentType's AccessMode, even just ReadOnly vs. ReadWrite. Joachim once theorized that we should just get rid of the per-component-type access modes entirely, as 90+% of them were probably specified incorrectly, and in most cases we wouldn't throw an error because we don't use them (because we get RO vs. R/W metadata from elsewhere, to be clear -- not because we treat these modes identically).

My gut says that Exclude is a vestigial feature from the early Entities prototypes that was created to solve a specific problem, but never handled consistently throughout the package. My guidance would still be that we should deprecate and remove the Exclude access mode, avoid (if not also deprecate & remove) WithNone, and prefer WithAbsent or WithDisabled as the best ways to express this intent.