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 GetVanillaNetInfoSpeedLimit() to only inspect customisable lanes #1362

Merged
merged 2 commits into from Feb 6, 2022
Merged

Fix GetVanillaNetInfoSpeedLimit() to only inspect customisable lanes #1362

merged 2 commits into from Feb 6, 2022

Conversation

originalfoo
Copy link
Member

Fixes: #1346

GetVanillaNetInfoSpeedLimit() was not filtering lanes to those which may have custom speed limits, resulting in problems like this (trace logging from #1346):

- Info 440.2720848: m_vehicleType = None, laneInfo.m_laneType = None, speedLimit = 1
- Info 440.2727466: m_vehicleType = None, laneInfo.m_laneType = None, speedLimit = 1
- Info 440.2734355: m_vehicleType = None, laneInfo.m_laneType = None, speedLimit = 1
- Info 440.2740341: m_vehicleType = None, laneInfo.m_laneType = None, speedLimit = 1
- Info 440.2746089: m_vehicleType = None, laneInfo.m_laneType = None, speedLimit = 1
+ Info 440.2752129: m_vehicleType = Car, laneInfo.m_laneType = Vehicle, speedLimit = 0.6
+ Info 440.2757840: m_vehicleType = Car, laneInfo.m_laneType = Vehicle, speedLimit = 0.6
Info 440.2763682: m_vehicleType = None, laneInfo.m_laneType = Pedestrian, speedLimit = 0.1
Info 440.2769400: m_vehicleType = None, laneInfo.m_laneType = Pedestrian, speedLimit = 0.1

In the example above, we should have been getting a return value of 0.6 but were instead getting 1.

This should fix some weird bugs at the following call sites:

  • Cached network speeds (LoadData() of SpeedLimitsManager)
  • Speed limit overlays
  • Roundabout bulk applicator (where it sets speed of roundabout based on curvature)

My biggest worry with this PR is how it will affect networks which aren't applicable for custom speed limits (eg. ship paths, pedestrian paths, etc) - I suspect we will need to create an additional method (which merely skips None, None type lanes, rather than looking for customisable lanes) for use in the LoadData() of SpeedLimitsManager but will wait for feedback from devs who have worked on this code before doing that.

@originalfoo
Rename GetVanillaNetInfoSpeedLimit -> `FindFastestCustomisableVanil…
33ed5b8 
…laLaneSpeedLimit`

Make it obvious it's not merely getting a value, it's doing some iteration.
@originalfoo
Filter lanes to those that can have custom speed limit
1fd20a5
@originalfoo originalfoo added BUG Defect detected high priority Affects lots of users STABLE TM:PE STABLE branch SPEED LIMITS Feature: Speed limits Overlays Overlays, data vis, etc. MASS EDIT The mass edit tool TEST TEST version of the mod / workshop page labels Feb 6, 2022
@originalfoo originalfoo added this to the 11.6.4-hotfix-7 milestone Feb 6, 2022
@originalfoo originalfoo self-assigned this Feb 6, 2022
kvakvs
kvakvs approved these changes Feb 6, 2022
View reviewed changes
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.

Alright

@originalfoo
Copy link
Member Author

@kvakvs In the LoadData() method, is it having to process stuff like pedestrian paths, ship paths, etc., or is it purely interested in customisable lanes?

@originalfoo originalfoo changed the title Fix get vanilla net info speed limit Fix GetVanillaNetInfoSpeedLimit() to only inspect customisable lanes Feb 6, 2022
@krzychu124
Copy link
Member

My biggest worry with this PR is how it will affect networks which aren't applicable for custom speed limits (eg. ship paths, pedestrian paths, etc) - I suspect we will need to create an additional method (which merely skips None, None type lanes, rather than looking for customisable lanes) for use in the LoadData() of SpeedLimitsManager but will wait for feedback from devs who have worked on this code before doing that.

It's only for configurable networks. There is no need to test other lanes than supported since unsupported things are handled by vanilla code, not TM:PE.

originalfoo added a commit that referenced this pull request Feb 6, 2022
@originalfoo
What's New in TM:PE 11.6.4.7 ?
3a2faeb 
- [Meta] TM:PE 11.6.4-hotfix-7
- [Meta] Bugfix for default speeds which affects speed limits tool, overlays, and roundabout curvature speed
- [Fixed] Default netinfo speed should only inspect customisable lanes #1362 #1346 (aubergine18)
- [Fixed] Fix `SPEED_TO_MPH` value in `ApiConstants.cs` #1364 #1363 #988 (aubergine18)
- [Removed] Obsolete: `SPEED_TO_MPH` and `SPEED_TO_KMPH` in `Constants.cs` #1364 #1363 (aubergine18)
@originalfoo originalfoo mentioned this pull request Feb 6, 2022
Merged
@originalfoo
Copy link
Member Author

I'm going to do some additional testing in-game just in case :)

@originalfoo
Copy link
Member Author

Ships, pedestrians, and even planes (my airport is working?!) confirmed working fine :)

Also confirmed speeds overlay working:

image

@originalfoo originalfoo merged commit b9e9d2c into master Feb 6, 2022
@originalfoo originalfoo deleted the fix-GetVanillaNetInfoSpeedLimit branch February 6, 2022 02:32
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

@krzychu124 krzychu124 krzychu124 approved these changes

@kianzarrin kianzarrin Awaiting requested review from kianzarrin

Assignees

@originalfoo originalfoo

Labels
BUG Defect detected high priority Affects lots of users MASS EDIT The mass edit tool Overlays Overlays, data vis, etc. SPEED LIMITS Feature: Speed limits STABLE TM:PE STABLE branch TEST TEST version of the mod / workshop page
Projects
None yet
Milestone
11.6.4-hotfix-7
Development

Successfully merging this pull request may close these issues.

Something is wrong with speed limits tool
3 participants
@originalfoo @krzychu124 @kvakvs