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

lighweight highway merging rules #1542

Merged
merged 21 commits into from
Jun 7, 2022
Merged

Conversation

kianzarrin
Copy link
Collaborator

@kianzarrin kianzarrin commented Apr 22, 2022

fixes #1530 . see issue for details.

ATTENTION: Only applies to highway on-ramps(merging) where lane arithmetic works (incoming= outgoing).

bonus: I also modified optimized the way global options variables are accessed. to speed up value access I cache the delegate. this reduces one if condition to check null.

latest branch build: TMPE.zip

tests:

  • routing: cars will stay in lane when condition is met. this can be tested using routing detector tool.
  • edit option: changing option takes effect immediately.
  • no more than one highway rules policy: enabling legacy highway rules or the new highway merging rules disables the other. it should be possible to turn both off.
  • option persistency: highway rule option should persist when saving/loading game
  • routing persistency: when saving with this option, existing highways will keep their routing after load.
  • main menu: it should not be possible to change this option in main menu.

test condition: according to #1530.

  • only works on highways. Does not work on urbran/transition nodes.
  • only works when merging
  • only works when lane mathematics add up (ie incoming lanes == outgoing lanes).
  • only works when there is no lane connections on the merging lanes. (same as legacy)
  • there is no cars may switch lanes on junctions rule. (same as legacy)
  • does not restrict lane arrow tool.

backward compatibility:

  • legacy highway rules still works
  • legacy routing persistency
  • legacy option takes effect immediately.

TLM/TLM/Manager/Impl/OptionsManager.cs Show resolved Hide resolved
@kianzarrin kianzarrin changed the title 1530 highway merging rules lighweight highway merging rules Apr 22, 2022
@originalfoo originalfoo added the LANE ROUTING Feature: Lane arrows / connectors label Apr 22, 2022
@originalfoo originalfoo self-requested a review April 22, 2022 21:29
@krzychu124
Copy link
Member

Werid behavior:

  • initial state: both options disabled
  • enabling highway rules (old) and then enabling highway rules (new) disables both and also calls OptionsManager.UpdateRoutingManager() 3 times.

@kianzarrin kianzarrin marked this pull request as draft April 22, 2022 23:28
@krzychu124
Copy link
Member

@kianzarrin did you test this? After reading the code I doubt it does anything when "new highway rules" is enabled and may apply changes to non-highway roads too if somehow works.

@kianzarrin
Copy link
Collaborator Author

@krzychu124 its draft. did you read description?

@krzychu124
Copy link
Member

It wasn't a draft when I was reviewing and testing the code...

image

@kianzarrin
Copy link
Collaborator Author

I did not ask for review on this on the description said this is not ready.

@kianzarrin
Copy link
Collaborator Author

@krzychu124 did you test this? After reading the code I doubt it does anything when "new highway rules" is enabled and may apply changes to non-highway roads too if somehow works.
I don't understand why you say that.
Except for a typo the code is otherwise fine:
image

and does not apply to non-highway either.
image

@kianzarrin kianzarrin marked this pull request as ready for review April 27, 2022 18:44
}
}

if (numOutgoingSegments == 1 && numIncomingSegents == 2 && !laneSwitching && highwayOnly) {
Copy link
Member

Choose a reason for hiding this comment

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

Hmm, so it's no longer restricted to one-way roads like legacy (line 552-553 below)?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That is actually a very interesting point. What should we do in case of 2 way highways like the highway over wall collection?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

practically numOutgoingSegments == 1 && numIncomingSegents == 2 only works for oneway (unless if you really try to break it but even then lane arithmetic won't add up).

I will add condition for oneway to clarify/full-proof the code.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

BTW I just noticed when using legacy highway rule option one lane may not be not used at all
image

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

OK now I only check for oneway highways.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I created #1561

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.

Amazing feature
Is there any easy way to graphically render incoming and outgoing lanes as a single curve? There should be a way, like build a bezier from center of incoming to center of outgoing using node as a bezier pivot. Or even from last 75% of incoming to first 25% of outgoing.

@kianzarrin
Copy link
Collaborator Author

@aubergine10 @krzychu124 I hope you didn't forget about this. I have addressed the issues you raised.

@kianzarrin
Copy link
Collaborator Author

@aubergine10 have your concerns been addressed?

@originalfoo originalfoo added this to the 11.6.6.0 milestone May 22, 2022
@originalfoo
Copy link
Member

@kianzarrin I'll be testing this later today.

@kianzarrin
Copy link
Collaborator Author

@aubergine10 can I merge this? (you have requested changes).

@kianzarrin kianzarrin merged commit 2c3d471 into master Jun 7, 2022
@kianzarrin kianzarrin deleted the 1530-highway-merging-rules branch June 7, 2022 21:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
LANE ROUTING Feature: Lane arrows / connectors
Projects
None yet
Development

Successfully merging this pull request may close these issues.

light highway merging rules
4 participants