Parking Restrictions tool - Lane highlighting

[kianzarrin]

fixes #702

parking lanes are highlighted:

• green=allowed red=not allowed.
• shift => blue color for all affected segments. (see issue for pictures)
• click => highlight is emboldened and turns orange (visual feedback)
• oneway roads have 2 parking lanes affected by the same GUI, I highlight both lanes here.

Scope:
The scope of this review is not to change the functionality of the parking tool, but rather provide visual feedback for what it already does.

Crowdin: I did not introduce the shift functionality but since it does not show in the tooltip keybind I decided to add it. Can anyone add the following crowdin key:

FILE: Menu.csv
"Tooltip.Keybinds:Parking restrictions"=>"[shift] apply changes to the whole road."

EDIT: fixed overlapping sprite overlays. see #708 (comment)

[kvakvs]

Do not rely on green/red/blue/orange, because there exist color blind people.
Of course you can still use color, but make green brighter, and red darker, and that should work well.

[kianzarrin]

Do not rely on green/red/blue/orange, because there exist color blind people.
Of course you can still use color, but make green brighter, and red darker, and that should work well.

The orange color is also embolden. blue color is supposed to be natural so it does not matter. as for green VS red I am not relying on it but rather its just a nice touch. Note that there is GUI sprite that shows the person the state of parking restriction.

[originalfoo]

The primary purpose of the lane highlights is to illustrate what will be affected. The colours are just embelishments. The parking icons remain as the primary explanation of state, with the bar across the icon indicating in non-colour terms when parking is disabled (again, the fact that the bar is red is just an embelishment).

EDIT: At some point, once the highlight features are all centralised, it would be good to provide mechanisms that extend embelishments to the full range of end users (colour is fine for roughly 85%-90% of users depending on which statistics are referred to). We can provide mod option that allows selection of colour scheme for various types of vision.

[originalfoo]

I might be doing something wrong, but I'm not seeing lane highligting of parking lanes when hovering (or clicking) lanes or icons:

Edit: To double check I manually deleted the dlls in the local mod folder, switched branches a couple of times on git (and back to 708) then did clean rebuild. Still no highlights.

[originalfoo]

I suspect the issue is to do with direction inversion happening in LHT maps as I just discovered in #709.

So the highlight is possibly working, just in the wrong direction or something.

[kianzarrin]

fixed LHT direction problem by using m_finalDirection.

[kvakvs]

Code's lookin' good.
Just minor comments on field naming style as discussed in the chat.
Private camelCase_, other fields, structs, classes and functions CamelCase, functions where possible to begin with a verb.

[originalfoo]

It's working a bit now, but still have some issues (testing with LHT city).

Link to road assets in workshop

👍 This one-lane one-way road with parking is working properly - and both parking lanes highlighted which is good!

⚠️ Two-lane two-way with parking isn't working - EDIT: no lane highlights:

However, from the same roads pack linked above the surburban parking road works fine:

⚠️ Also from the same road pack, this asymmetric road with parking isn't working - EDIT: no lane highlights:

👍 Again, from same roads pack, this two-lane one-way with parking works properly - EDIT: lanes are highlighted:

Maybe there are issues with the road assets? If so, it would be good to log them to TMPE.log when game loads if possible (so asset authors can get some idea what they did wrong). Otherwise it might be bugs in PR.

[originalfoo]

Found some issues - see comment above.

[kianzarrin]

@aubergine10
What do you mean by not working?
I did some testing with 2L2W road with master branch TMPE. banning parking on one side does not do anything while banning parking on the other side bans both sides. shift does not work.

Something weird going on. Not my fault but I will try to see If I can do something quick to help as part of this review ...

[kianzarrin]

Also annoying that the parking sprites overlap and you can click both of them at the same time:

[originalfoo]

I don't think sprite overlap trick works for parking (I use it all the time on speed limits, but never had need to use it for parking so not sure).

The issues I reported above are about lane highlights however; sometimes the lane highlight isn't showing regardless of which sprite (or even overlap) is hovered.

EDIT: I just realised all my screenshots had overlapping sprites; that's purely coincidental. The primary issue was the lack of lane highlights. That being said, if hovering the overlap would show lane highlights for both sprites, and clicking overlap toggle both sets of parking lanes, that would be good for consistency with speed limtis tool.

[kianzarrin]

The direction of the parking is both. That is why TMPE gets confused. see #516 (comment).

The issues I reported above are about lane highlights however; sometimes the lane highlight isn't showing regardless of which sprite (or even overlap) is hovered.

These roads are not supported by TMPE so the highlight behavior is undefined. I will print error message and #516 will handle the rest. we have to change the code fundamentally as part of #516 so its not worth dealing with direction=both right now. if anything it is a good feedback to the user that its not working!

[originalfoo]

Is direction=both an error on part of asset author though? For example, I suspect that would also be cause of #366 which I noticed some time ago (I think, with those same roads!). I know the asset author so I can ask him to change the setting, but what to?

[kianzarrin]

@kvakvs lets be easy about the camelCase_ VS camelCase in this review. next time OK?

[originalfoo]

Discussion about direction=both on parking lanes moved to: #516

[originalfoo]

LGTM 👍

Ideally we should decide what to do with direction=both (see #516) before lane highlighting goes out to end-users. But regardless, this PR #708 works as advertised.

[kvakvs]

The underscores in names were looking alright :D

[originalfoo]

just noticed another commit; do you want me to retest?

[kianzarrin]

Wait! I fixed the overlapping sprite problem.
EDIT:

[kianzarrin]

Also added error message for bad lanes:

#if DEBUG
            if(DebugSwitch.BasicParkingAILog.Get() &&
                dir != NetInfo.Direction.Forward &&
                dir != NetInfo.Direction.Backward) {
                Log.Error($"bad direction: {dir} expected forward or backward");
            }
#endif

added wiki information here : https://github.com/CitiesSkylinesMods/TMPE/wiki/Notes-for-Asset-Creators#parking-lanes . when #516 is fixed this may no longer be required.

Test result:

Error 1,682.2137200: parking lane with bad m_finalDirection:Both, expected forward or backward
   at CSUtil.Commons.Log.LogToFile(System.String log, LogLevel level)
   at CSUtil.Commons.Log.Error(System.String s)
   at TrafficManager.Manager.Impl.ParkingRestrictionsManager.SetParkingAllowed(UInt16 segmentId, Direction finalDir, Boolean flag)
   at TrafficManager.Manager.Impl.ParkingRestrictionsManager.ToggleParkingAllowed(UInt16 segmentId, Direction finalDir)
   at TrafficManager.UI.SubTools.ParkingRestrictionsTool.DrawParkingRestrictionHandles(UInt16 segmentId, Boolean clicked, NetSegment ByRef segment, Boolean viewOnly, Vector3 ByRef camPos)
   at TrafficManager.UI.SubTools.ParkingRestrictionsTool.ShowSigns(Boolean viewOnly)
   at TrafficManager.UI.SubTools.ParkingRestrictionsTool.ShowGUIOverlay(ToolMode toolMode, Boolean viewOnly)
   at TrafficManager.UI.TrafficManagerTool.OnToolGUI(UnityEngine.Event e)
   at ToolBase.OnGUI()

[kianzarrin]

@aubergine10 I have modified the code since you reviewed it last. I prevented the sprites of speed limit tool and parking tool from overlapping. Also added debug error for bad parking lanes.

[originalfoo]

I prevented the sprites of speed limit tool and parking tool from overlapping

Now I can't change both directions with one click any more. :(

[kianzarrin]

@aubergine10 Now I can't change both directions with one click any more. :(

What you wanted them to overlap?! are you teasing me? lol

don't loos hope if you give camera descent angle you can still change both with one click lol! before you could change them both but you could not use the shift properly.

[originalfoo]

It's only a minor thing, I can live without it. I think it will probably be better for most users to have them more separated like you have them in this PR, the main thing here is that the lanes are highlighted so IMO this PR is good to merge.