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

Tool overlay render optimisations #521

Merged
merged 5 commits into from
Sep 10, 2019

Conversation

kvakvs
Copy link
Collaborator

@kvakvs kvakvs commented Sep 7, 2019

Closes #520

On my machine and my city saves about 12-25 ms (50-60ms jumps to 75-85 ms) per frame as long as the camera remains stationary.

  • Add two new utility classes for storing an array of something (node ids or segment ids) and for storing camera position and rotation
  • Use the new functionality in
    • Lane Connector tool
    • Speed Limits tool
    • Parking Restrictions tool

Not used in Priority Signs tool and in Vehicle Restrictions tool because they have own different caching mechanism.

@originalfoo originalfoo added code cleanup Refactor code, remove old code, improve maintainability LANE ROUTING Feature: Lane arrows / connectors PARKING Feature: Parking AI / restrictions / etc performance Make it faster! SPEED LIMITS Feature: Speed limits technical Tasks that need to be performed in order to improve quality and maintainability labels Sep 7, 2019
@originalfoo originalfoo added this to the 11.0 milestone Sep 7, 2019
@originalfoo
Copy link
Member

originalfoo commented Sep 7, 2019

Tested:

  • Speed limits - drops 3fps while camera is static with lots of roads on screen (before was about 6fps)
  • Lane connector - drops 1fps while camera is static (before was about 3fps)
  • Parking restrictions - note to self: Don't update graphics drivers in the middle of fps testing

@originalfoo
Copy link
Member

Retesting with new graphics drivers and saved camera positions (ensure same scene complexity)...

Before optimisations:

  • Speed limits: 46 fps
  • Lane connector: 29-37 fps
  • Parking restrictions: 43 fps

After optimisations:

  • Speed limits: 46 fps
  • Lane connector: 46-47 fps
  • Parking restrictions: 44 fps

So lane connector is certainly a lot faster.

@kvakvs
Copy link
Collaborator Author

kvakvs commented Sep 7, 2019

The two others did already have some similar optimisation in place, so i just changed the code to use the same code in all 3. There should not be observed speed up in those others but good to know it is stable and so far is safe.

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 👍

@originalfoo originalfoo added Overlays Overlays, data vis, etc. UI User interface updates labels Sep 7, 2019
@krzychu124
Copy link
Member

Testing...

TLM/TLM/UI/SubTools/LaneConnectorTool.cs Show resolved Hide resolved
Ray mouseRay = currentCamera.ScreenPointToRay(Input.mousePosition);

// Check if camera pos/angle has changed then re-filter the visible nodes
// Assumption: The states checked in this loop don't change while the tool is active
Copy link
Member

Choose a reason for hiding this comment

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

What about overlay? Tool doesn't have to be enabled to render its overlay (viewOnly).
I agree that for now this kind of optimization is OK, but we need something better in the future.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This might potentially cache too much, if the user has overlay on and is editing roads. But moving the camera will have it fixed right away. Also isn't this always view overlay for debug only?

Copy link
Member

Choose a reason for hiding this comment

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

You can't have overlay on and edit/build road ;)

Btw I've noticed that overlay is not rendered when you deselect and select tool without moving camera afterwards

Copy link
Member

Choose a reason for hiding this comment

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

Btw I've noticed that overlay is not rendered when you deselect and select tool without moving camera afterwards

I was unable to reproduce this but will do some more testing.

Copy link
Member

Choose a reason for hiding this comment

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

Speed limits tool only

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Added cache reset when the tool is activated, in all 3 affected tools. Testing...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The overlays should be rendered by other code than the tool itself. I.e. having the tool code called when the tool is not active is terrible mistake and the render overlay and caching code should be moved outside. Now I also observe that when Overlay is on and Speed limits tool is on and off, the speed limits disappear until the camera is moved.

Copy link
Member

Choose a reason for hiding this comment

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

I've now been able to replicate the issue with speed limits.

Note to self; Camera moves when it's over water. lol

@krzychu124
Copy link
Member

Conclusions:

  • if I don't move camera it's fine. ✔️
    Almost no frame rate change when camera is not moving, but horrible lags when it does (sometimes drop from >30 to ~15-18)...

@originalfoo
Copy link
Member

originalfoo commented Sep 7, 2019

I think it would still be worth pushing this update as is, it makes lane connector much faster while camera is still and that's still useful for users.

For overall improvement, especially when camera moves, we need that proper caching manager we discussed some time ago (see #415 and possibly duplicate #517).

@krzychu124
Copy link
Member

krzychu124 commented Sep 7, 2019

I've made one more test because I was curious what is the exact difference in performance of ShowOverlay() between stationary camera and constantly moving with tool enabled(Speed limits).

My potato computer generated following result of benchmark:
Stationary camera: 0.4-0.64 ms/call
Camera rotating around intersection: 5.1-6.9 ms/call

@kvakvs
Copy link
Collaborator Author

kvakvs commented Sep 8, 2019

After the yesterday's discussion i reverted my uncommitted changes.
What should be fixed here, or deliver it as is?

@originalfoo
Copy link
Member

originalfoo commented Sep 8, 2019

Everything in this PR is working fine, except the speed limits don't show on second usage if camera did not change from prior usage.

EDIT: Did you push the commits that clear the cache when the speed tool is activated?

@kvakvs
Copy link
Collaborator Author

kvakvs commented Sep 8, 2019

Fixed the speed limits by resetting the cached camera on tool activation.
This still will not show overlay until the camera moves but that is not a deal breaker and i hope that is something we can address separately when changing each tool.

@kvakvs kvakvs merged commit 39bd499 into CitiesSkylinesMods:master Sep 10, 2019
@kvakvs kvakvs mentioned this pull request Feb 3, 2020
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 Overlays Overlays, data vis, etc. PARKING Feature: Parking AI / restrictions / etc performance Make it faster! SPEED LIMITS Feature: Speed limits technical Tasks that need to be performed in order to improve quality and maintainability UI User interface updates
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Task] Low hanging UI performance fruits
3 participants