689 add raytracerview config #699

LukasElster · 2023-01-12T10:55:12Z

Reference to a related issue in the repository

Add a description

PR adds RaytracerViewConfig to SensorView fpr raytracinf interfaces and changes Lidar/RadarSensorView in interfaces without rendering techniques related fields. Changes in Lidar and RadarSensorView are not backward compatible.

Take this checklist as orientation for yourself, if this PR is ready for the Change Control Board:

My suggestion follows the style and contributors guidelines.
I have taken care about the message documentation and the fields and enums documentation.
I have done the DCO signoff.
My changes generate no errors when passing CI tests.
I have successfully implemented and tested my fix/feature locally.
Appropriate reviewer(s) are assigned.

If you can’t check all of them, please explain why.
If all boxes are checked or commented and you have achieved at least one positive review, you can assign the label ReadyForCCBReview!

Signed-off-by: @lukas.elster <[email protected]>

…pen-simulation-interface into 689-add_raytracerview_config

…perate physical interface and rendering based interface Signed-off-by: @lukas.elster <[email protected]>

Signed-off-by: @lukas.elster <[email protected]>

PhRosenberger

I would not merge before clarifying if we write "ray tracer" or "raytracer" and we should correct the other small typos.

Additionally, I raised some additional questions to be answered before merging.

osi_common.proto

osi_sensorview.proto

osi_common.proto

osi_sensorview.proto

osi_sensorviewconfiguration.proto

Signed-off-by: @lukas.elster <[email protected]>

PhRosenberger

No, I guess, it's fine.

thomassedlmayer

We should probably rewrite the section between line 34 to 46 in osi_sensorviewconfiguration.proto as the number of rays is not included in any of the configurations anymore? Were these number_of_rays fields removed on purpose? In the current OSI version they are marked as raytracer specific characteristic, so I was just wondering where they went...

thomassedlmayer · 2023-01-25T16:04:09Z

osi_sensorviewconfiguration.proto

    //
-    // \note Rotation is defined analog Spherical3d


This note on the rotation got lost in the new SpatialSignalGain message which replaces the AntennaDiagramEntry message. Maybe we should add it again just to be clear?

Then we should add this note it in the osi_common, where the message is defined. What is not completly clear is the note itself, because we dont have the type Spherical3d here and one dimension less. Therefore the rotation can't be analog to Spherical3d. What do you think, @thomassedlmayer ?

Yes, it should be added in osi_common. I just added the note to the removed line.
You're right, we have one dimension less. But we should still determine the direction of the angles, right? E.g. right hand rule

osi_sensorviewconfiguration.proto

thomassedlmayer · 2023-01-26T08:11:20Z

osi_sensorviewconfiguration.proto

    //
-    optional uint32 max_number_of_interactions = 8;
+    optional double beam_divergence_horizontal = 7;


Maybe also rename the field of view-fields in RadarSensorViewConfiguration/Ultrasonic to beam divergence then? Does this make sense there too?

Also, the note that the rotation is as defined in Spherical3d is missing again.

beam_divergence is typically a parameter in the datasheet of the lidar sensor. In the case of radar the continious character of the electromagnetic wave together with the antenna diagramm characteristics define the beam_divergence. Therefore this field should not be added to radar. In case of ultrasonic I'm not shure. Can you help @PhRosenberger?

thomassedlmayer · 2023-01-26T08:21:26Z

osi_sensorviewconfiguration.proto

    //
-    repeated Vector3d directions = 11;
+    optional MountingPosition receiver_pose = 10;


We could maybe be consistent with the naming. In RayTracerSensorViewConfig these fields are called receiver_position/transmitter_position. Maybe also move these position fields up to the mounting position if we already move fields around anyway?

Good suggestion. I also added als explanation in transmitter_position // In case of lidar sensors this could be e.g. a mirror position. and for receiver_position // In case of lidar sensors this could be e.g. a photodiode.

osi_sensorview.proto

ClemensLinnhoff · 2023-03-03T07:29:27Z

osi_sensorview.proto

+    //
+    // The signal can be received from a different angle than it has been emitted to.
+    //
+    // \note Data is in sensor coordinate system.


Specify that it is the physical sensor coordinate system and not the virtual sensor coordinate system

ClemensLinnhoff · 2023-03-03T07:35:12Z

The corresponding issue #689 is assigned to milestone 3.6.0. This will not work as this PR breaks backwards compatibility. Can the PR be split into a compatible part for 3.6.0 and an incompatible part for 4.0?

jdsika · 2023-03-03T08:20:14Z

osi_sensorviewconfiguration.proto

+
+    // Different predefined ray tracer formats
+    // 
+    enum RayTracerFormat


I do not agree to have a raytracer format for every project. What's next? GaiaX format, Pegasus format? We need to come to an agreement here

I fully agree. Wouldn't it be possible to merge the four different raytracer types?
“unknown” and “other” have no attributes anyway. Usage of those types will incorporate ambiguities and doesn't contribute to harmonization.
The approaches of DIVP and Vivaldi largely overlap:

distance (which should be further elaborated) may coincide or be calculated form intersection_path_length + hitpoints

relative_speed is included in both approaches

polarized_attenuation can be calculated (afaik) from the Jones Vector

independent (optional?) parameters incorporate angles of incidence/reflection as well as first/last hit points...

Shouldn't it be possible to reach an agreement here?

LudwigFriedmann

As commented, I'd suggest to merge the four proposed RayTracerFormats.

LudwigFriedmann · 2023-03-03T09:16:03Z

osi_sensorviewconfiguration.proto

+
+    // Different predefined ray tracer formats
+    // 
+    enum RayTracerFormat


I fully agree. Wouldn't it be possible to merge the four different raytracer types?
“unknown” and “other” have no attributes anyway. Usage of those types will incorporate ambiguities and doesn't contribute to harmonization.
The approaches of DIVP and Vivaldi largely overlap:

distance (which should be further elaborated) may coincide or be calculated form intersection_path_length + hitpoints

relative_speed is included in both approaches

polarized_attenuation can be calculated (afaik) from the Jones Vector

independent (optional?) parameters incorporate angles of incidence/reflection as well as first/last hit points...

Shouldn't it be possible to reach an agreement here?

LukasElster added 13 commits November 14, 2022 16:05

First version of RaytracerView/Config

22e2659

Signed-off-by: @lukas.elster <[email protected]>

Added missing bracket

0a3bde8

Signed-off-by: @lukas.elster <[email protected]>

Replaced tabs with whitespace

507513e

Signed-off-by: @lukas.elster <[email protected]>

Added message type in sensor view configuration

8bd9721

Signed-off-by: @lukas.elster <[email protected]>

Changed message type of RaytracerFormat to optional

cae9817

Signed-off-by: @lukas.elster <[email protected]>

Edited ruling for optional field

3b4168f

Signed-off-by: @lukas.elster <[email protected]>

Merge branch 'master' of https://github.com/OpenSimulationInterface/o…

513cd44

…pen-simulation-interface into 689-add_raytracerview_config

Integrated changes in sensor view and sensor view configuration to se…

8673313

…perate physical interface and rendering based interface Signed-off-by: @lukas.elster <[email protected]>

Added SpatialSignalGain

f4f519e

Signed-off-by: @lukas.elster <[email protected]>

Bugfix

875cd70

Signed-off-by: @lukas.elster <[email protected]>

Removed non ASCII sign in Unit

96a107f

Signed-off-by: @lukas.elster <[email protected]>

Changed rules to run CI Pipeline

7dcc5b0

Signed-off-by: @lukas.elster <[email protected]>

Rule fix

63611e9

Signed-off-by: @lukas.elster <[email protected]>

LukasElster added the SensorModeling The Group in the ASAM development project working on sensor modeling topics. label Jan 12, 2023

LukasElster added this to the V4.0.0 milestone Jan 12, 2023

LukasElster requested a review from PhRosenberger January 12, 2023 10:55

LukasElster self-assigned this Jan 12, 2023

LukasElster linked an issue Jan 12, 2023 that may be closed by this pull request

RaytracerView_Config #689

Open

PhRosenberger requested changes Jan 20, 2023

View reviewed changes

Implemented chnages due to feedback of Philipp Rosenberger

6b9b67d

Signed-off-by: @lukas.elster <[email protected]>

PhRosenberger approved these changes Jan 24, 2023

View reviewed changes

LukasElster requested review from thomassedlmayer and pmai January 25, 2023 09:09

thomassedlmayer reviewed Jan 26, 2023

View reviewed changes

ClemensLinnhoff reviewed Mar 3, 2023

View reviewed changes

jdsika added the OpenMSL Required to enable sub-libraries in OpenMSL. label Mar 3, 2023

jdsika reviewed Mar 3, 2023

View reviewed changes

Merge branch 'master' into 689-add_raytracerview_config

9b5b2d0

LudwigFriedmann requested changes Mar 3, 2023

View reviewed changes

PhRosenberger mentioned this pull request Jan 17, 2024

Getting SensorView and SensorViewConfig topics done #764

Open

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

689 add raytracerview config #699

689 add raytracerview config #699

LukasElster commented Jan 12, 2023

PhRosenberger left a comment

PhRosenberger left a comment

thomassedlmayer left a comment

thomassedlmayer Jan 25, 2023

LukasElster Feb 22, 2023

thomassedlmayer Feb 22, 2023

thomassedlmayer Jan 26, 2023

LukasElster Feb 22, 2023

thomassedlmayer Jan 26, 2023

LukasElster Feb 22, 2023

ClemensLinnhoff Mar 3, 2023

ClemensLinnhoff commented Mar 3, 2023

jdsika Mar 3, 2023

LudwigFriedmann Mar 3, 2023

LudwigFriedmann left a comment

LudwigFriedmann Mar 3, 2023

689 add raytracerview config #699

Are you sure you want to change the base?

689 add raytracerview config #699

Conversation

LukasElster commented Jan 12, 2023

Reference to a related issue in the repository

Add a description

Take this checklist as orientation for yourself, if this PR is ready for the Change Control Board:

PhRosenberger left a comment

Choose a reason for hiding this comment

PhRosenberger left a comment

Choose a reason for hiding this comment

thomassedlmayer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ClemensLinnhoff commented Mar 3, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LudwigFriedmann left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment