-
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
Tool overlay render optimisations #521
Tool overlay render optimisations #521
Conversation
Tested:
|
Retesting with new graphics drivers and saved camera positions (ensure same scene complexity)... Before optimisations:
After optimisations:
So lane connector is certainly a lot faster. |
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. |
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 👍
Testing... |
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 |
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.
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.
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.
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?
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.
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
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.
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.
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.
Speed limits tool only
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.
Added cache reset when the tool is activated, in all 3 affected tools. Testing...
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 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.
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.
I've now been able to replicate the issue with speed limits.
Note to self; Camera moves when it's over water. lol
Conclusions:
|
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). |
I've made one more test because I was curious what is the exact difference in performance of My potato computer generated following result of benchmark: |
After the yesterday's discussion i reverted my uncommitted changes. |
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? |
Fixed the speed limits by resetting the cached camera on tool activation. |
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.
Not used in Priority Signs tool and in Vehicle Restrictions tool because they have own different caching mechanism.