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.

Sign up for GitHub

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

Jump to bottom

Fix vehicle stop at yield sign if no traffic #662

Merged
merged 2 commits into from
Feb 10, 2020
Merged

Fix vehicle stop at yield sign if no traffic #662

merged 2 commits into from
Feb 10, 2020

Conversation

krzychu124
Copy link
Member

Fixes issue with vehicle stopping at yield sign if no incoming traffic on main road

@krzychu124 krzychu124 self-assigned this Feb 7, 2020
@krzychu124 krzychu124 added the PRIORITY SIGNS Feature: Stop / Yield / Priority signs label Feb 7, 2020
@krzychu124 krzychu124 added this to the 11.1 milestone Feb 7, 2020
@krzychu124 krzychu124 linked an issue Feb 7, 2020 that may be closed by this pull request
v11: Vehicles stuttering or stopping at yield signs? #655
Closed
@krzychu124 krzychu124 added BUG Defect detected confirmed Represents confirmed issue or bug labels Feb 7, 2020
@originalfoo
Copy link
Member

How much of a pause are they expected to make when entering via Yield sign? For me they still seem to be slowing down a lot... (will post vid to discord in a bit)

originalfoo
originalfoo approved these changes Feb 7, 2020
View reviewed changes
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 👍

@originalfoo
Copy link
Member

With Yield traffic moving so smoothly on to junction, there's no visual difference between Yield roads and Priority roads most of the time. Maybe we should also reduce the MaxYieldVelocity a bit? Not sure how we'd make it update for end-users though, as their global config xml will already have the 2.5 value in it.

@originalfoo originalfoo mentioned this pull request Feb 7, 2020
Merged
@krzychu124
Copy link
Member Author

Not sure how we'd make it update for end-users though, as their global config xml will already have the 2.5 value in it.

If we bump config version it will reload values, I think.
On the other hand it looks like still something is missing, maybe value is not correct when it's passed to final speed calculation (missing conversion or something).

@originalfoo
Copy link
Member

does this need a 'do not merge yet' label?

@krzychu124 krzychu124 added the DO NOT MERGE YET Don't merge this PR, even if approved, until further notice label Feb 7, 2020
@krzychu124
Copy link
Member Author

krzychu124 commented Feb 10, 2020 •
edited

It looks like it works. No changes are necessary. We can make some tweaks later after feedback from users.

[EDIT] (...)or maybe not.... I have to test same savegame with old stable build...

@krzychu124 krzychu124 added DO NOT MERGE YET Don't merge this PR, even if approved, until further notice and removed DO NOT MERGE YET Don't merge this PR, even if approved, until further notice labels Feb 10, 2020
@krzychu124
Copy link
Member Author

Hmm, compared to 10.21.1 priorities behaviour looks very very similar. I would merge it

@krzychu124 krzychu124 removed the DO NOT MERGE YET Don't merge this PR, even if approved, until further notice label Feb 10, 2020
@originalfoo
Copy link
Member

Were we going to reduce the MaxYieldVelocity a bit and bump global config version to make sure it gets updated? I'll test again shortly but last time I tested this PR yield vehicles were too smooth and visually indistinguishable from priority traffic. Yield traffic should maybe slow just a little more?

@kianzarrin kianzarrin closed this Feb 10, 2020
@kianzarrin kianzarrin deleted the fix/stop-at-yield-sign-no-traffic branch February 10, 2020 12:10
@krzychu124
Copy link
Member Author

@kianzarrin what are you doing????

@originalfoo originalfoo restored the fix/stop-at-yield-sign-no-traffic branch February 10, 2020 13:52
@originalfoo
Copy link
Member

Restored branch and reopened PR

@originalfoo originalfoo reopened this Feb 10, 2020
@originalfoo
Copy link
Member

Going to merge this as it's working and should be part of 11.1 release. I'll create an issue to change the yield velocity, that can go in to 11.2

@originalfoo originalfoo merged commit 14567c3 into master Feb 10, 2020
@originalfoo originalfoo deleted the fix/stop-at-yield-sign-no-traffic branch February 10, 2020 14:52
This was referenced Feb 10, 2020
Open
Closed
@kianzarrin
Copy link
Collaborator

@krzychu124 I'm so sorry I got totally confused!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kvakvs kvakvs kvakvs approved these changes

@originalfoo originalfoo originalfoo approved these changes

Assignees

@krzychu124 krzychu124

Labels
BUG Defect detected confirmed Represents confirmed issue or bug PRIORITY SIGNS Feature: Stop / Yield / Priority signs
Projects
None yet
Milestone
11.1.0
Development

Successfully merging this pull request may close these issues.

v11: Vehicles stuttering or stopping at yield signs?
4 participants
@krzychu124 @originalfoo @kianzarrin @kvakvs