Skip to content

feat: add additional metadata for moving objects 2#123

Merged
rmessaou merged 1 commit into
lichtblick-suite:mainfrom
buraktiryaki:feat/add-additional-metadata-for-moving-objects
Aug 25, 2025
Merged

feat: add additional metadata for moving objects 2#123
rmessaou merged 1 commit into
lichtblick-suite:mainfrom
buraktiryaki:feat/add-additional-metadata-for-moving-objects

Conversation

@buraktiryaki

@buraktiryaki buraktiryaki commented Aug 25, 2025

Copy link
Copy Markdown
Contributor
  • Added additional metadata (velocity, acceleration, lane-assignement) for moving object.
  • Added a unit test
  • Fixed previous runtime error (undefined length)

The following shows a metadata panel for a pedestrian and a vehicle:
image
image

@buraktiryaki buraktiryaki force-pushed the feat/add-additional-metadata-for-moving-objects branch from 65223d7 to c87ae3a Compare August 25, 2025 02:16
@rmessaou rmessaou merged commit a030c38 into lichtblick-suite:main Aug 25, 2025
1 check passed
@thomassedlmayer

Copy link
Copy Markdown
Collaborator

@rmessaou @jdsika Should we somehow make sure that as many OSI fields as possible keep being optional for the viewer? With the changes of this PR, if acceleration/velocity values are missing, the metadata generation throws an error and you get an incomplete visualization as the rendering process is cancelled immediately for the rest of the frame. This should probably be fixed by only showing the metadata if it's actually given in the data.

Generally, it might be useful to allow the most minimal set of OSI which is necessary for the elementary visualization of objects. I think, we should have this in mind as an additional requirement when developing new features.
Being able to visualize incomplete OSI traces might help a lot for use cases like debugging of trace generation etc. where you might not set every single field of OSI in your trace but still want to see something in the visualization.

Also, MovingObject::assigned_lane_id is deprecated since the introduction of MovingObject::MovingObjectClassification::assigned_lane_id about 4 years ago. Is it intentional that the deprecated field is used here?

@buraktiryaki

Copy link
Copy Markdown
Contributor Author

@rmessaou @jdsika Should we somehow make sure that as many OSI fields as possible keep being optional for the viewer? With the changes of this PR, if acceleration/velocity values are missing, the metadata generation throws an error and you get an incomplete visualization as the rendering process is cancelled immediately for the rest of the frame. This should probably be fixed by only showing the metadata if it's actually given in the data.

Generally, it might be useful to allow the most minimal set of OSI which is necessary for the elementary visualization of objects. I think, we should have this in mind as an additional requirement when developing new features. Being able to visualize incomplete OSI traces might help a lot for use cases like debugging of trace generation etc. where you might not set every single field of OSI in your trace but still want to see something in the visualization.

Also, MovingObject::assigned_lane_id is deprecated since the introduction of MovingObject::MovingObjectClassification::assigned_lane_id about 4 years ago. Is it intentional that the deprecated field is used here?

PR for deprecated assigned_lane_id: #128

@jdsika jdsika added this to the v0.0.7 milestone Sep 4, 2025
@jdsika

jdsika commented Sep 4, 2025

Copy link
Copy Markdown
Collaborator

@thomassedlmayer I do agree with having proper exception handling for missing fields and only throw an error if it is not possible by the minimum requirements of Lichtblick to visualize an object derived from the schema definition.

But I am also asking myself if we should add mandatory fields in the BaseMoving with regards to the checker framework?

I made note of this in this issue #130

Sign up for free to 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.

4 participants