Tag entity exclusion #59
Conversation
Signed-off-by: Aleksander Kamiński <[email protected]>
There was a problem hiding this comment.
Good job 👍
When testing I got the following bug:
- I added cube with tag that is excluded in scene configuration.
- I ran the simulation and the cube was ignored by RGL 👍
- I stopped the simulation and removed tag from the cube (Tag component is still present)
- I ran the simulation again and the cube was ignored by RGL even though the tag was removed.
Please investigate my test case. Also, I left some comments to address.
| void RGLSystemComponent::OnTick(float deltaTime, AZ::ScriptTimePoint time) | ||
| { | ||
| for (auto entityId : m_managersToBeRemoved) | ||
| { | ||
| m_entityManagers.erase(entityId); | ||
| m_unprocessedEntities.insert(entityId); | ||
| } | ||
| m_managersToBeRemoved.clear(); | ||
| } |
There was a problem hiding this comment.
Why couldn't we erase entity manager in ReviseEntityPresence(AZ::EntityId entityId)?
There was a problem hiding this comment.
This is so that the caller can signal revision inside a bus notification and can then safely disconnect from said bus after the call has ended (on destruction).
There was a problem hiding this comment.
At the moment, the only caller is EntityTagListener which is not destroyed in ReviseEntityPresence() so your case doesn't occur. Is it solution for the future usages?
There was a problem hiding this comment.
Exactly. I believe that this could lead to bugs that would be hard to track down as in previous iterations of the solution the entity manager handlet the tag notification bus and the ReviseEntityPresence method was called from within the callbacks which lead to segfaults caused by destruction of the entity manager before the callback was finished. In my opinion a small amount of additional complexity in this case is worth it.
There was a problem hiding this comment.
Instead of creating the new class and having separate container for it in RGLSystemComponent I'd extend EntityManager with LmbrCentral::TagComponentNotificationsBus::Handler. I see no advantages in having this separation.
There was a problem hiding this comment.
This is to allow for a scenario where an entity is excluded from the raycasting due it being tagged with an excluded tag after which the tag is removed and the entity must be processed. With the bus handled by the entity manager we would not be able to achieve this.
There was a problem hiding this comment.
I wonder if dynamic handling of tag creation/destruction is necessary (if there are any real use cases). If not, this solution could be significantly simplified to checking ShouldEntityBeExcluded in OnEntityContextCreateEntity and discarding entities that should be excluded (skipping processing them and putting them to m_unprocessedEntities). Did you get the detailed requirements for this feature?
There was a problem hiding this comment.
The Tag Component allows for tag addition and removal at runtime. I believe we should cover these cases as some proved useful before (as seen in this pr).
@msz-rai |
Signed-off-by: Aleksander Kamiński <[email protected]>
Signed-off-by: Aleksander Kamiński <[email protected]>
alek-kam-robotec-ai
deleted the
feature/better-entity-exclusion/alkam
branch
This pr adds a new way to exclude entities from raycasting using the Tag component.
To exclude an entity from raycasting tag it using the tag component and include the tag name in the scene configuration componet.