-
Notifications
You must be signed in to change notification settings - Fork 85
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
Speed Limit tool - Lane highlighting #709
Speed Limit tool - Lane highlighting #709
Conversation
Interestingly, when I hold down Ctrl key for lane-mode, it was highlighting correct lanes. Seems the inversion issue is only affecting direction-mode. |
…lDirection instead of laneInfo.m_direction
I replaced m_direction with m_finalDirection. This is fixed now. I need to develop a test check list because I keep forgetting to test in all possible circumstances. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The amount of if (something)
conditions is getting out of hand.
Is not a blocker for this PR but there should be a way to reduce nesting and reduce the cyclomatic complexity (amount of code branching to keep in head while reading) for the future code.
@@ -9,6 +9,7 @@ internal class SegmentLaneMarker { | |||
} | |||
|
|||
internal Bezier3 Bezier; | |||
internal float size = 1.1f; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this a const? Can it change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it does change.
@@ -200,12 +312,15 @@ public SpeedLimitsTool(TrafficManagerTool mainTool) | |||
} | |||
|
|||
// no speed limit overlay on selected segment when in vehicle restrictions mode | |||
DrawSpeedLimitHandles( | |||
hover|= DrawSpeedLimitHandles( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing a space.
"cyclomatic complexity" ... interesting concept. how should I do this though? move branches into sub methods? Note that when the user holds shift, FPS drops because the code goes though a big ass slow for loop. as an optimization effort I created a separate code path for when shift is not pressed so that in most cases FPS is high. I haven't performed any benchmark though. I could do some caching too but the FPS seems fine on my slow computer so I didn't bother. |
If you later revisit this code, yes, please split logic into functions and reduce the nesting. |
There's another performance hit with speed limits -- and maybe other tools, haven't checked -- in that to facilitate user being able to 'drag a speed' along a series of segments... this: #388 (not relevant to this PR but something to consider for future performance tuning) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested in-game, LGTM from end-user perspective 👍
Need to do some more testing on this; found some issues with parking lane higlight in other PR and need to see if issues also apply to speed limits. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If two icons hovered at same time, please highlight all affected lanes not just lanes for one icon.
this is more like a bug (you view it as a trick). In any case I increased the distance of the sprites so they do not overlap any more. |
I'd suggest |
Do not bake keys into translation strings on Crowdin, just do |
@aubergine10 ! I renamed clear to to default. The screenshot are a commit too old.
Of course . if you look at the code you will notice its not part of the translation. |
@kianzarrin Did you get chance to update the direction-mode lane highlights? Everything else in this PR is good to go. |
I tested this on LHT with invert flag on and off and both directions of start/end nodes. |
For the corners, I prefer the gap rather than the weird overlap. Will test latest commits now... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍
fixes #682
highlight the affected lanes when user hovers speed limit sprite.
EDIT: I made major changes:
TrafficManagerTool
has a new method to render only one side of a segment see screenshotsShowPerLane
toggle works in the same way as "entire road" toggle but with CTRL key.EDIT: I forgot to mention I introduced a couple methods/properties to Lane/Sagement traverse data structures and put some commonly used code in there.
Test: