Search Unity

  1. Megacity Metro Demo now available. Download now.
    Dismiss Notice

Bug GetSingleton does not respect it's query context's enableable filter

Discussion in 'NetCode for ECS' started by ts_headfirst, Nov 6, 2023.

  1. ts_headfirst

    ts_headfirst

    Joined:
    Aug 1, 2023
    Posts:
    87
    Calling GetSingleton on a query, which has an enableable component as part of the filter, disregards the filter, e.g.

    Code (CSharp):
    1. var query = SystemAPI.QueryBuilder().WithAll<Id, GhostOwnerIsLocal>().Build();
    2. var singleId = query.GetSingleton<Id>();
    will return the first Id found completely disregarding the enabledstate of the GhostOwnerIsLocal.

    In fact looking at the code for GetSingleton, it expects there to be only a single chunk, so it is for the Archetype rather than the query, that the component is a singleton.
    The documentation however states:
    A singleton component is a component of which only one instance exists that satisfies this query.
     
    Last edited: Nov 6, 2023
  2. NikiWalker

    NikiWalker

    Unity Technologies

    Joined:
    May 18, 2021
    Posts:
    313
    This is intended behaviour, yes. See SystemAPI.GetSingleton for detail:
    This component type must not implement IEnableableComponent.

    Just a heads up: This is an Entities question, not a Netcode for Entities question.
     
  3. ts_headfirst

    ts_headfirst

    Joined:
    Aug 1, 2023
    Posts:
    87
    True, however it doesn't state that the query cannot have IEnableableComponents as part of the query, as in my example. Or rather it can, but their enabled state just won't be respected.
    I think it would make sense to include that information in the docs.

    True, my apologies, the reason is that this was initially thought to be related to GhostOwnerIsLocal (for which I created another post) which clearly it is not.
     
  4. NikiWalker

    NikiWalker

    Unity Technologies

    Joined:
    May 18, 2021
    Posts:
    313
    Agreed. I forwarded this feedback internally.
     
  5. CMarastoni

    CMarastoni

    Unity Technologies

    Joined:
    Mar 18, 2020
    Posts:
    891
    SystemAPI.GetSingleton does not consider (by design) the enable state of the component. And that make sense, because it is the only provided API for accessing the singleton and does not allow you to specify query option.

    If you want that, you need to provide you own query that consider the enable state.
     
  6. ts_headfirst

    ts_headfirst

    Joined:
    Aug 1, 2023
    Posts:
    87
    Nope, that is what isn't working, as per my example.
    The enabled state is never considered for EntityQuery.GetSingleton either.
     
  7. CMarastoni

    CMarastoni

    Unity Technologies

    Joined:
    Mar 18, 2020
    Posts:
    891
    But then, for the second you would had got a tons of exceptions because they will say:
    'you can't use GetSingletonEntity, TryGetSingleton or HasSingleton, on query that has enable components`

    The only way to use that is to use your own wrapper around CalculateEntityCount (unfortunately)
     
  8. ts_headfirst

    ts_headfirst

    Joined:
    Aug 1, 2023
    Posts:
    87
    Well no that is the issue :).

    You are correct that if the requested type T is an IEnableableComponent, you will get an error.

    However if the query simply contains an IEnableableComponent as part of the query, no error is reported, it simply ignores the enabled state, as in my example.
    I query for a component called Id, which is a standard component, but then the query also requires GhostOwnerIsLocal, the enabled state of which is not considered.

    In entities 1.1 this would then produce an error if the query returns more than 1 match (which is the case here), but in 1.0 you just get the first match, i.e. the first entry in the (Id, GhostOwnerIsLocal) archetype chunk.
     
  9. CMarastoni

    CMarastoni

    Unity Technologies

    Joined:
    Mar 18, 2020
    Posts:
    891
    The enable state is only ignored if you use the GetSingleton<T>. But if you use any of the TryGetSingleton<T> or HasSingleton<T> or TryGetSingletonEntity<T>, where T is not an IEneableComponent, in all these cases the CalculateEntityCount is invoked.

    i.e
    Code (csharp):
    1.  
    2. public bool TryGetSingleton<T>(out T value)
    3.     where T : unmanaged, IComponentData
    4. {
    5.     var hasSingleton = HasSingleton<T>();
    6.     value = hasSingleton ? GetSingleton<T>() : default;
    7.     return hasSingleton;
    8. }
    9. public bool HasSingleton<T>()
    10.         {
    11. #if ENABLE_UNITY_COLLECTIONS_CHECKS || UNITY_DOTS_DEBUG
    12.             var typeIndex = TypeManager.GetTypeIndex<T>();
    13.             if (TypeManager.IsEnableable(typeIndex))
    14.             {
    15.                 var typeName = typeIndex.ToFixedString();
    16.                 throw new InvalidOperationException(
    17.                     $"Can't call HasSingleton<{typeName}>() with enableable component type {typeName}.");
    18.             }
    19. #endif
    20.             //THIS ONE INCLUDE THE ENABLECOMPONENT AND ANY OTHER FILTER IN THE QUERY (i.e SharedComponent)
    21.             int matchingEntityCount = CalculateEntityCount();
    22. #if ENABLE_UNITY_COLLECTIONS_CHECKS || UNITY_DOTS_DEBUG
    23.             if (Hint.Unlikely(matchingEntityCount > 1))
    24.             {
    25.                 var typeName = typeIndex.ToFixedString();
    26.                 throw new InvalidOperationException(
    27.                     $"HasSingleton<{typeName}>() found {matchingEntityCount} instances of {typeName}; there must only be either zero or one.");
    28.             }
    29. #endif
    30.             return matchingEntityCount == 1;
    31.         }
    32.  
    So unless there is bug, the expected behaviour is that the TryGetSingleton should return false in that case. At least this is what the code should do.
     
  10. ts_headfirst

    ts_headfirst

    Joined:
    Aug 1, 2023
    Posts:
    87
    But if HasSingleton does respect it and GetSingleton does not respect it, that is totally inconsistent and hence problematic.

    Using the original example query:
    Let's say we have two entities A and B with the Archetype (Id, GhostOwnerIsLocal) and appearing in that order in the chunk, with GhostOwnerIsLocal enabled for B but not for A.

    A call to HasSingleton would produce true in both versions 1.0 and 1.1, right? Only B's Id is a match.
    A call to GetSingleton on the other hand:
    1.0: returns A's Id
    1.1: throws an exception? or maybe this has been fixed in 1.1 so it doesn't actually throw but will instead correctly return B's Id?
     
  11. optimise

    optimise

    Joined:
    Jan 22, 2014
    Posts:
    2,126
    It has been designed this way for performance reason. Consider using other way to only query enableable component that is enabled like idiomatic foreach.
     
  12. ts_headfirst

    ts_headfirst

    Joined:
    Aug 1, 2023
    Posts:
    87
    Sure, the documentation simply needs to reflect this, but that feedback has already been forwarded to the relevant people, as per NikiWalker's post.

    The continuation of this thread is to do with inconsistencies :).
     
  13. CMarastoni

    CMarastoni

    Unity Technologies

    Joined:
    Mar 18, 2020
    Posts:
    891
    The feedback will be given. We were just discussing about how things work at the moment and also why.
    If you think about the semantic, GetSingleton<T> need to always returning something (apart triggering an exception if you try to get it on something it does not have the component at all (physically)).
    A system that requires a singleton to exist also has probably a RequireForUpdate(query) too or the user has to check that the query has a singleton via HasSingleton<T> anyway for consistency.

    Indeed the API is a little confusing and inconsistent in some sense, but definitively makes sense that the HasSingleton<T> return false and the TryGetSingleton<T> return false for that query.

    We have another similar example of that for SharedComponent that expect user will always check HasComponent<T> before invoking the GetSharedComponent<T> that will otherwise return garbage.
     
  14. ts_headfirst

    ts_headfirst

    Joined:
    Aug 1, 2023
    Posts:
    87
    @CMarastoni its seems we are talking a bit past each other :)

    I have created a small project to display the issue, because
    is exactly what does NOT happen.

    HasSingleton returns true, but GetSingleton returns the wrong result.

    To see, just open the project and run the default scene, it's just a single System showing the issue.
     

    Attached Files:

  15. CMarastoni

    CMarastoni

    Unity Technologies

    Joined:
    Mar 18, 2020
    Posts:
    891
    Then that looks like a bug in the API.

    However, looking at your sample, you are also doing something "in the limit": you created three entities and you are using as GetSingleton for retrieving the only enabled instance. That is really a little borderline, because the GetSingleton expect that the chunk contains only one entity in general
    Looking at the code, there are some "optimised" path that does not check for any filter or enableable component.
    And the other ones, does all the work almost correctly. But the GetSingletonChunk does not consider enableable component (only query filter, like shared component).
    Indeed, looks like there is a bug or at least the API need clarification about how it is expected to work.
     
  16. ts_headfirst

    ts_headfirst

    Joined:
    Aug 1, 2023
    Posts:
    87
    Agreed, of course it is a matter of context, which can be solved by clarifying the documentation.
    As it reads now however, it suggests that the context of the singleton is the query, but in reality the context is the world.
    This is also more in tune with how singletons are usually defined, but just not clear from the docs.
    For a query context, GetSingle() is probably more appropriate.
    It is definitely a bug :), HasSingleton should return false if GetSingleton returns an incorrect result or throws (1.1).
    Should I report it via the bug tool or has it already been done?
     
  17. CMarastoni

    CMarastoni

    Unity Technologies

    Joined:
    Mar 18, 2020
    Posts:
    891
    I think it is a good idea to report the behaviour as a case. I already notified internally about that and the discussion agreed there is a need for both uniforming and changing how the GetSingleton work in general.
    Please open a case for it so it is properly tracked.
     
    ts_headfirst likes this.
  18. ts_headfirst

    ts_headfirst

    Joined:
    Aug 1, 2023
    Posts:
    87
    Bug created: IN-60076