Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix documentation on coordinate systems and transformations #723

Merged
merged 2 commits into from
Jun 15, 2023
Merged

Fix documentation on coordinate systems and transformations #723

merged 2 commits into from
Jun 15, 2023

Conversation

thomassedlmayer
Copy link
Contributor

This PR fixes some issues with the documentation on coordinate systems and transformations.

  • Added section on the definition of "host vehicle coordinate system" to the chapter on coordinate systems used for OSI. This (host) vehicle coordinate system is referenced in many places in OSI but never explained anywhere in the documentation.
  • Replaced Figure 10 by another image from the OSI class reference docs (MovingObject) because physical sensor coordinate system is not with respect to center of bounding box but with respect to center of rear axle.
  • Fixed minor bugs (typos, copy/paste errors)
  • MovingObject Detailed Description:
    • Fixed reference to MovingObject::vehicle_extension which was probably replaced by MovingObject::vehicle_attributes and MovingObject::vehicle_classification.
    • Aligned image scale

I've tested the Antora documentation build locally.

Feel free to add any thoughts.

Checklist:

@jdsika jdsika mentioned this pull request Apr 12, 2023
Open
@jdsika jdsika added the HelpWanted I have tried my best - please help me! label Apr 12, 2023
@thempen thempen added the Harmonisation The Group in the ASAM development project working on harmonisation with other standards. label Apr 26, 2023
thempen
thempen reviewed May 10, 2023
View reviewed changes
doc/usecases/transforming_coordinate_systems.adoc

``SensorData::mounting_position``::
This field defines the sensor's position and orientation and thereby the origin of the sensor coordinate system.
The mounting position is given in the vehicle coordinate system.
This field defines the sensor's virtual mounting position and orientation and thereby the origin of the virtual sensor coordinate system.
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not just say "mounting position" instead of "virtual mounting position"? Has the virtual some special meaning hiere?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried to emphasize the distinction between physical and virtual mounting positions so that they do not get confused. The SensorData::mounting_position is virtual/"imaginary", while SensorDetectionHeader::mounting_position is referring to the actual physical mp of a detector. The OSI class reference docs also describe the SensorData::mounting_position as "virtual mounting position".
I just tried to align/unify terms so far. But I think it would make sense to also add an explanation of the physical mounting position in this section.

Copy link
Contributor

Choose a reason for hiding this comment

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

Just as a side-note: For OSI 4.0 it would be a good idea to name the fields accordingly.

thempen
thempen reviewed May 10, 2023
View reviewed changes
doc/usecases/transforming_coordinate_systems.adoc
@@ -91,19 +91,19 @@ In Open Simulation Interface, an object's position is defined by the coordinates
This field defines the orientation of the vehicle's reference point in global coordinates.

``GroundTruth::moving_object::vehicle_attributes::bbcenter_to_rear``::
This field specifies the vector pointing from the vehicle's reference point to the middle of the rear axle under neutral load conditions in the vehicle coordinates.
This field specifies the vector pointing from the vehicle's reference point to the middle of the rear axle under neutral load conditions.
Copy link
Contributor

Choose a reason for hiding this comment

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

The "vehicle reference point" is the center of the bounding box, right? Therefore, i would suggest to call it object reference point to have it in a similar naming, than the object coordinate system.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, we could do that. Though, this section only describes the messages in the context of a vehicle so I tried to not change even more things except for the necessary parts.

We could also extend or partly generalize this section for other objects. We should just make sure to adapt all occurrences of "vehicle reference point". This term is used all over the OSI class reference documentation.

@thomassedlmayer thomassedlmayer mentioned this pull request May 10, 2023
Open
@thempen
Copy link
Contributor

thempen commented May 24, 2023

Harmonization group meeting result:

  • PR accepted to improve the existing documentation for the coordinate system naming.
  • The description and definition of coordinate systems should be reworked for 4.0. There are multiple coordinate systems not mentioned yet and additional problems, e.g. the dynamic definition of bounding box.

@thempen thempen added the ReadyForCCBReview Indicates that this MR is ready for a final review and merge by the CCB. label May 24, 2023
@thomassedlmayer thomassedlmayer marked this pull request as ready for review May 24, 2023 08:50
@pmai
Copy link
Contributor

pmai commented Jun 12, 2023

CCB 2023-06-12: Merge as-is.

@pmai pmai added ReadyToMerge This PR has been approved to merge and will be merged by a member of the CCB. and removed ReadyForCCBReview Indicates that this MR is ready for a final review and merge by the CCB. labels Jun 12, 2023
@thomassedlmayer @pmai
Fix MovingObject description and image size
f7b655b 
Signed-off-by: Thomas Sedlmayer <[email protected]>
@thomassedlmayer @pmai
Fix parts of coordinate system documentation
3494001 
- Add definition of host vehicle coordinate system
- Fix multiple bugs in coordinate transformation chapter
- Replace example figure that shows wrong phys. mounting pos. reference

Signed-off-by: Thomas Sedlmayer <[email protected]>
@pmai pmai force-pushed the fix/docs-coordinate-systems branch from e0dcbcc to 3494001 Compare June 15, 2023 17:30
@pmai pmai merged commit 7699763 into OpenSimulationInterface:master Jun 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ClemensLinnhoff ClemensLinnhoff ClemensLinnhoff approved these changes

@thempen thempen thempen left review comments

@jdsika jdsika Awaiting requested review from jdsika

Assignees
No one assigned
Labels
Harmonisation The Group in the ASAM development project working on harmonisation with other standards. HelpWanted I have tried my best - please help me! ReadyToMerge This PR has been approved to merge and will be merged by a member of the CCB.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@thomassedlmayer @thempen @pmai @ClemensLinnhoff @jdsika