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.

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

LaneArrows facelift and refactor #726

Merged
merged 24 commits into from
Apr 7, 2020

Conversation

kvakvs
Copy link
Collaborator

@kvakvs kvakvs commented Feb 22, 2020

Fixes #571

New Features

New API for building forms: U
You can build forms with nested code structure with using (var x = UIBuilder...) {} API. Each form component can have an init function, and an optional resize function (which will be called when UI needs resizing) and can reposition and resize each control. The controls can be stacked to previous sibling's right, or below, with spacing if you wish. And controls can have a padding.
New UI forms take notifications from the Options UI scale slider and scale accordingly.

bild
(note: The keybinds feature is commented out and will be reintroduced later)

bild

TODO

  • Darker shades of blue for active
  • Darker shades of grey for hover
  • Lower the left and right arrow images a little (maybe)
  • Check "Delete" works
  • UI Scaling reduce to 150% at 4k resolution (current: 200%)
  • Allow UI Scale slider range from 50% (current 65%)
  • Position window correctly upon creation
  • Allow selecting other segments in the tool window state.
  • Remove keybinds panel for this merge, and reintroduce in a separate PR.
  • When screen resolution changes, the window is misaligned with the node on screen.

@kvakvs kvakvs self-assigned this Feb 22, 2020
@originalfoo originalfoo added code cleanup Refactor code, remove old code, improve maintainability technical Tasks that need to be performed in order to improve quality and maintainability UI User interface updates Usability Make mod easier to use LANE ROUTING Feature: Lane arrows / connectors labels Feb 24, 2020
@originalfoo originalfoo added this to the 11.2 milestone Feb 26, 2020
@kvakvs
Copy link
Collaborator Author

kvakvs commented Feb 27, 2020

Testing and debugging at the moment.
Funny enough, having tool window for Lane Arrows as a child of TMPE Main Menu creates rubber banding drag effect on the tool window while dragging the main menu. But other than that the window is always positioned at the exact center and looks alright.

bild

@originalfoo
Copy link
Member

This is looking awesome! Do you think we can get this in to the 11.1.1 release?

@originalfoo
Copy link
Member

Also, can we add a reset button?

@kvakvs kvakvs force-pushed the new-lane-arrows-ui branch from d4f5029 to ddbeb45 Compare March 22, 2020 03:49
@originalfoo originalfoo modified the milestones: 11.2, 11.3 Mar 28, 2020
@kvakvs kvakvs force-pushed the new-lane-arrows-ui branch from 21d076c to ba53a07 Compare March 29, 2020 12:29
@kvakvs kvakvs marked this pull request as ready for review March 29, 2020 23:27
@kvakvs kvakvs requested review from kianzarrin, krzychu124 and originalfoo and removed request for kianzarrin and krzychu124 March 29, 2020 23:36
@kvakvs
Copy link
Collaborator Author

kvakvs commented Mar 30, 2020

Important
This needs to be tested at 4k resolution and possibly we need a formula to reduce the scaling a bit, so it is not very large compared to the rest of the game UI.

@originalfoo
Copy link
Member

The UI momentarily appears small in middle of screen then pops in to position at chosen user scaling. See attached vid:

bandicam 2020-03-31 22-28-14-496.zip

Maybe have the UI hidden until it's possitioning/sizing is finalised?


With a segment selected, it feels a bit clunky selecting a different segment. Could we make it so I can just click any other junction-entering segment to select it (even if another segment is already selected). Right-click should still de-select current segment as it currently does.


The UI still feels a bit big to me, is it possible to allow the scale slider in mod options to go down to 50%? (Currently it's min value is 65%)


The tooltip is making the floating panel unnecessarily large when there's less than 3 lanes:

image

Could it word wrap? Or maybe an option to toggle it's display (once I know the shortcut I don't really want to be seeing it every time)?


The positioning seems to get offset when screen res is changed in-game. For example, on 1920x1080 (everything above was tested at this res), the panel is shown in centre of junction like so:

image

When I change to 2560x1440 it gets offset like this:

image


Note: everything above was using 65% UI scaling in mod options. I just did a quick test at 100% and the same issues occur.

Copy link
Member

@originalfoo originalfoo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

see comment above

@kvakvs
Copy link
Collaborator Author

kvakvs commented Apr 4, 2020

This now looks like its scaling correctly, and positioning correctly too.

@krzychu124
Copy link
Member

I'm getting exception when I right click with arrows tool enabled

NullReferenceException: Object reference not set to an instance of an object
  at TrafficManager.UI.SubTools.LaneArrows.LaneArrowTool.OnToolRightClick () [0x00000] in <filename unknown>:0 
  at TrafficManager.UI.TrafficManagerTool.OnToolUpdate () [0x00000] in <filename unknown>:0 
  at ToolBase.Update () [0x00000] in <filename unknown>:0 

to be precise it breaks at this line:

Log._Debug(string.Format("LaneArrow right click state={0}, new={1}", state, this.fsm_.State));

@originalfoo
Copy link
Member

It's allowing seemingly non-valid arrows to be selected:

image

I would expect the left turn button there to be disabled.


Text doesn't seem to be scaling with the UI?

image


Other than those two very minor details it's working great! Hope you do Speed Limits palette next :)

@kvakvs
Copy link
Collaborator Author

kvakvs commented Apr 5, 2020

  • Font scaling fixed, now font scales to ridiculous small or big size too.
  • Lane-arrows-window hopping on creation fixed.
  • Clicking other segments was showing old Lane-arrows-window, now fixed, the state is correctly exited and re-entered and the window is updated now too.
  • @krzychu124 there's a null check in that code, i am confused how can it happen. I removed log messages they are not necessary anyway. Maybe sending right-click trigger to exit Lane Arrows editor would set fsm_ to null, in that case i added an additional null check after the trigger.

As for extra rendering of direction arcs and disabling the buttons, i am not sure the old UI had this feature either @aubergine10 https://github.com/CitiesSkylinesMods/TMPE/blob/master/TLM/TLM/UI/SubTools/LaneArrowTool.cs#L311-L348

@kvakvs
Copy link
Collaborator Author

kvakvs commented Apr 6, 2020

@krzychu124 The fix for that rare null exception is now in the next branch i'm working on. Should i backport it to this branch or go like this? The next branch is ready very soon.

@krzychu124
Copy link
Member

@kvakvs you can leave it there.
I think we will push both your PR's and Kian's (i have to test it) as one update to the Labs

@kvakvs kvakvs added the DO NOT MERGE YET Don't merge this PR, even if approved, until further notice label Apr 6, 2020
@kvakvs
Copy link
Collaborator Author

kvakvs commented Apr 6, 2020

Setting DONOTMERGE, as the next PR will come into this branch, and when all is good, they both can be merged.
kvakvs#3
Or instead merge this, then that PR will come on top of master nicely.

Copy link
Member

@krzychu124 krzychu124 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

Copy link
Member

@originalfoo originalfoo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

@kvakvs kvakvs removed the DO NOT MERGE YET Don't merge this PR, even if approved, until further notice label Apr 7, 2020
@kvakvs kvakvs merged commit 96ca33c into CitiesSkylinesMods:master Apr 7, 2020
@originalfoo originalfoo modified the milestones: Planned stuff, 11.3.0 Apr 7, 2020
krzychu124 pushed a commit that referenced this pull request Apr 7, 2020
* Old SubTool became LegacySubtool. New TrafficManagerSubtool is added for LaneArrows
* Fixed main tool & state machine behaviour. Tool right click in the initial state leaves the tool
* LaneArrows is using new UIBuilder class, button icons for lane arrows
* Lane Arrows window UI and sprites and clicks and position connected
* Texture loading fixed; Normal icons are now semitransparent inverted
* State machine for tool selection fixed and simplified
* Rebase on master
* Smart sizing and positioning (WIP)
* Recursive update OnResize for new U controls
* Smart sizing and positioning of controls
* Subscription to UI scaling events works; UI, stacking and margins work
* Keybind label builder helper in UIBuilder
* Recolored Lane Arrow buttons; Delete shortcut works now
* Removed obsolete code
* Removed keybinds panel; GUI scale range 50..200 now; Repositioning lane arrows window on creation
* Moved bunch of static geometry and road calculations into new static GeometryUtil class
* UIScaler takes screen size from UIView not from Screen object
* Commented on not using Screen.width|height and using UIView.fixedWidth|Height instead
* Scaling fixed: using UIView.GetComponent[Camera]().pixelWidth|Height
* Torn down the incorrect scaling code
* Text scaling fixed, window hopping fixed, clicking other segment correctly exits and reenters edit state
* Minor: Documentation and rename
* Ignore if left click is landing into LaneArrow editor Window
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
code cleanup Refactor code, remove old code, improve maintainability LANE ROUTING Feature: Lane arrows / connectors technical Tasks that need to be performed in order to improve quality and maintainability UI User interface updates Usability Make mod easier to use
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Lane arrow super-small on some resolutions
3 participants