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

more roundabout quick setup features #863

Merged
merged 10 commits into from
May 19, 2020
Merged

more roundabout quick setup features #863

merged 10 commits into from
May 19, 2020

Conversation

kianzarrin
Copy link
Collaborator

@kianzarrin kianzarrin commented Apr 24, 2020

Added new features to roundabout quick setup (fixes #578). They can be setup in the settings.

Once I implemented the undo feature for roundabout I decided to use it for high priority road and road selection panel as well:

  • CTRL+SHIFT+CLICK on a roundabout to set it up. do it again to restore it to the state it was before (previous rules setup by user are restored
  • same thing applits for CTRL+SHIFT+CLICK on high priority road or CTRL+SHIFT on high priority junction
  • Open the road selection panel and activate any of the buttons to setup traffic rules. Deactivate them again to undo those changes.
  • Controversial! : The clear button can now be activated. deactivating it will bring back the cleared traffic rules.
  • clicking on any of the traffic manager tools will hide the road selection panel, and deactivates all buttons. This WILL PREVENT the following scenario:
    1- activate roundabout rules on road selection panel.
    2-change some traffic rules using other TM tools.
    3- deactivate roundabout rules on road selection panel to undo the changes from step 1
    4- traffic rules modified in step 2 may be gone unexpectedly.

PS1: I temporarily have activated all new features by default for your testing convenience with a TOOD comment to change default before commit. I now have applied default options so if you want to test them you need to turn them on.
PS2: please discuss the feature future of the undo feature in #860 so as to not to pollute review comments.

- parking ban
- speed limit
- allow/ban entering blocked roundabout.
- undo (created IRecordable interace to take snapshot of the current state)
@kianzarrin
Copy link
Collaborator Author

Screenshot (901)

@kianzarrin kianzarrin changed the title added roundabout features: more roundabout quick setup features Apr 24, 2020
@kianzarrin
Copy link
Collaborator Author

kianzarrin commented Apr 24, 2020

My code
TrafficManager.zip

original code:
TrafficManager-master.zip

@chameleon-tbn
Copy link
Contributor

image

The Line Break sign in front of the Text comes from missing translation as @kianzarrin told me. So we have something to translate :)

@chameleon-tbn
Copy link
Contributor

During testing in Vanilla game I figured out that sometimes the Parking overlay is not showing up. If this happen it is no matter how often i click the (P)

image

Just zoom out a litte and the Parking Overlay is back.

image

@chameleon-tbn
Copy link
Contributor

chameleon-tbn commented Apr 25, 2020

Parking on Branches will never come back if you do following:

Steps to reproduce...

Klick round-about button
image

Klick Clear button
image

Klick round-about button to setup round-about rules again
image

Klick round-about button to undo round-about rules
image

Could be only solved by upgrading the branches with the road tool

@kianzarrin
Copy link
Collaborator Author

kianzarrin commented Apr 25, 2020

@thebugfixnet Parking on Branches will never come back if you do following:

images from previous comment

1-Klick round-about button
image

2-Klick Clear button
image

3- Klick round-about button:
image

4- click round-about button to undo round-about rules
image

This is expected behavior. Before step 3 the branches had parking bans. This is the state that is restored when undoing the roundabout at step 4.

@kianzarrin kianzarrin self-assigned this Apr 25, 2020
@chameleon-tbn
Copy link
Contributor

I figured out that the Speed-Limit function works within round-abouts in the same way they do on normal roads.

On normal roads with Shift-Klick the speed limit is assigned from one junction to the next. I can understand this as in real world also the speed limits at the next crossing may be different. So this behavior i was ecpexting.

But on roundabouts in my opinion there should be the logic to set the speed limit for the whole roundabout street with Shift-Klick. This is the way ot is in real i think. Additionally the super function of automated lower speed limits on roundabouts do it for the whole roundabout road. To be consistend to this function it would be good to handle it in the same way for the speed-limit function.

image

@originalfoo
Copy link
Member

Just zoom out a litte and the Parking Overlay is back.

It's due to the way we currently do caching based on camera position - if it's cached then you close tool, then open it again, it won't invalidate the cache until camera moves a little. We will be fixing that hopefully in v11.6 where we plan to make the caching more intelligent.

@kianzarrin
Copy link
Collaborator Author

@thebugfixnet this review is already too big. I created issue #869 for speed limit tool to cover the roundabout.

@originalfoo
Copy link
Member

Ok, just thinking about these options...

image

Once again we have 'enter blocked junction' used as a strange and confusing alternative to a stop or yield sign; made even more confusing as we also have priority sign features for roundabouts. This just does not sit well with me. Discussion on that topic can be found in #424 (particularly #424 (comment) under the image of a box junction).

Regarding the realistic speed limits, are we jumping in too soon with that feature? Is it the correct place to implement it? I think there are some questions we should consider...

  • Why is it only applied to roundabouts, and not all roads? All roads can have speed-affecting curves, not just roundabouts.
  • We could perhaps have a "magic wand" button on speed palette that would automatically apply realistic speed to a road. But that would be weird; in real life the speed limit sets maximum speed for road, and drivers are expected to slow at bends (otherwise they crash, so...).
  • So, maybe it would be better if it were an option on Gameplay options tab? Something like Road curvature affects speed. It could be implmented in much the same way as Road condition has a bigger impact on vehicle speed and perhaps part of that same code path? So we'd cache the curvature speed nerf on ExtSegment, users would set their road speeds in normal way but if the Road curvature affects speed option is enabled curved segments will have limited speed based on amount of curvature. That would apply to whole map.

@kianzarrin
Copy link
Collaborator Author

Once again we have 'enter blocked junction' used as a strange and confusing alternative to a stop or yield sign; made even more confusing as we also have priority sign features for roundabouts. This just does not sit well with me. Discussion on that topic can be found in #424 (particularly #424 (comment) under the image of a box junction).

as I said in #424 (comment) and #424 (comment) and #871 I think we are not ready to abolish enter blocked junctions. So I think until then we need to make it configurable. I think it would be wrong to wait until we find a better solution for enter blocked junction. I do not like the "perfect or nothing" approach (unless when it comes to reducing flexibility)

@kianzarrin
Copy link
Collaborator Author

@aubergine10

Regarding the realistic speed limits, are we jumping in too soon with that feature? Is it the correct place to implement it? I think there are some questions we should consider...
Why is it only applied to roundabouts, and not all roads? All roads can have speed-affecting curves, not just roundabouts.

Lets do it in several stages. For now I only do it for roundabout. Later we can expand this feature to other segments as well as we have discussed in #793

Copy link
Collaborator

@kvakvs kvakvs left a comment

Choose a reason for hiding this comment

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

Code looks good except naming/style comments.
Did not test the final version in game yet.

TLM/TLM/Manager/Impl/SpeedLimitManager.cs Show resolved Hide resolved
TLM/TLM/State/OptionsTabs/OptionsMassEditTab.cs Outdated Show resolved Hide resolved
TLM/TLM/UI/SubTools/PrioritySignsTool.cs Outdated Show resolved Hide resolved
TLM/TLM/Util/PriorityRoad.cs Show resolved Hide resolved
TLM/TLM/Util/PriorityRoad.cs Outdated Show resolved Hide resolved
TLM/TLM/Util/RoundaboutMassEdit.cs Outdated Show resolved Hide resolved
@kianzarrin
Copy link
Collaborator Author

kianzarrin commented Apr 30, 2020

This is broken. I need to figure out why.
Actually its not broken. I don't know why it didn't work the first time. but a fresh build fixed the problem (there was some null reference exception with regards to UIView).

@kianzarrin
Copy link
Collaborator Author

@thebugfixnet The Line Break sign in front of the Text comes from missing translation as @kianzarrin told me. So we have something to translate :)
image

No longer a problem
Screenshot (947)

@kianzarrin
Copy link
Collaborator Author

kianzarrin commented Apr 30, 2020

@aubergine10 Considering that you are not so found of the keep clear option I reduced it from 2 options to only one option:
Screenshot (948)

EDIT: also I now turned of speed limits and parking on branches by default so you need to turn them on to test them.

fixed option serialization.
@kianzarrin kianzarrin requested a review from kvakvs May 7, 2020 06:14
@originalfoo
Copy link
Member

Looks ok from initial testing. I still have some more testing to do.

@originalfoo originalfoo added JUNCTION RESTRICTIONS Feature: Junction restrictions Localisation Localised text and features PARKING Feature: Parking AI / restrictions / etc PRIORITY SIGNS Feature: Stop / Yield / Priority signs SPEED LIMITS Feature: Speed limits UI User interface updates labels May 17, 2020
@originalfoo originalfoo added this to the 11.6.0 milestone May 17, 2020
@kianzarrin
Copy link
Collaborator Author

kianzarrin commented May 17, 2020

@aubergine10 Do you agree with the UI? EDIT and functionality.

Copy link
Collaborator

@kvakvs kvakvs left a comment

Choose a reason for hiding this comment

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

Nice

@originalfoo
Copy link
Member

originalfoo commented May 17, 2020

I find this option confusing:
image
I assume it means something like "do not enter blocked junction if coming from a yield road"?

These could be better worded as Ban parking ...:
image

Also, I'm not sure users will comprehend what is meant by roundabout branches - I assume this refers to 'roads connecting to the roundabout'? Maybe it could be worded Ban parking on connecting roads?

image
From user perspective, what is the difference? Does one just set priority signs and the other set based on policies options?

The Shift+Ctrl+Click might be better summarised as Quick setup road/roundabout policies ?

@originalfoo
Copy link
Member

^ edited comment above

@originalfoo
Copy link
Member

Other than the minor text details above, it LGTM.

@kianzarrin
Copy link
Collaborator Author

From user perspective, what is the difference? Does one just set priority signs and the other set based on policies options?

yeah the only difference is priority signs. and also it could setup priority signs on low priory road too by applying yield signs. on the highlighted road. So I do think it is misleading.

It all can be fixed from the crowdin so let this merge with master before any conflicts come up!

@kianzarrin
Copy link
Collaborator Author

@aubergine10
image
image

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 👍

@kianzarrin kianzarrin merged commit 1a699f5 into master May 19, 2020
@kianzarrin kianzarrin deleted the roundabout+undo branch May 19, 2020 05:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
JUNCTION RESTRICTIONS Feature: Junction restrictions Localisation Localised text and features PARKING Feature: Parking AI / restrictions / etc PRIORITY SIGNS Feature: Stop / Yield / Priority signs SPEED LIMITS Feature: Speed limits UI User interface updates
Projects
None yet
4 participants