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

Junction restrictions on unnecessary and unreachable pedestrian crossings #1558

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

Elesbaan70
Copy link
Contributor

This resolves #1302.

  • When a segment has no pedestrian lanes and is not part of the shortest path between two other segments with pedestrian lanes on the same node, crossings for that segment are disallowed by default.
  • When none of the segments on a node have pedestrian lanes, crossings are disallowed and are not configurable.

The "shortest path" is determined by the number of segments a pedestrian must cross, without regard for geometry; so it may not reflect true distances.

graph TD

    Allowed[Allowed=true<br/>Configurable=true]
    Configurable[Allowed=false<br/>Configurable=true]
    Disallowed[Allowed=false<br/>Configurable=false]

    HasPedestrians{Does this segment<br/>have pedestrian lanes?}
    HasPedestrians -->|Yes| Allowed
    HasPedestrians -->|No| OtherSegments

    OtherSegments{How many other segments<br/>have pedestrian lanes?}
    OtherSegments -->|More than 1| ShortestPath
    OtherSegments -->|1| Configurable
    OtherSegments -->|0| Disallowed

    ShortestPath{Does any shortest path go<br/>through this segment?}
    ShortestPath -->|Yes| Allowed
    ShortestPath -->|No| Configurable
Loading

Copy link
Collaborator

@kianzarrin kianzarrin left a comment

Choose a reason for hiding this comment

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

see comments above

@kianzarrin
Copy link
Collaborator

I like the idea of checking distance from both sides. it did not occur to me.

@kianzarrin
Copy link
Collaborator

Whats happening here?
image
I see zebra crossing flag at that segment end.

@Elesbaan70
Copy link
Contributor Author

Whats happening here?

I missed that! The existing code is hardcoded to bypass the computation when it's not configurable. (Great example of an unsafe optimization.) Node Controller wasn't affected because of the way the patch works.

I'm on mobile, but I'll fix it when I'm back at my computer.

@Elesbaan70 Elesbaan70 requested a review from kianzarrin May 7, 2022 02:54
@Elesbaan70
Copy link
Contributor Author

@kianzarrin Have your concerns been addressed?

@krzychu124 Have you had a chance to look at this?

@@ -645,18 +646,23 @@ public class JunctionRestrictionsManager
return ret;
}

private static bool IsNodePedestrianCrossingConfigurable(ref NetNode node) {
return (node.m_flags & (NetNode.Flags.Junction | NetNode.Flags.Bend)) != NetNode.Flags.None
Copy link
Collaborator

Choose a reason for hiding this comment

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

image
at bend nodes pedestrian crossing is configurable. Is this intentional?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is not a change. The code was simply moved from another location in this commit: Elesbaan70@8e732e6

Copy link
Collaborator

Choose a reason for hiding this comment

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

@Elesbaan70 are bend nodes within the scope of this PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would say no. That code was only touched to address an issue with the way default values were handled when not configurable. This PR is about segments without pedestrian lanes, and not really related to the type of node. And this is how the code worked before.

In my opinion, it should be configurable anyway.

@originalfoo originalfoo removed their request for review May 16, 2022 15:17
Copy link
Collaborator

@kianzarrin kianzarrin left a comment

Choose a reason for hiding this comment

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

works good in game.

@krzychu124
Copy link
Member

@krzychu124 Have you had a chance to look at this?

hehe, not yet but I'll try to find some time today or tomorrow. Changes make sense and should work as described, I just need to test it in game.
Unfortunately I already noticed (current Test and Stable) that sometimes pedestrians don't care about disabled crossing at highway segments or may even stuck if they finally notice it so there must be a bug hidden somewhere in the pathfinding code 😕

@Elesbaan70
Copy link
Contributor Author

Unfortunately I already noticed (current Test and Stable) that sometimes pedestrians don't care about disabled crossing at highway segments or may even stuck if they finally notice it so there must be a bug hidden somewhere in the pathfinding code 😕

The original idea that led to this change was just disabling all crossings when no segment on the node has pedestrian lanes at all. Should I put some conditional compiling in place to limit this to just that one change, until the issue you're describing is resolved?

@kianzarrin
Copy link
Collaborator

Unfortunately I already noticed (current Test and Stable) that sometimes pedestrians don't care about disabled crossing at highway segments or may even stuck if they finally notice it so there must be a bug hidden somewhere in the pathfinding code

is it with NCR?

@krzychu124
Copy link
Member

No, I posted screenshot yesterday on our discord. I've checked networks with Network detective and highway road doesn't have pedestrian lanes, only empty lanes (who knows what for). It's might be just some kind of asset issues but I didn't find anything obvious. I'll try to find the source of that problem at the weekend.

image

Should I put some conditional compiling in place to limit this to just that one change

I didn't test this PR yet so it's not related (not caused by your changes). Like I said I'll investigate what is going on and try to fix before we release next Test version

@Elesbaan70
Copy link
Contributor Author

My concern was that if disabling crossings is what triggers this problem, then until it's resolved, we might want to avoid introducing a disabled default state on nodes where it might matter.

@krzychu124
Copy link
Member

Yes, I understand, but it's still configurable (assuming there is at least one segment with pedestrian lanes) so there is IMO big chance that user will try to configure it the same way - will spot the same problem before I fix it, plus the thing it is already broken in some cases 😉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
JUNCTION RESTRICTIONS Feature: Junction restrictions Pedestiran Crossing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Disable pedestrian crossing if adjacent roads have no pedestrian lanes
4 participants