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

dedicated turning lanes phase2 #755

Merged

Conversation

kianzarrin
Copy link
Collaborator

fixes #567
dedicated turning lanes now has a state-machine of 2-states:

Notes:

  • I moved code to dedicated UTIL class.
  • I polished the code to use the same code path for near and far turns reducing code complexity.

Test:
I have tested it on 1 lanes 2 lanes and 3 lanes and 12 lane roads with and without bus lanes with and without left hand drive.

@originalfoo
Copy link
Member

I have tested it on 1 lanes 2 lanes and 3 lanes and 12 lane roads

image

@originalfoo originalfoo added enhancement Improve existing feature LANE ROUTING Feature: Lane arrows / connectors Usability Make mod easier to use labels Feb 29, 2020
@originalfoo originalfoo added this to the 11.2 milestone Mar 2, 2020
@originalfoo
Copy link
Member

Ignore those two comments I added (now deleted) - had built the wrong PR, doh!!

@originalfoo
Copy link
Member

originalfoo commented Mar 6, 2020

Asymmetric roads, drive on left. This is the default vanilla state of the roads:

image

After Alt+Clicking each segment:

image

@kvakvs
Copy link
Collaborator

kvakvs commented Mar 6, 2020

There are arcs of angles somewhere in our code, which correspond to arrow directions. I suggest someone find them, check whether they match what the game is using, and maybe move out somewhere to lane arrow manager.
Might be just inconsistent angle arcs.

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's looking good.

@originalfoo originalfoo modified the milestones: 11.2, 11.3 Mar 28, 2020
@originalfoo
Copy link
Member

Looks like this branch is based on 11.1.0 tag - can you rebase to master?

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.

see comment above

@kianzarrin
Copy link
Collaborator Author

@aubergine10 @kvakvs I merged in the latest master version.

kvakvs
kvakvs previously requested changes Apr 1, 2020
TLM/TLM/UI/SubTools/LaneArrowTool.cs Outdated Show resolved Hide resolved
@originalfoo originalfoo added the DO NOT MERGE YET Don't merge this PR, even if approved, until further notice label Apr 1, 2020
@originalfoo
Copy link
Member

originalfoo commented Apr 3, 2020

Note: Left Hand Traffic

issue 1 - wrong arrows

Vanilla:
image0

Alt+Click segment in lane connector tool:
image

issue 2 - wrong arrows Vanilla: ![image](https://user-images.githubusercontent.com/1386719/78327082-ead8ff80-7573-11ea-9f96-f9f9859b9f18.png)

Alt+Click:
image

issue 3 - segment selection With this vanilla road, viewed from this camera angle, there was a selection dead-zone (indicated by red dashed area) where the mouse was not detecting the road.

image

If camera was rotated to pretty much any other angle, the road selection worked normally.

@kianzarrin
Copy link
Collaborator Author

kianzarrin commented Apr 4, 2020

issue 3 - segment selection
With this vanilla road, viewed from this camera angle, there was a selection dead-zone (indicated by red dashed area) where the mouse was not detecting the road.

I will check it out but most likely needs a issue with saved game and fixed camera attached to it (there is mod for that).
would be interesting to see if NetAdjust panel can hover the segment properly.

@kianzarrin
Copy link
Collaborator Author

I fixed the bug in my code. Now lane arrows should be right.

@kianzarrin
Copy link
Collaborator Author

issue 3 - segment selection
With this vanilla road, viewed from this camera angle, there was a selection dead-zone (indicated by red dashed area) where the mouse was not detecting the road.

@aubergine10 I didn't manage to reproduce this. Can you please create city with minimum test case and attach it to issue (please create new issue)

@originalfoo
Copy link
Member

Testing again now....

If I manage to recreate issue 3 I'll try and create a save with vanilla + TMPE and nothing else and attach to new issue.

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 in-game: It's working great now! And no recurrence of issue 3 either.

LGTM from end-user perspective! 👍

@kianzarrin kianzarrin removed the DO NOT MERGE YET Don't merge this PR, even if approved, until further notice label Apr 7, 2020
@kianzarrin kianzarrin requested a review from kvakvs April 7, 2020 15:15
@kianzarrin
Copy link
Collaborator Author

This is ready for review.

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.

Just little issues, but works great

TLM/TLM/UI/SubTools/LaneArrows/LaneArrowTool.cs Outdated Show resolved Hide resolved
TLM/TLM/UI/SubTools/LaneArrows/LaneArrowTool.cs Outdated Show resolved Hide resolved
@originalfoo
Copy link
Member

@kvakvs can this be merged now?

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.

i'm ok with that

@kianzarrin kianzarrin merged commit c680381 into CitiesSkylinesMods:master Apr 14, 2020
@kianzarrin kianzarrin deleted the 567-dedicated-turning-phase2 branch April 14, 2020 18:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Improve existing feature LANE ROUTING Feature: Lane arrows / connectors Usability Make mod easier to use
Projects
None yet
Development

Successfully merging this pull request may close these issues.

State machine for separate turning lanes.
4 participants