Skip to content

fix: the type error after vehicle lights implementation#119

Merged
jdsika merged 3 commits into
lichtblick-suite:mainfrom
buraktiryaki:fix/fix_the_type_issue
Aug 8, 2025
Merged

fix: the type error after vehicle lights implementation#119
jdsika merged 3 commits into
lichtblick-suite:mainfrom
buraktiryaki:fix/fix_the_type_issue

Conversation

@buraktiryaki

@buraktiryaki buraktiryaki commented Aug 7, 2025

Copy link
Copy Markdown
Contributor

The "vehicle_classification" field could sometimes return null even when it was present (if obj is pedestrian). This bug has been fixed in this PR.

Comment thread src/index.ts Outdated
];
}

function isMovingObject(obj: MovingObject | StationaryObject): obj is MovingObject {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't the name be rather "hasLightState" as that is what is checked? I am also wondering why you mention the StationaryObject here?
I am also questioning if the lightstate is actually sufficient as it is allowed to have e.g. only the field: license_plate_illumination_rear filled out without a break light state?
I am quoting Thomas:

This should be checked again. Pedestrians or animals usually don't have lights at all and in my opinion it should also be allowed to have incomplete OSI data (vehicles without light definitions) and still be able to render the rest of the object.

In which situations is this feature supposed to be used and when should the lights be rendered? I Would say the moment the state in that field is set and not undefined?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Null checks were separated out specifically. Now it is checking for brakelights and indicator lights only and if they are not defined or null we are not rendering any of lights.

For mentioning StationaryObject:
osiObject can be also StationaryObject
https://github.com/lichtblick-suite/asam-osi-converter/pull/119/files#diff-a2a171449d862fe29692ce031981047d7ab755ae7f84c707aef80701b3ea0c80R95

@buraktiryaki buraktiryaki requested a review from jdsika August 7, 2025 14:13
@jdsika

jdsika commented Aug 8, 2025

Copy link
Copy Markdown
Collaborator

TODO: Please create a follow-up PR for some unit tests

@jdsika jdsika merged commit a5c521d into lichtblick-suite:main Aug 8, 2025
1 check passed
@thomassedlmayer

thomassedlmayer commented Aug 8, 2025

Copy link
Copy Markdown
Collaborator

The changes of this PR only partially solve the issues I recently mentioned.

If I understand the code correctly, both hasBrakeLightState and hasIndicatorState only return false if

  • the passed object's light_state is undefined (meaning it has no light state at all), or
  • the passed object's vehicle_classification is already undefined which should be the case for
    • stationary objects,
    • pedestrians,
    • animals,
    • vehicles that don't have vehicle_classification.

So for those situations, the lights are not rendered at all, which is correct.

Though, if the object is a MovingObject which contains partial light states, it returns true because of protobuf semantics (independent of proto2 or proto3):

All basic typed fields (string, int, enum, etc.) of a protobuf message are deserialized to their default values if they are not explicitly set, e.g.:

  • indicator_state is explicitly set to INDICATOR_STATE_OFF.
  • All other enums in light_state are not set during serialization.
  • All unset enums are deserialized as GENERIC_LIGHT_STATE_UNKNOWN (0) which is not equal to undefined!

This means that in the above example obj.vehicle_classification?.light_state?.brake_light_state != undefined should actually evaluate to true, as it is NOT undefined. Hence, if any light state is set (could also be the head light or whatever), both indicator and brake lights are visualized (probably with their off state).

@buraktiryaki @jdsika

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.

3 participants