-
Notifications
You must be signed in to change notification settings - Fork 130
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
Conversation
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 thanks for that change, think it looks good. My only suggestion would be that there are two things to be explained:
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? |
@caspar-ai thanks I hope e374ebc makes it clearer. Please feel free to make changes. |
There was a problem hiding this 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!
OSI CCB:
|
There was a problem hiding this 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
CCB Output 21.07.2021
|
osi_trafficupdate.proto
Outdated
@@ -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 |
There was a problem hiding this comment.
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.
// 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.
There was a problem hiding this comment.
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?
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>
443bbc0
to
8e79ad7
Compare
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:
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!