CitiesSkylinesMods / TMPE

Cities: Skylines Traffic Manager: President Edition
https://steamcommunity.com/sharedfiles/filedetails/?id=1637663252
MIT License
569 stars 85 forks source link

Junction restrictions on unnecessary and unreachable pedestrian crossings #1558

Open Elesbaan70 opened 2 years ago

Elesbaan70 commented 2 years ago

This resolves #1302.

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
kianzarrin commented 2 years ago

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

kianzarrin commented 2 years ago

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

Elesbaan70 commented 2 years ago

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 commented 2 years ago

@kianzarrin Have your concerns been addressed?

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

krzychu124 commented 2 years ago

@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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 😉