Skip to content

feat: New OSI message osi_route #705

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
Feb 13, 2024

Conversation

FabianPfeufferCAP
Copy link

Add a description

Added a new OSI message osi_route.proto to send a planned route to an agent or a device, such as a vehicle or navigation system. This route information can then be used to plan a path and traverse the road network.

Some questions to ask:
What is this change?

A new OSI feature to provide agent models with more logical data to perform path planning.

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

@tbleher
Copy link
Contributor

tbleher commented Feb 8, 2023

Thank you @FabianPfeufferCAP ! Looks good to me.
@ThomasNaderBMW I think this PR should be discussed in the "Other Models" WG, since it uses logical lanes.
First review in the Harmonization WG was generally favorable (though we did not do a detailed review of the patch, but we discussed the naming, general concept and placement within the OSI structures).

@ThomasNaderBMW
Copy link
Contributor

Feedback WG Other Models:

  • Does it have to match with OpenScenario e.g. Follow Path Actions?
  • Is logical lanes really the best option, if yes explain the use case better/ make it clear in the description of the message. For what shall it be used and for what not? What about LEVEL3 Use Cases, is it e.g. kind of global strategy or is this use case excluded?
  • Is there a connection to SetLevel4to5

@thomassedlmayer
Copy link
Contributor

WG Other Models (2023-05-08): Waiting for minor rework of PR (see comments by @ThomasNaderBMW). Ready for CCB after next Other Models WG meeting.

@ThomasNaderBMW
Copy link
Contributor

WG Other Models, open questions:

  • Presentation in OpenScenario-Group necessary. Is there something similar planned?
  • Information of Way Points necessary? Instead or as add on?
  • Next step: Presentation in appropriate OSC-Group

@thempen thempen added the ReadyForCCBReview Indicates that this MR is ready for a final review and merge by the CCB. label Jan 15, 2024
Fabian Pfeuffer added 2 commits January 15, 2024 15:58
The new message allows sending route information to a traffic agent
in the simulation. The route information can then be used by the
agent to traverse the road network.

Signed-off-by: Fabian Pfeuffer <Fabian.Pfeuffer@partner.bmw.de>
Signed-off-by: Fabian Pfeuffer <Fabian.Pfeuffer@partner.bmw.de>
@pmai pmai force-pushed the OsiRoute branch 4 times, most recently from 06e5461 to 7083ca2 Compare January 15, 2024 15:49
Signed-off-by: Pierre R. Mai <pmai@pmsf.de>
@pmai
Copy link
Contributor

pmai commented Feb 12, 2024

CCB 2024-02-12: Merge as-is.

@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 Feb 13, 2024
@pmai pmai merged commit 7953232 into OpenSimulationInterface:master Feb 13, 2024
@pmai pmai removed this from the V3.6.0 milestone Apr 4, 2024
@pmai pmai added this to the V3.7.0 milestone Apr 4, 2024
{
// The unique id of the route.
//
// \note This field is mandatory.
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

I suggest to put rules into a new issue and/or PR, as we would find more fields, where we would add them, right?

Copy link
Contributor

@ClemensLinnhoff ClemensLinnhoff May 2, 2024

Choose a reason for hiding this comment

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

I will create a new PR (see #805) an check everything for mandatory fields. We should definitely add this to v3.7.0, since I consider this a bug.

Copy link
Contributor

@jdsika jdsika left a comment

Choose a reason for hiding this comment

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

Review v3.7.0

@PhRosenberger changes suggested

@jdsika
Copy link
Contributor

jdsika commented Apr 23, 2024

I mention #804 here

@ClemensLinnhoff
Copy link
Contributor

I created a separate PR to add rules to existing comments describing rules: #806

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Other Models 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.

9 participants