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

feat: Add virtual detection area #781

Merged
merged 2 commits into from
Mar 25, 2024
Merged

feat: Add virtual detection area #781

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c45f95e
feat: Add virtual detection area
jruebsam Feb 25, 2024
14593d6
Apply suggestions from code review
pmai Mar 25, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 12 additions & 0 deletions osi_common.proto
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1089,3 +1089,15 @@ message KeyValuePair
//
optional string value = 2;
}

//
// \brief Polygon in 3 dimensions
//
// A polygon in 3 dimensions which contains a list of vertices.
//
message Polygon3d
{
// 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 •
edited
Loading

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 ...

}
19 changes: 19 additions & 0 deletions osi_sensordata.proto
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -313,4 +313,23 @@ message SensorData
// in cartesian coordinates.
//
optional LogicalDetectionData logical_detection_data = 27;

//
// \brief Virtual detection area of a sensor
//
// The virtual detection area describes the area the sensor can see in a best case scenario
pmai marked this conversation as resolved.
Show resolved Hide resolved
// where no occlusion or other statistical effects are considered. It is described by a set
// of polygons.
pmai marked this conversation as resolved.
Show resolved Hide resolved
//
message VirtualDetectionArea
{
// List of polygons. Each polygon represents a surface of the virtual detection area
// and is given with respect to the virtual sensor coordinate system.
//
repeated Polygon3d polygon = 1;
}

// Virtual detection area of the sensor
//
optional VirtualDetectionArea virtual_detection_area = 28;
}