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.

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

feat: Add virtual detection area #781

Merged
merged 2 commits into from
Mar 25, 2024
Merged

Conversation

jruebsam
Copy link
Contributor

@jruebsam jruebsam commented Feb 26, 2024

Reference to a related issue in the repository

733

Add a description

see Issue

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

@jruebsam jruebsam force-pushed the feature/virtual-detection-area branch 4 times, most recently from 7831d29 to 5d06ae0 Compare February 26, 2024 08:22
@thempen
Copy link
Contributor

thempen commented Mar 11, 2024

Other Models & Harmonization Group:

  • The Polygon3D should not be restricted to 2 dimensions. The restriction should be made in the VirtualDetectionArea documentation.
  • The position to the virtual sensor coordinate system should be clarified.

@thempen thempen added the ReadyForCCBReview Indicates that this MR is ready for a final review and merge by the CCB. label Mar 11, 2024
@jruebsam jruebsam force-pushed the feature/virtual-detection-area branch 5 times, most recently from bfe2ea6 to 7548473 Compare March 11, 2024 09:21
@pmai pmai requested a review from PhRosenberger March 11, 2024 10:34
osi_common.proto Outdated Show resolved Hide resolved
@jruebsam jruebsam force-pushed the feature/virtual-detection-area branch from 7548473 to c45f95e Compare March 11, 2024 12:48
Copy link
Contributor

@PhRosenberger PhRosenberger left a comment

Choose a reason for hiding this comment

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

In general, I support the idea of the virtual detection area.

However, some more discussion is needed. Let's discuss the open points and questions I raised, shall we?

osi_sensordata.proto Outdated Show resolved Hide resolved
{
// A list of vertices
//
repeated Vector3d vertex = 1;
Copy link
Contributor

Choose a reason for hiding this comment

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

Why did you choose Vector3d / cartesian coordinates for the Polygon3d? - I suggest to use Spherical3d instead, because imho it is more common to use angles and ranges to describe the field of view of a sensor.

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually, I wonder why to use Polygon3d anyways, as you could describe the FoV with repeated Spherical3d coordinates already. Did you insert this intermediate step with the surfaces for a specific reason?

In other words: Why do we limit the Virtual detection area to be a composition of surfaces rather then a more flexibel solution? - Sensors like the Lovox Tele-15 definetly need more flexibility to be described.

Copy link
Contributor

@thomassedlmayer thomassedlmayer Mar 15, 2024

Choose a reason for hiding this comment

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

I think, Vector3d was used because that's what 3d engines offer for setting up such polygons. Converting this to spherical coordinates for transmission is just another step, I guess. So for our use case of visualizing such area Vector3d is completely fine.

The use case for this PR is just a quite basic visualization of such sensor cones, so we did not want it to become more complicated than it has to be. But I would agree, that the intention of this detection area should be mentioned in the description more specifically.

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 would say in general the Vector3d approach is more generic.
Using spherical3d defines something which always contains the origin at 0.
So for defining some abstract surface in 3d it will not be possible to use Spherical3d.

The question is how generic should it be ?

On the other side as Thomas mentioned the Vector3d Format is much better suited towards
rendering applications.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

edit: I just realized you can also exclude 0 from spherical ...

Copy link
Contributor

@pmai pmai left a comment

Choose a reason for hiding this comment

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

The prosposed wording changes might make the intent clearer.

osi_sensordata.proto Outdated Show resolved Hide resolved
osi_sensordata.proto Outdated Show resolved Hide resolved
@pmai
Copy link
Contributor

pmai commented Mar 25, 2024

CCB 2024-03-25: Merge with suggested changes.

@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 Mar 25, 2024
@pmai pmai merged commit f9de56c into master Mar 25, 2024
8 checks passed
@pmai pmai added this to the V3.7.0 milestone Apr 4, 2024
@jdsika
Copy link
Contributor

jdsika commented Apr 24, 2024

@pmai
We have the base_polygon. We should have defined this at the time?

//
// \brief Polygon in 2 dimensions
//
// A polygon in 2 dimensions which contains a list of vertices.
//
message Polygon2d
{
    // A list of vertices
    //
    repeated Vector2d vertex = 1;
}

Reviewed for v3.7.0

@jdsika jdsika deleted the feature/virtual-detection-area branch April 24, 2024 14:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ReadyToMerge This PR has been approved to merge and will be merged by a member of the CCB.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants