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

Quick setup of Priority roads #621

Merged
merged 46 commits into from
Feb 10, 2020

Conversation

kianzarrin
Copy link
Collaborator

@kianzarrin kianzarrin commented Jan 21, 2020

Fixes #541
related #542
related #568: undo delete option.
Usage:

  • select priority sign tool.
  • Ctrl+click to quick setup priority junction
  • Ctrl+Shift+Click to do the same to multiple segments.

Situations handled: *

  • minor road joins main avenue
  • Split 2 way road into one-way roads
  • semi-roundabout
  • no yield required when lane arithmetic is observed

* see #541 (comment) for test city and more information.

Crowdin:
I asked krzychu to add crowding keys. I also modified tutorial and hotkey corwdin texts (waiting for approval)
FYI:

EDIT: @krzychu124 did not respond. Can anyone please add these keys?

Options.csv:
"MassEdit.Group.Priority roads"->"Priority roads"
"Priority roads.Option:Allow far turns"->"Allow cars to take far turn from/into main road (not recommended)"
"Priority roads.Tooltip:Allow far turns"->"If enabled, lane arrows are untouched and cars can cross road median (does not apply when splitting avenue)."
"Priority roads.Option:Enter blocked yield road"->"Allow cars on yield road to enter blocked main road"
"Priority roads.Option:Stop signs on entry"->"Use stop signs when entering main road"
"Priority roads.Tooltip:Stop signs on entry"->"If enabled, puts stop signs on roads entering the main road. If disabled, puts yield signs instead"
"Priority roads.Option:Allow pedestrian crossings on main road"->"Allow pedestrian crossings on main road"

I made the mass edit overly to last a second after processing is done so that the user has some visual feedback that something has happened.

Ignore arrows for one way main road.
Polished round about code.
Fixed bug in roundabout turning right.
roundabout search path prunes paths that are too long.
polish
doc
…HandTraffic.

search for duplicated nodes inorder to check for roundabout.
polish + documentation.
@kianzarrin
Copy link
Collaborator Author

Also, Ctrl seems to be setting it's place as the "multitool tool". Whereas Shift will just propagate basic tool down the road, Ctrl+Shift would apply other tools too (junction restricitons, lane arrows, etc...) hence desire for Alt+Ctrl and Alt+Ctrl+Shift.

in comparison CTRL+SHIFT does the following:

  • remove two crossings from main road
  • add yield sign on connected road[s]
  • ban far turn on connected road[s]
  • ban far turn[s] on main road
  • go through blocked junction on main road.
    Even remembering all this requires a lot of energy. let alone applying them all to several junctions with no mistakes.

so ALT+CTRL+Shift only sets priority signs and go through blocked junction? that is something that can be easily done manually. if the objective is to set go through blocked junction on multiple junctions at once it should be part of bulk edit feature in junction restriction.

If you still feel strongly about ALT+CTRL+Shift I can add it but in my opinion its too much and it only confuses people.

@originalfoo
Copy link
Member

With the way it currently stands, if I want alternate crossing mode (either remove or don't remove, ie. opposite of whatever is set in mod options) I have two options which are both clunky:

  1. Change mod options:
    • Exit all the way to pause menu
    • Select options, find TMPE in list
    • Go to policies tab, change option
    • Exit from options
    • Go back in to tool, use the shortcut
    • or...
  2. Switch tools:
    • Apply the usual shortcut in priority tool
    • Switch to junction restrictions tool
    • Select junction
    • Change crossings
    • Potentially for several junctions

Having Alt key temporarily toggle what happens to crossings over main road would be much easier.

From user perspective, I don't think they will try and remember what Ctrl modifier does (as it's a bit differnet depending on each junction). The Alt modifier will just be: "Whatever Ctrl would do, but invert what happens to crossings over main road"

Like, if I want normal behaviour (remove crossings) along route, except on couple of junctions, I could:

  • Ctrl+Shift+Click the route
  • Alt+Ctrl+Click the two junctions

No need to change tools or mod options, just very fluid and quick interaction with my roads using one tool.

@originalfoo
Copy link
Member

originalfoo commented Feb 6, 2020

BTW, I'm already using this bulk applicator all over my city, it's awesome!!

The only usability friction I'm encountering is that occasional need to invert what happens to main road crossings.

@kianzarrin
Copy link
Collaborator Author

kianzarrin commented Feb 7, 2020

@aubergine10 Having Alt key temporarily toggle what happens to crossings over main road would be much easier.
From user perspective, I don't think they will try and remember what Ctrl modifier does (as it's a bit differnet depending on each junction). The Alt modifier will just be: "Whatever Ctrl would do, but invert what happens to crossings over main road"
Like, if I want normal behaviour (remove crossings) along route, except on couple of junctions, I could:
Ctrl+Shift+Click the route
Alt+Ctrl+Click the two junctions

What happens to lane arrows?

having the alt key doing opposite of what crossings do in the options is strange. maybe remove that option? maybe provide a panel like the one in roundabout mod? maybe we can do this in the road selection panel? I feel like we are rushing in the ALT modifier. lets open an issue about this so that we can have some proper discussion. It would be wrong to shove it in a code review last minute.

Screenshot (413)

EDIT: isn't it illegal to block junctions when cars joining the road can take the far turn?

@brunoais
Copy link

brunoais commented Feb 7, 2020

EDIT: isn't it illegal to block junctions when cars joining the road can take the far turn?

Where I live, that's only illegal if there's markings on the ground meaning as such. Otherwise, it's legal

@kianzarrin
Copy link
Collaborator Author

Translation keys work.
Screenshot (414)

@kianzarrin
Copy link
Collaborator Author

@aubergine10 I created a branch with the ALT key just for you! https://github.com/kianzarrin/Cities-Skylines-Traffic-Manager-President-Edition/tree/AltPriorityRoad

the alt key force applies pedestrian crossing regardless of the options.
alt+ctrl+shift+click (road)
alt+ctrl+click (junction)

Copy link
Member

@originalfoo originalfoo left a comment

Choose a reason for hiding this comment

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

LGTM 👍 Will test the Alt branch, that could be added later maybe

@originalfoo
Copy link
Member

@kvakvs @krzychu124 : Please test this PR. I'd ideally like to get it published to LABS this weekend so end-users can start testing. It's the main "new feature" of v11.1 release.

@kvakvs
Copy link
Collaborator

kvakvs commented Feb 8, 2020

Motorway merge 2+1=3
Lane arithmetic is observed
Yield is produced too
bild

This is case of semi roundabout.

If the user applies quick-fix on junctions where it does not make sense that's on him!

That does not mean my code is good. actually in this case 2 wrongs make a right!

@originalfoo
Copy link
Member

Good catch!!

@kianzarrin
Copy link
Collaborator Author

kianzarrin commented Feb 8, 2020

OK made 2 fixes:
Fix number 1: fix for counting lanes of Asym roads.
Fix number 2: better isSemiRoundabout detection and I call fix functions from roundabout utility.

Screanshot 1: Only Fix number 1.
ScreanShot 2 and 3: Both fixes.

Screenshot (426)
Screenshot (428)
Screenshot (429)

Note that some roundabouts have roads connected from inside.

Copy link
Member

@originalfoo originalfoo left a comment

Choose a reason for hiding this comment

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

Tested latest commit in-game, it's working well for me. Sooner we can get this out to LABS users and have them test it on their weird and wonderful cities the sooner we'll find any lingering issues. Hopefully this gets merged soon, it's very useful tool.

Copy link
Member

@krzychu124 krzychu124 left a comment

Choose a reason for hiding this comment

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

I'm worried a bit about those lock thingies (I think it won't work - you probably need to lock whole instance). Other parts of code looks good 👍

@krzychu124
Copy link
Member

@aubergine10 is it me, or delete button was changed from recycle bin to x

@kianzarrin
Copy link
Collaborator Author

@krzychu124 I'm worried a bit about those lock thingies (I think it won't work - you probably need to lock whole instance).

Whole instance of what? Can you please elaborate. I don't have too much experience in multi-threading. What can go wrong here? what should I do?

@aubergine10 @kvakvs any ideas?

@kianzarrin
Copy link
Collaborator Author

@aubergine10 is it me, or delete button was changed from recycle bin to x

I has always been x for me. maybe I should merge.

@krzychu124
Copy link
Member

v10.21.1
vlcsnap-error890

Regards to locking: https://stackoverflow.com/a/505604

@originalfoo
Copy link
Member

originalfoo commented Feb 10, 2020

IMO that button doesn't matter too much; I think at some point the Priority Signs UI workflow / interaction model needs changing to something more like junction restrictions. So don't delay merging this becaue of that button.

@krzychu124
Copy link
Member

Just noticed a change when I was testing priority bug, you can merge it 😉

@kianzarrin
Copy link
Collaborator Author

Regards to locking: https://stackoverflow.com/a/505604
@aubergine10 I don't see it. In the link you sent Its returning a class by ref. I do not return a class. I return bolean. DateTime is also a struct (which i don't return). I am not even using any byref values let alone mishandling them.

Am I missing something. is there another problem maybe?

@kianzarrin kianzarrin merged commit ca6dee7 into CitiesSkylinesMods:master Feb 10, 2020
@kianzarrin kianzarrin deleted the PriorityRoad branch February 10, 2020 10:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
JUNCTION RESTRICTIONS Feature: Junction restrictions LANE ROUTING Feature: Lane arrows / connectors PRIORITY SIGNS Feature: Stop / Yield / Priority signs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

shortcut to fix traffic rules where minor road joins a main avenue
5 participants