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

Logical Traffic Rules and Speed Limit #753

Merged
merged 10 commits into from
Apr 4, 2024
Merged

Conversation

lemmer-fzi
Copy link
Contributor

Introduce a message for traffic rule information for logical lanes. The message is intended to be used by traffic participants to easily get information in traffic rules without having to process traffic signs or other additional information.

While this contains an implementation for a speed limit traffic rule the message is designes so that it can be extended with additional traffic rule types in the future.

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

// Based on the type the respective message containing the information
// corresponding to the traffic rule should be filled.
//
optional TrafficRuleType traffic_rule_type = 1;
Copy link
Contributor

Choose a reason for hiding this comment

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

Because of the expendability of the type, the traffic_rule_type field could be repeated.
Therefore you would be able to have mutliple rules for the same type of vehicles and ranges. You have also a framwork to manage colliding rules. E.g. "no overtaking" and "you are allowed to overtake tractors".
You could for example use the order of the repeated field as a priority to work on. The order is fixed for protobuf and would not change: https://protobuf.dev/programming-guides/encoding/#optional

Copy link
Contributor

Choose a reason for hiding this comment

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

Discussion from Other Models group: This message should only contain the currently valid rule. There should not be the necessity for the agent model to select between contradictory rules.
This should be further clarified in the documentation.

@thomassedlmayer
Copy link
Contributor

@lemmer-fzi The enums (other than the fields numbers) should start with 0. You changed the one that was already correct to be wrong now as well ;)

@thempen thempen added the ReadyForCCBReview Indicates that this MR is ready for a final review and merge by the CCB. label Mar 11, 2024
@pmai pmai force-pushed the feature/logical-traffic-rules branch from f85853d to 9730cba Compare March 25, 2024 09:21
@pmai
Copy link
Contributor

pmai commented Mar 25, 2024

CCB 2024-03-25: Ready for merge from CCB point, will be merged after final review form @tbleher.

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 @pmai for the pointer, and @lemmer-fzi for the good work :)

I have added a few minor comments inline. Two general comments:

The first (maybe minor) comment: I currently see no way to explicitly define "no speedlimit" (for German highways). So I guess "no speedlimit" is modelled as the absence of a TrafficRule. This makes it impossible for a model, however, to know whether the simulation environment has provided traffic rules or not. I suggest explicitly modelling "no speedlimit".

The second comment might be a bigger issue: how are variable speed limits handled (e.g. from variable message boards, see type vmsBoard in OpenDRIVE 1.8)?
Typically the logical lanes would be part of GroundTruthInit and would be static. But speed limits can be dynamic. So it should (I think) at least be clarified what should happen if a speed limit changes (see OpenSimulationInterface/osi-sensor-model-packaging#108 (comment) for a few problems in that area).

Should the whole logical lane be sent again in the SensorView (which would be bad for performance)? Should an empty lane be sent, with just the limits set? What if the limit changes from e.g. 130km/h to "unlimited" (as pointed out above, then currently no TrafficRule would be set).

So I think the interaction between TrafficRule, vmsBoard and GroundTruthInit should be clarified before merge (note: as far as I am aware vmsBoard is not part of OSI yet, but I would expect a simulation environment which encounters a vmsBoard with an active speed limit on it to insert a speed limit sign into the SensorView).

osi_logicallane.proto Outdated Show resolved Hide resolved
osi_logicallane.proto Outdated Show resolved Hide resolved
osi_logicallane.proto Show resolved Hide resolved
osi_logicallane.proto Show resolved Hide resolved
osi_logicallane.proto Outdated Show resolved Hide resolved
osi_logicallane.proto Outdated Show resolved Hide resolved
osi_logicallane.proto Outdated Show resolved Hide resolved
osi_logicallane.proto Show resolved Hide resolved
lemmer-fzi and others added 10 commits April 4, 2024 12:19
Signed-off-by: Markus Lemmer <lemmer@fzi.de>
Signed-off-by: Markus Lemmer <lemmer@fzi.de>
Signed-off-by: Markus Lemmer <lemmer@fzi.de>
Signed-off-by: Markus Lemmer <lemmer@fzi.de>
Signed-off-by: Markus Lemmer <lemmer@fzi.de>
Signed-off-by: Markus Lemmer <lemmer@fzi.de>
Signed-off-by: Markus Lemmer <lemmer@fzi.de>
Signed-off-by: Markus Lemmer <lemmer@fzi.de>
Signed-off-by: Markus Lemmer <lemmer@fzi.de>
Signed-off-by: Pierre R. Mai <pmai@pmsf.de>
@pmai pmai force-pushed the feature/logical-traffic-rules branch from 8a305db to e41415e Compare April 4, 2024 10:19
@pmai pmai requested a review from thempen April 4, 2024 10:20
@pmai
Copy link
Contributor

pmai commented Apr 4, 2024

The first (maybe minor) comment: I currently see no way to explicitly define "no speedlimit" (for German highways). So I guess "no speedlimit" is modelled as the absence of a TrafficRule. This makes it impossible for a model, however, to know whether the simulation environment has provided traffic rules or not. I suggest explicitly modelling "no speedlimit".

As discussed in today's CCB meeting, this is not considered a specific problem for the traffic rules, as this is a general problem for OSI (detection whether the absence of a field/entry is due to lack of support or no content), hence should be handled at a different level. Especially for the traffic rules, there is only a single instance where no speed limit exists, i.e. on motorways for certain vehicle types, hence there will always be a rule in place somewhere that makes this detection possible.

The second comment might be a bigger issue: how are variable speed limits handled (e.g. from variable message boards, see type vmsBoard in OpenDRIVE 1.8)? Typically the logical lanes would be part of GroundTruthInit and would be static. But speed limits can be dynamic. So it should (I think) at least be clarified what should happen if a speed limit changes (see OpenSimulationInterface/osi-sensor-model-packaging#108 (comment) for a few problems in that area).

As discussed in today's CCB meeting, this is already an existing problem, as traffic signs are usually also considered static but can change dynamically (see traffic sign variability), and in consequence certain lane properties are likely to change (e.g. lane type from TYPE_STOP to TYPE_NORMAL for signs turning the hard shoulder into an extra normal lane).

So this whole topic will have to be handled as part of the work of determining dynamic/static parts and the handling of updates. We always knew that GroundTruthInit was a partial solution that needs more definition going forward, and dynamic signs (which already are a part of OSI) just make this more pressing.

So I think the interaction between TrafficRule, vmsBoard and GroundTruthInit should be clarified before merge (note: as far as I am aware vmsBoard is not part of OSI yet, but I would expect a simulation environment which encounters a vmsBoard with an active speed limit on it to insert a speed limit sign into the SensorView).

As stated above, dynamic traffic signs have been a part of OSI for a long time, how to handle them in the face of GroundTruthInit has been unspecified for as long.

@pmai
Copy link
Contributor

pmai commented Apr 4, 2024

@thempen I have added the discussed text and made some spelling/formatting fixes. Please approve if this matches your expectations.

@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 Apr 4, 2024
@pmai pmai merged commit c6735c6 into master Apr 4, 2024
8 checks passed
@pmai pmai deleted the feature/logical-traffic-rules branch April 4, 2024 13:35
@pmai pmai added this to the V3.7.0 milestone Apr 4, 2024
@jdsika
Copy link
Contributor

jdsika commented Apr 24, 2024

I am actually a bit indecisive if I really like this feature... we replicate information that is part of the trafficSign assigned_lane_id.

The answer to be comment will be "we already have a lot of redundant information anyway"... I think I want to eventually switch to a work model where suggesting new features is somehow supported by showing a useful implementation along with it.

Reviewed v3.7.0

@ClemensLinnhoff
Copy link
Contributor

The wording might be a bit misleading. Logical lanes are the equivalent of OpenDRIVE lanes:

If OSI is generated from OpenDRIVE, then LogicalLanes map directly to OpenDRIVE lanes.

But in OpenDRIVE the term "traffic rule" refers to left handed or right handed traffic (see documentation).

@thempen
Copy link
Contributor

thempen commented May 8, 2024

This was discussed in the CCB on monday (6.5.2024):
"Traffic rule" is seen as a much more generic description, as only left handed or right handed traffic, therefore in our opinion, the wording in OpenDRIVE should be changed in the long run. (It can't be changed right now, because of the backwad compatibility)
Having this name, as it is, could theoretically mislead a user. The messages however are so different, that it is nearly impossible to implement it in the wrong way.
There is a big overlap between OpenDRIVE and OSI, but there will be differences, when naming conventions will not be seen as useful.

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

None yet

7 participants