Train tracks segment speed limits #1467

krzychu124 · 2022-03-15T20:59:11Z

Closes #1337

changed calculation of average speed limit from the per direction to the lane vehicle type,
added three additional directions to the method which applies segment speed limits,
eliminated SpeedValue equality check boxing

…ted lane vehicle types

…uality

TLM/TMPE.API/Traffic/Data/SpeedValue.cs

originalfoo

Still need to test in-game, but just some comment on the code for now...

TLM/TLM/UI/SubTools/SpeedLimits/Overlay/OverlaySegmentSpeedlimitHandle.cs

When `null`, the speed will be applied to all lanes _except_ those with `None` direction.

Removes loop of directions.

This is simplified to take advantage of the updates to `SpeedLimitManager.Instance.SetSegmentSpeedLimit()` in which lane direction specification is no longer mandatory. It wipes out several loops, particularly over segment lanes.

TLM/TLM/Manager/Impl/SpeedLimitManager.cs

kianzarrin · 2022-03-23T11:04:30Z

might be out of scope of this PR but do these warning need action? if not maybe we should hide them.

krzychu124 · 2022-03-23T12:14:34Z

might be out of scope of this PR but do these warning need action? if not maybe we should hide them.

It's harmless in this case. We've talked about that on Discord. Warning makes sense to all custom structs but not primitive types. So for ushort is not problem but for idk. Vector3, TimeSpan it will be a problem.

kvakvs · 2022-03-23T12:37:27Z

I think that field never changes, don't worry.

krzychu124 · 2022-03-23T12:40:17Z

I think that field never changes, don't worry.

warning does not warn about field but behaviour when field type is a non-readonly struct (compiler will create defensive copy on each access (create variable and set it with field value before use) to ensure original value won't ever change)

kianzarrin · 2022-03-23T13:12:32Z

if it is harmless then I think it would be wise to hide that warning. if there is too many warnings around then ppl will not pay attention and then important warnings can be ignored. but that is outside the scope of this PR.

originalfoo · 2022-03-23T16:33:40Z

I'll send a commit to suppress that warning inline, handle a few other warnings, and also add lane type & vehicle type checks back in to the segment iteration function (in ClickMultiSegment - just to ensure we don't go iterating down non-applicable routes).

Completely useless warning for: "A closing brace within a C# element, statement, or expression is preceded by a blank line."

Useful for checking if a segment has at least one matching lane for `LANE_TYPES` and `VEHICLE_TYPES`.

TLM/TLM/Util/Extensions/NetSegmentExtensions.cs

kianzarrin · 2022-03-24T12:11:16Z

TLM/TLM/Util/Extensions/NetSegmentExtensions.cs

+
+                    return true;
+                }
+


I like the fact that you have not used goto nextiter :)

I vowed never to use goto again in 1986 :)

I still think goto has some uses like breaking out of nested loops.

kianzarrin · 2022-03-24T12:20:30Z

TLM/TLM/Util/Extensions/NetSegmentExtensions.cs

@@ -72,6 +72,41 @@ public static class NetSegmentExtensions {
            return new GetSegmentLaneIdsEnumerable(initialLaneId, netInfo.m_lanes.Length, laneBuffer);
        }

+        /// <summary>


[Note]
Based this code laneType or VehicleType should never be 0. Maybe we can Assert this condition even if it is only in DEBUG build?
Is there a [NotNull] or [NotZero] annotation we can use here?

in some places in CS they check forlaneType == 0 || (laneInfo.m_laneType & laneType) != 0) but I I think in speed limit manager that does not make sense.

I've added a DEBUG-only AssertNotNone shortcut to test both params.

never mind. I deleted message.

[low priority]
is this critical code path. do the assertion need to be debug only?

[low priority]
is this critical code path. do the assertion need to be debug only?

heh, it was your suggesstion in #1467 (comment) :

Maybe we can Assert this condition even if it is only in DEBUG build?

TLM/TLM/Manager/Impl/SpeedLimitManager.cs

TLM/TLM/UI/SubTools/SpeedLimits/Overlay/OverlaySegmentSpeedlimitHandle.cs

It's a relic from older incarnation of speed limit tool.

`AnyApplicableLane()` will never return `true` if either of the relevant params are `None`.

kianzarrin · 2022-03-24T21:35:17Z

why the speed limit sprites are not on lanes? is it out of scope of this PR?

kianzarrin · 2022-03-24T21:36:53Z

another visual problem. when I hold shift on roundabout:

lane-wise overlay works on roundabout
segment-wise overlay does not (does not highlight roundabout)
if i shift click though it does change the speed of roundabout.

kianzarrin

see comments above.

originalfoo · 2022-03-24T22:17:48Z

segment-wise overlay does not (does not highlight roundabout)

Sounds like issue with the overlay (not in scope of this PR). Please create issue to track that.

why the speed limit sprites are not on lanes? is it out of scope of this PR?

That's just the way the render overlay works; for per-lane it uses icon grid with icon-sized gap in the middle. It could be improved, perhaps, but I wouldn't bother until we create a centralised overlay manager.

kianzarrin · 2022-03-24T23:00:25Z

You are right. the problem can be reproduced in master. Ill add it to the same ticket you created.

originalfoo · 2022-03-24T23:17:11Z

@krzychu124 looks like CI server borked - can you prod it to rerun tests?

kianzarrin · 2022-03-27T23:05:04Z

👍

krzychu124 added 3 commits March 15, 2022 21:51

Changed avg speed limit calculation from per lane direction to suppor…

e2c81e9

…ted lane vehicle types

implemented IEquatable - eliminate boxing when checking SpeedValue eq…

c322a5d

…uality

removed no longer used method

4bf04ee

krzychu124 added BUG Defect detected SPEED LIMITS Feature: Speed limits regression old bug comes back or a new bug introduced in code that used to work. labels Mar 15, 2022

krzychu124 requested review from kvakvs, originalfoo and kianzarrin March 15, 2022 20:59

krzychu124 self-assigned this Mar 15, 2022

kvakvs approved these changes Mar 15, 2022

View reviewed changes

TLM/TMPE.API/Traffic/Data/SpeedValue.cs Show resolved Hide resolved

TLM/TMPE.API/Traffic/Data/SpeedValue.cs Outdated Show resolved Hide resolved

originalfoo reviewed Mar 16, 2022

View reviewed changes

TLM/TLM/UI/SubTools/SpeedLimits/Overlay/OverlaySegmentSpeedlimitHandle.cs Outdated Show resolved Hide resolved

originalfoo added 4 commits March 16, 2022 12:18

SetSegmentSpeedLimit can take finalDir:null

4c4ad42

When `null`, the speed will be applied to all lanes _except_ those with `None` direction.

Overloaded method now uses finalDir of null

3868782

Removes loop of directions.

Improve naming of bool

86b5cf5

Update OverlaySegmentSpeedlimitHandle.cs

2d6ffb6

This is simplified to take advantage of the updates to `SpeedLimitManager.Instance.SetSegmentSpeedLimit()` in which lane direction specification is no longer mandatory. It wipes out several loops, particularly over segment lanes.

krzychu124 commented Mar 16, 2022

View reviewed changes

TLM/TLM/Manager/Impl/SpeedLimitManager.cs Outdated Show resolved Hide resolved

Fix + simplify direction check

e65f7b6

originalfoo added this to the 11.6.5.2 milestone Mar 17, 2022

originalfoo self-requested a review March 17, 2022 05:30

originalfoo mentioned this pull request Mar 21, 2022

What's New in TM:PE 11.6.5.2 #1480

Merged

originalfoo added 3 commits March 23, 2022 16:43

Check for applicable lane, and some minor cleanup

bf5b2af

Merge branch 'master' into bugfix/1337-train_tracks_speed_limits_tool

a5b6245

Turn off SA1508

624b776

Completely useless warning for: "A closing brace within a C# element, statement, or expression is preceded by a blank line."

AnyApplicableLane segment extension

7234d43

Useful for checking if a segment has at least one matching lane for `LANE_TYPES` and `VEHICLE_TYPES`.

kianzarrin reviewed Mar 24, 2022

View reviewed changes

TLM/TLM/Util/Extensions/NetSegmentExtensions.cs Show resolved Hide resolved

kianzarrin reviewed Mar 24, 2022

View reviewed changes

kianzarrin requested changes Mar 24, 2022

View reviewed changes

originalfoo added 4 commits March 24, 2022 14:56

Remove superfluous finalDir param

8d37fc0

It's a relic from older incarnation of speed limit tool.

Add missing default to switch

a4be76d

Lanes with no direction must not be customised

0470b50

Assert flag params aren't None

bbbe358

`AnyApplicableLane()` will never return `true` if either of the relevant params are `None`.

originalfoo mentioned this pull request Mar 24, 2022

Make more use of LaneUtil, etc. #1489

Open

kianzarrin approved these changes Mar 24, 2022

View reviewed changes

kianzarrin requested changes Mar 24, 2022

View reviewed changes

kianzarrin approved these changes Mar 24, 2022

View reviewed changes

originalfoo approved these changes Mar 24, 2022

View reviewed changes

krzychu124 added 2 commits March 27, 2022 23:29

Merge branch 'master' into bugfix/1337-train_tracks_speed_limits_tool

1e5447f

Polishing the code

648a65a

krzychu124 merged commit 2519838 into master Mar 27, 2022

krzychu124 deleted the bugfix/1337-train_tracks_speed_limits_tool branch March 27, 2022 23:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Train tracks segment speed limits #1467

Train tracks segment speed limits #1467

krzychu124 commented Mar 15, 2022

originalfoo left a comment

kianzarrin commented Mar 23, 2022

krzychu124 commented Mar 23, 2022

kvakvs commented Mar 23, 2022

krzychu124 commented Mar 23, 2022

kianzarrin commented Mar 23, 2022

originalfoo commented Mar 23, 2022

kianzarrin Mar 24, 2022

originalfoo Mar 24, 2022

kianzarrin Mar 24, 2022

kianzarrin Mar 24, 2022

originalfoo Mar 24, 2022

kianzarrin Mar 24, 2022

kianzarrin Mar 24, 2022

originalfoo Mar 24, 2022

kianzarrin commented Mar 24, 2022

kianzarrin commented Mar 24, 2022

kianzarrin left a comment

originalfoo commented Mar 24, 2022

kianzarrin commented Mar 24, 2022

originalfoo commented Mar 24, 2022

kianzarrin commented Mar 27, 2022

Train tracks segment speed limits #1467

Train tracks segment speed limits #1467

Conversation

krzychu124 commented Mar 15, 2022

originalfoo left a comment

Choose a reason for hiding this comment

kianzarrin commented Mar 23, 2022

krzychu124 commented Mar 23, 2022

kvakvs commented Mar 23, 2022

krzychu124 commented Mar 23, 2022

kianzarrin commented Mar 23, 2022

originalfoo commented Mar 23, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kianzarrin commented Mar 24, 2022

kianzarrin commented Mar 24, 2022

kianzarrin left a comment

Choose a reason for hiding this comment

originalfoo commented Mar 24, 2022

kianzarrin commented Mar 24, 2022

originalfoo commented Mar 24, 2022

kianzarrin commented Mar 27, 2022