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

Add flag to filter static content from SensorView #407

Merged
merged 2 commits into from
Oct 14, 2021

Conversation

pmai
Copy link
Contributor

@pmai pmai commented Aug 18, 2020

This PR represents a PR from the SETLevel4To5 project to add a flag to the SensorViewConfiguration to indicate that no static content should be included in the SensorView ground truth information. It is purposefully left open what constitutes static information, to defer to ASAM discussions on this that transcend this PR. See OpenSimulationInterface/osi-sensor-model-packaging#56 for more details and the corresponding changes to packaging.

This PR is provided as a starting point for discussions in the relevant ASAM WPs.

Signed-off-by: Pierre R. Mai [email protected]

  • My code and comments follow the style guidelines and contributors guidelines of this project.
  • I have performed a self-review of my own code.
  • I have made corresponding changes to the documentation.
  • My changes generate no new warnings.
  • I have added tests that prove my fix is effective or that my feature works.
  • New and existing unit tests / travis ci pass locally with my changes.

@tbleher
Copy link
Contributor

tbleher commented Mar 12, 2021

@pmai (CC @clemenshabedank) Any chance this can make it into OSI 3.4?
I find that I need this flag in order to connect traffic agents in a real-time simulation - if all roads in the surrounding are transmitted all the time, it is practically impossible to have agents connected to a simulation in realtime (especially on complex realworld maps with lots of roads and width changes).

I understand that there needs to be some description of what will be omitted if this flag is set.

A relatively simple suggestion:

  • In the next version of OSMP, mandate that the OSMPGroundTruthInit message only contains static objects
  • Additionally, mandate in OSMP that if omit_static_information is set, no static objects are contained in the sensor view
  • Agent models can opt into this by referencing the newer version of OSMP (in their modelDescription.xml), and setting omit_static_information
  • The simulation environment can decide for itself which object is static and which is not, but it is required to ensure that OSMPGroundTruthInit contains only static objects (by its internal definition), and OSMPSensorViewIn contains only dynamic objects (of course, only if omit_static_information is set).
  • Then OSMPGroundTruthInit and OSMPSensorViewIn do not overlap, and can simply be merged in the agent model.
  • The simulation environment is completely free to decide which objects are static and which are not: e.g. if a simulation environment has support for dynamically changing road markings, it must not put any road markings that might change at runtime into OSMPGroundTruthInit.
  • The split between static and dynamic does not have to be per object-class. Some objects might truly be static in the simulation environment (e.g. a building), while other might be dynamic (e.g. a box that is moved by a person).

It seems to me that this could be done in a backwards-compatible manner for OSI 3.4.
Do you think this might work?

@clemenshabedank clemenshabedank added the Architecture The group in the ASAM development project working on architecture. label Apr 12, 2021
@clemenshabedank clemenshabedank added this to the V3.4.0 milestone Jun 9, 2021
@@ -190,6 +190,16 @@ message SensorViewConfiguration
// Unit: s
optional Timestamp simulation_start_time = 10;

// TBD: I guess we would not introduce this field in favor of the
// GroundTruthInitConfiguration.
Copy link
Contributor

Choose a reason for hiding this comment

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

TBD

// Omit static street layer
//
// The street layer, i.e. \c Lane, \c LaneBoundary and \c RoadMarking
// is regarded static and should be sent only at initialization.
Copy link
Contributor

Choose a reason for hiding this comment

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

TBD. Maybe we want a description more similar to the bool omit_static_objects, because even elements from the street layer might change during simulation.

@clemenshabedank
Copy link
Contributor

@ThomasNaderBMW @pmai @tbleher please have a look as well as the corresponding OSMP PR OpenSimulationInterface/osi-sensor-model-packaging#69

@stefancyliax
Copy link
Contributor

OSI CCB:

  • This needs to be topic at the architecture meeting on Friday (June 25th)!

@clemenshabedank
Copy link
Contributor

@tbleher @pmai @ThomasNaderBMW friendly reminder. Tomorrow we want to discuss it in the architecture group meeting together with the OSMP PR. Thank you !

// \note: While sending certain object at initialization and others
// at runtime, it must be ensured that each object still has a unique id.
//
optional bool omit_static_objects = 3;
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is a sensible definition, but will need matching changes to OSMP. Currently https://github.com/OpenSimulationInterface/osi-sensor-model-packaging/blob/master/doc/specification.rst says:

OSMPGroundTruthInit MUST contain all static data (e.g. roads) encountered by the model during a simulation run. Any dynamic data (e.g. MovingObjects) it contains MUST NOT be used and has no specified semantics.

If I remember correctly, the second sentence was added on request of dSpace, who said they could not guarantee that only static objects would be present in the GroundTruthInit message. (The change was OpenSimulationInterface/osi-sensor-model-packaging@cf890b4, in case you want to look up the original discussion in SETLevel4to5). I understand where this argument is coming from (probably most people just use an already existing function to generate the GroundTruth, add an object to the world, set the radius to maximum, and generate the GroundTruth from there), but I do not find it particularly compelling: it's quite easy to remove the dynamic objects before passing them to an FMU as part of OSMPGroundTruthInit.

If we allow to omit static objects (which I find important), then OSMP has to be changed to say that the OSMPGroundTruthInit message may only contain completely static objects, which will not appear in later messages. If any dynamic object (which can change later or disappear) is present in the initial ground truth, then it becomes impossible to later determine whether the object is static (and thus not present in a later sensor view), or whether it moved out of sight.

Note: I do find omit static objects to be important: our maps typically have 1000s of static objects (trees, poles, patches on the roads, buildings, ...). Example: the map I demoed on the "SETLevel4to5 Halbzeitevent" contains 3574 static objects on the 1x1km map. I had to disable transmitting any static objects (even just within the 200m radius of the pedestrian) to get acceptable performance. So this flag is quite crucial to get realtime performance.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, I see now that this is addressed in OpenSimulationInterface/osi-sensor-model-packaging#69 .

@stefancyliax
Copy link
Contributor

FYI. This PR is a likely candidate to be dropped from V3.4. Otherwise get in touch!
@pmai @tbleher @clemenshabedank

@tbleher
Copy link
Contributor

tbleher commented Sep 16, 2021

From my POV, this patch is critical to support agent models with OSI in a driving simulator. Without this, it is impossible to run agent models in real time on the maps we use.
FWIW: we will open source and publish our Pedestrian model from SETLevel in the near future. This model will depend on this flag. It would be a pity if the model was not usable with OSI 3.4.

What would be needed to get this into OSI 3.4?

@stefancyliax
Copy link
Contributor

From my POV, this patch is critical to support agent models with OSI in a driving simulator. Without this, it is impossible to run agent models in real time on the maps we use.
FWIW: we will open source and publish our Pedestrian model from SETLevel in the near future. This model will depend on this flag. It would be a pity if the model was not usable with OSI 3.4.

What would be needed to get this into OSI 3.4?

Well, getting the PR ready to merge in the next few day. Mid of next week at the latest!
Have approving reviews, have documentation reviews, etc..

@clemenshabedank
Copy link
Contributor

clemenshabedank commented Sep 16, 2021

@tbleher I think it's good that you make pressure to get it into v3.4. A bit of a problem is that we haven't finalized the discussion about what OSI regards as static / not static and didn't discuss the changes I made. Probably we won't be able to finish the discussion in the next few days (or maybe Friday Architecture meeting?). A potential solution would be to merge it as it came out of SET Level but we didn't define what static means so this would be open to interpretation.
Another option (probably worse) would be to try to release a v3.5 soon and introduce it then properly.
@pmai what is your opinion on this?

@stefancyliax
Copy link
Contributor

@kmeids Please decide what to do with this PR. If the project wants this in 3.4 we would need to merge in the next 2-3 days.

@kmeids
Copy link

kmeids commented Sep 20, 2021

Hello everyone,
I don't think this PR can make it to 3.4 for the following reasons:

  1. As mentioned by Clemens, the discussions on static/dynamic have not yet been finalized.
  2. This PR have did not yet undergo any CCB review or documentation review.

Moving this now to 4.0, if it is later on decided that a 3.5 will be created then we can update.

@kmeids kmeids modified the milestones: V3.4.0, V4.0.0 Sep 20, 2021
@pmai
Copy link
Contributor Author

pmai commented Sep 20, 2021

The major reason this PR was delayed was because we wanted to introduce the GroundTruthInitConfiguration system, and it was hence waiting whether the flag is still needed in this case.

That being said, I think the flag part of the PR could be merged, just clarifying that it just removes everything that is in the GroundTruthInit message. This will be true now, and will continue to be true in the future with the added configuration system.

The configuration system is IMHO too late for 3.4, but the flag could be easily done. I'll split out Clemens changes and adjust the wording accordingly.

@pmai pmai force-pushed the feature/sensor-view-config-static-flag branch from d0e991e to 7f11084 Compare September 20, 2021 16:02
@pmai pmai marked this pull request as ready for review September 20, 2021 16:06
Copy link
Contributor

@tbleher tbleher left a comment

Choose a reason for hiding this comment

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

Thanks, this looks sensible and covers my use-case 👍

@kmeids
Copy link

kmeids commented Sep 21, 2021

Good from my side as well, just please keep in mind that we need to pick this up again after this release!

@kmeids kmeids added ReadyToMerge This PR has been approved to merge and will be merged by a member of the CCB. ReadyForCCBReview Indicates that this MR is ready for a final review and merge by the CCB. labels Sep 21, 2021
@ThomasNaderBMW
Copy link
Contributor

@kmeids : Do we need a documentation review here or can we skip it this time? Or maybe it works for you @pasched to have a short check?

@ThomasNaderBMW ThomasNaderBMW modified the milestones: V4.0.0, V3.4.0 Sep 21, 2021
@kmeids kmeids added Documentation Everything which impacts the quality of the documentation and guidelines. and removed ReadyToMerge This PR has been approved to merge and will be merged by a member of the CCB. labels Sep 22, 2021
@kmeids
Copy link

kmeids commented Sep 22, 2021

@ThomasNaderBMW,
3.4 will be delayed let's take this PR to our next CCB meeting.

@tbleher
Copy link
Contributor

tbleher commented Oct 12, 2021

@kmeids What was the outcome of the CCB? Will this be merged for 3.4?

@kmeids
Copy link

kmeids commented Oct 12, 2021

@tbleher, CCB meeting will take place tomorrow.

@kmeids
Copy link

kmeids commented Oct 13, 2021

Output CCB 13.10.2021:

  1. GroundTruthInit will be further defined in future releases.
  2. adding "omit_static_information " to 3.4 is ready to merge @pmai.

@kmeids kmeids 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 Oct 13, 2021
@pmai pmai merged commit fbba25b into master Oct 14, 2021
@pmai pmai deleted the feature/sensor-view-config-static-flag branch October 14, 2021 15:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Architecture The group in the ASAM development project working on architecture. Documentation Everything which impacts the quality of the documentation and guidelines. 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.

Support for Partial Updates (e.g. of static GroundTruth data)
6 participants