Skip to content

Updated EntityCategory msg type with a new unknown type - #3

Open
ayushgnv wants to merge 1 commit into
ros-simulation:mainfrom
ayushgnv:update_EntityCategory
Open

Updated EntityCategory msg type with a new unknown type#3
ayushgnv wants to merge 1 commit into
ros-simulation:mainfrom
ayushgnv:update_EntityCategory

Conversation

@ayushgnv

@ayushgnvayushgnv commented Mar 31, 2025

Copy link
Copy Markdown
Member

It would helpful to have a default category (value of 0) to account for entities that dont exist so we can differentiate between a Generic object that exists vs one that is not found.

@ayushgnv

Copy link
Copy Markdown
MemberAuthor

@adamdbrw any thoughts on this?

@adamdbrw

Copy link
Copy Markdown
Contributor

My idea with the OBJECT category was that it encompasses these cases (as in, everything is an object).
What would be a way in which we get entity returned by our APIs but this entity actually does not exist? Could you give me some context to that?

@ayushgnv

Copy link
Copy Markdown
MemberAuthor

My idea with the OBJECT category was that it encompasses these cases (as in, everything is an object).

What would be a way in which we get entity returned by our APIs but this entity actually does not exist? Could you give me some context to that?

In GetEntityInfo, we return a result and EntityInfo. EntityInfo contains EntityCategory which can only be set as OBJECT and others. I think when a entity is not found rather than returning the default OBJECT with a error result, it would be cleaner for the user if EntityCategory had a field for UNKNOWN (or something similar) which is then returned with an error result.

@ayushgnv

Copy link
Copy Markdown
MemberAuthor

@peci1@azeey any thoughts regarding this?

@peci1

peci1 commented May 1, 2025

Copy link
Copy Markdown
Contributor

I don't think this is needed. When the result has error, the other fields have unspecified value and should not be examined at all.

@azeey

azeey commented Aug 7, 2025

Copy link
Copy Markdown
Collaborator

I'd agree with @peci1. If the result is an error, the values in the other fields are undefined.

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

@ayushgnv@adamdbrw@peci1@azeey@michalpelka