OpenSimulationInterface / open-simulation-interface

A generic interface for the environmental perception of automated driving functions in virtual scenarios.
Other
267 stars 125 forks source link

Fix sensordata reference frame definitions #721

Closed thomassedlmayer closed 1 year ago

thomassedlmayer commented 1 year ago

Resolves #712

jdsika commented 1 year ago

I think ISO23150 demands for detected objects to be in vehicle frame. @PhRosenberger we need to crosscheck this

pmai commented 1 year ago

Since the OSI definition is a strict superset, I don't think this is an issue: If the virtual mounting position is 0,0,0, then you get the vehicle frame (as defined via bbcenter to rear axle); if you use a different virtual mounting position, you get something else... So if ISO 23150 is so restricted, then people who care will use virtual mounting position of 0,0,0 (which will be the most common vmp anyways, at least for cars - e.g. trucks are very different at least for some sensors).

PhRosenberger commented 1 year ago

I agree to @pmai as long as we can support the way ISO23150 demands it, we are fine. However, OSI is and should stay more flexible than that, as simulation can do more than the actual sensor.

jdsika commented 1 year ago

I just want to note here that at least how I personally understand this it is NOT a bug but the currently desired defintion which initially came from ISO. For that reason it was mentioned that \note The parent frame of \c base is the sensor's vehicle frame.

I understand that apparently I was the only person who was ok with having two different refernece frames for objects and detections (which were always in sensor frame).

If we merge this it is breaking backwards compatibility. Otherwise we have to make a poll if every OSI user did see it your way?

ClemensLinnhoff commented 1 year ago

In my opinion the current documentation in itself is not backwards (or forwards) compatible because it is confusing and some parts contradict others of the same documentation. Therefore this needs to be changed and unified asap. I don't think a poll of all OSI users is necessary, because it is still possible to use the ISO definition.

jdsika commented 1 year ago

Sorry, the poll comment was with "sarcasm on".

Can you state where the documentation contradicts regarding this reference frame for detected objects?

thomassedlmayer commented 1 year ago

@jdsika: See #712

Also Clemens and I noticed that the terms (host) vehicle coordinate or (host) vehicle coordinate system are used all around the OSI docs while never being defined properly anywhere. As Clemens said it is quite hard to understand the used coordinate system from reading the documentation.

I'm currently working on clarifying things (and also fixing some minor errors) in this chapter of the OSI documentation. For example, the image of the example at the end of chapter 2.2.4 is showing a different (probably wrong?) parent frame of the physical mounting position.

jdsika commented 1 year ago

I just checked it internally and at BMW we are doing it "correctly" in accordance with the main branch documentation:

thomassedlmayer commented 1 year ago

I just checked it internally and at BMW we are doing it "correctly" in accordance with the main branch documentation:

  • object lists are in vehicle frame "ego-vehicle-rear-axis"
  • detection lists are in sensor coordinate frame

I think, if you're not setting a virtual mounting position/orientation (= SensorView::mounting_position) this should be ok. Though, IMO this is still not strictly based on the "main branch documentation". The OSI documentation itself is quite clear on this (see https://opensimulationinterface.github.io/osi-documentation/#_sensor_data). It is just the class reference doc that was probably forgotten to be adapted at some time.

I fixed another contradiction for logical detection reference frames. Within the message LogicalDetection the attribute "position" was defined to be with respect to the host vehicle coordinate system while the higher-level description of the LogicalDetection message itself referred to the virtual sensor coordinate system for contained attributes. I assumed, the latter is the correct definition and adapted it accordingly.

thempen commented 1 year ago

As I am not to deep into the coodinate systems...

I understood, that there is the object coordinate system -> leading to the host vehicle coordinate system -> leading tho the (virtual) sensor coordinate system. For me it is sometimes hard to distinguish between the actual and virtual mounting position. This can be a topic for 4.0 to rename the parameters itself to have it in a more "speaking" way.

In total, the clarification makes sense for me.

// The virtual sensor coordinate system is relative to the vehicle coordinate // system which has its origin in the center of the rear axle of the ego // vehicle. This means if virtual sensor mounting position and orientation are // set to (0,0,0) the virtual sensor coordinate system coincides with the // vehicle coordinate system. This is something, i would make as a \note as it should not be necessary to understand the overall concept.

For the position and velocity, the changes will be breaking, yes. E.g. for trucks, the virtual sensor mounting position can be dynamic to the host vehicle coordinate system. Seen as a bug from the beginning, i would suggest on changing it within a minor release.

thomassedlmayer commented 1 year ago

I would agree on renaming the SensorData::mounting_position field for OSI 4.0 to emphasize that it is not the mounting position of a physical sensor (e.g. virtual_mounting_position). I will create a github issue for that.

I also agree on adding the /note tag. I will add this change soon and then add the ReadyForCCB label as discussed in today's meeting.

For the position and velocity, the changes will be breaking, yes.

It only fixes a contradiction/bug in the documentation. So I suppose it is not a breaking change but a bug fix.

pmai commented 1 year ago

CCB 2023-05-22: Merge as-is, @pmai to resolve conflicts.