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

Adds dual_opposing_dedicated_right_turn_lanes map #36

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

agalbachicar
Copy link
Contributor

🎉 New feature

Closes #29

Summary

Solves the following map:

image

Test it

Following the instructions in the readme, you can reproduce the map.

Checklist

  • Signed all commits for DCO
  • Added tests
  • Added example and/or tutorial
  • Updated documentation (as needed)
  • Updated migration guide (as needed)
  • Consider updating Python bindings (if it affects the public API)

Note to maintainers: Remember to use Squash-Merge and edit the commit message to match the pull request summary while retaining Signed-off-by messages.

Signed-off-by: Agustin Alba Chicar <ag.albachicar@gmail.com>
@liangfok
Copy link
Contributor

Thanks for creating this! I'll test it ASAP.

Copy link
Contributor

@francocipollone francocipollone left a comment

Choose a reason for hiding this comment

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

Just some comments to go!

@francocipollone
Copy link
Contributor

One extra comment, you can add the generation of this map to the tool script: https://github.com/maliput/maliput_xodr/blob/main/tools/update_resources.sh

Signed-off-by: Franco Cipollone <franco.c@ekumenlabs.com>
Copy link
Contributor

@francocipollone francocipollone left a comment

Choose a reason for hiding this comment

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

LGTM

@francocipollone
Copy link
Contributor

Thanks for creating this! I'll test it ASAP.

@liangfok , I pushed some fixes mainly related to the traffic direction rules. For testing use the latest files that I now pushed.

@liangfok
Copy link
Contributor

Got it. I just grabbed the latest revision. Thanks for the note.

@francocipollone
Copy link
Contributor

Gently ping to check if this PR is mergeable as it is. @liangfok

@liangfok
Copy link
Contributor

This is still undergoing the initial review, but based on work in #37, I will likely have to re-generate the map using our downstreawm tooling once our downstream tooling supports stop lines with and without stop signs. I'm going to wait for #37 to pass initial testing before refocusing on this map.

@francocipollone
Copy link
Contributor

This is still undergoing the initial review, but based on work in #37, I will likely have to re-generate the map using our downstreawm tooling once our downstream tooling supports stop lines with and without stop signs. I'm going to wait for #37 to pass initial testing before refocusing on this map.

Friendly ping @liangfok . Based on this result, is it necessary to update something here?

@liangfok
Copy link
Contributor

I continue to wait for our downstream users to evaluate this map. This particular map is a lower priority for them, so it might be a while before they have the cycles to evaluate it. They are just starting to use the map in 54a70d4.

@francocipollone
Copy link
Contributor

I continue to wait for our downstream users to evaluate this map. This particular map is a lower priority for them, so it might be a while before they have the cycles to evaluate it. They are just starting to use the map in 54a70d4.

Great! Thanks for the update! Let's wait for the feedback then.

@francocipollone
Copy link
Contributor

I need to fix minor conflicts due to the last PRs that got merged. However, let me know if this PR is mergeable @liangfok

@liangfok
Copy link
Contributor

liangfok commented Dec 6, 2023

Will do. Currently our downstream users are focused on other maps, but will definitely return to this one in the future.

@liangfok
Copy link
Contributor

Let's rename this map "dual_opposing_dedicated_right_turn_lanes." This is to highlight the fact that the dedicated turn lanes are right turn lanes rather than left turn lanes, which is more important than the fact that they are oriented North/South.

Signed-off-by: Franco Cipollone <franco.c@ekumenlabs.com>
Signed-off-by: Franco Cipollone <franco.c@ekumenlabs.com>
@francocipollone
Copy link
Contributor

Let's rename this map "dual_opposing_dedicated_right_turn_lanes." This is to highlight the fact that the dedicated turn lanes are right turn lanes rather than left turn lanes, which is more important than the fact that they are oriented North/South.

Done.

@francocipollone
Copy link
Contributor

Ready to review @liangfok

@liangfok
Copy link
Contributor

liangfok commented Feb 5, 2024

Just a quick note that @max-shaan is helping out with the downstream usage of this map.

@liangfok
Copy link
Contributor

liangfok commented Feb 7, 2024

I noticed that the south-bound dedicated right turn lane is not modeled, which makes sense given the graphic above. I'm asking our downstream users whether this is expected since it is odd for there to be a south-bound middle lane in the north segment without it being a dedicated right-turn lane.

@francocipollone francocipollone changed the title Adds a crossing with a lane turning to the right. Adds dual_opposing_dedicated_right_turn_lanes map Feb 13, 2024
@liangfok
Copy link
Contributor

I confirm we want to model both dedicated right-turn lanes, see attached screenshot.

model_2nd_dedicated_right_turn

Side note: No need to annotate any lanes as having oncoming traffic. We can add that later when the need arises.

Signed-off-by: Franco Cipollone <franco.c@ekumenlabs.com>
Signed-off-by: Franco Cipollone <franco.c@ekumenlabs.com>
Signed-off-by: Franco Cipollone <franco.c@ekumenlabs.com>
Signed-off-by: Franco Cipollone <franco.c@ekumenlabs.com>
Signed-off-by: Franco Cipollone <franco.c@ekumenlabs.com>
Signed-off-by: Franco Cipollone <franco.c@ekumenlabs.com>
@francocipollone francocipollone force-pushed the agalbachicar/#29_dedicated_turn_lanes_north branch from 79a741a to 5982e29 Compare February 20, 2024 18:47
@francocipollone
Copy link
Contributor

francocipollone commented Feb 20, 2024

@liangfok

  • Requested changes were addressed:

  • Extended lead-in and lead-out to 300m

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Map request: dual_opposing_dedicated_right_turn_lanes
3 participants