Skip to content

clarified semantics currently supported with TrafficUpdate #525

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

Merged
merged 3 commits into from
Aug 4, 2021

Conversation

clemenshabedank
Copy link
Contributor

@clemenshabedank clemenshabedank commented Jun 14, 2021

Signed-off-by: Habedank Clemens qxs2704@europe.bmw.corp

#### Reference to a related issue in the repository
Add a reference to a related issue in the repository.

Add a description

Add a description of the changes proposed in the pull request.
Clarifying semantics after feedback from SETLevel project. The semantics for other potential use cases, e.g. multiple TrafficParticipants packaged in one model would need to be elaborated and specified (OSMP + OSI) and are therefore currently not supported. A trailer seems unproblematic in this regard, so keeping the field repeated is possible.

Some questions to ask:
What is this change?
What does it fix?
Is this a bug fix or a feature? Does it break any existing functionality or force me to update to a new version?
How has it been tested?

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

@clemenshabedank
Copy link
Contributor Author

By reading through the documentation of trailer_id I felt it could be made more clear that a trailer ID is basically a MovingObject ID. Any opinions on that? Maybe @DerBaertige @thempen

@clemenshabedank clemenshabedank added the TrafficParticipants The group in the ASAM development project working on traffic participants. label Jun 14, 2021
@caspar-ai
Copy link
Contributor

@clemenshabedank thanks for that change, think it looks good.

My only suggestion would be that there are two things to be explained:

  1. That TrafficUpdate is designed to only be used for a single traffic participant within a top level message
  2. Given that, why is update a repeated field

You change does a good job of capturing (2), but there isn't anything explicitly that states (1), perhaps we should add something in the header, around line 15?

@clemenshabedank
Copy link
Contributor Author

@caspar-ai thanks I hope e374ebc makes it clearer. Please feel free to make changes.

Copy link
Contributor

@caspar-ai caspar-ai left a comment

Choose a reason for hiding this comment

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

Looks good to me, thanks!

@clemenshabedank clemenshabedank added this to the V3.4.0 milestone Jun 15, 2021
@clemenshabedank clemenshabedank added the ReadyForCCBReview Indicates that this MR is ready for a final review and merge by the CCB. label Jun 15, 2021
@stefancyliax
Copy link
Contributor

OSI CCB:

Copy link
Contributor

@ThomasNaderBMW ThomasNaderBMW left a comment

Choose a reason for hiding this comment

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

Maybe the trailer design should have been otherwise because its confusing. It is how it is now, but for this message it clarifies a lot. So +1

@kmeids
Copy link

kmeids commented Jul 21, 2021

CCB Output 21.07.2021

  1. @max-rosin to check the documentation.
  2. @pmai will merge when @max-rosin is done.

@kmeids kmeids added 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. and removed ReadyForCCBReview Indicates that this MR is ready for a final review and merge by the CCB. labels Jul 21, 2021
@@ -11,7 +11,9 @@ package osi3;
//
// \brief The traffic update message is provided by traffic
// participant models to provide updates to their position, state
// and future trajectory back to the simulation environment.
// and future trajectory back to the simulation environment. The message is designed
// to update data of exactly ONE traffic participant, optionally with an attached
Copy link
Contributor

@max-rosin max-rosin Jul 29, 2021

Choose a reason for hiding this comment

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

I would recommend against capitalizing words for emphasis.

Suggested change
// to update data of exactly ONE traffic participant, optionally with an attached
// to update data of exactly onetraffic participant, optionally with an attached

Otherwise, the text is good.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@max-rosin Sure, it's fine for me. But two words please ;) Am I supposed to do it or will you do the change?

Habedank Clemens and others added 3 commits August 4, 2021 09:53
Signed-off-by: Habedank Clemens <qxs2704@europe.bmw.corp>
Signed-off-by: Habedank Clemens <qxs2704@europe.bmw.corp>
Signed-off-by: Pierre R. Mai <pmai@pmsf.de>
@pmai pmai force-pushed the bugfix/tp/clarify-trafficupdate-repeated branch from 443bbc0 to 8e79ad7 Compare August 4, 2021 07:54
@pmai pmai self-assigned this Aug 4, 2021
@pmai pmai merged commit 6ae62d6 into master Aug 4, 2021
@pmai pmai deleted the bugfix/tp/clarify-trafficupdate-repeated branch August 4, 2021 07:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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. TrafficParticipants The group in the ASAM development project working on traffic participants.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants