-
Notifications
You must be signed in to change notification settings - Fork 844
Hd/redo fix inputregistering domainreload #4358
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
Hd/redo fix inputregistering domainreload #4358
Conversation
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 don't totally get this stuff, and I don't know what's expensive and why, but I can't see issues in the loop logic, and seems like this has been tested, so LGTM :)
spAxes.arraySize = endOfCurrentInputList + newEntries.Count; | ||
|
||
SerializedProperty spAxis = spAxes.GetArrayElementAtIndex(endOfCurrentInputList - 1); | ||
spAxis.Next(false); |
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.
are we doing this because spAxes.GetArrayElementAtIndex(endOfCurrentInputList) would not be valid?
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.
Exact. That was the issue in the initial PR.
But as we changed the size before it should be valide.
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.
Also previous version was doing it and it was not raising issue. So I am not 100% sure why it complained
* Update InputRegistering.cs * Remove asset modification if InputSystem package is in use * Prevent missuse of InputRegistering while InputSystem package is in use * cleaning * more cleaning * Update code to use Next instead of GetArrayElementAtIndex * Fix strange out of range error
* Update InputRegistering.cs * Remove asset modification if InputSystem package is in use * Prevent missuse of InputRegistering while InputSystem package is in use * cleaning * more cleaning * Update code to use Next instead of GetArrayElementAtIndex * Fix strange out of range error
* [HDRP] Fp16_PPAlpha Panini projection, Motion Blur test and moved other Fp16Alpha scenes to one #4290 * Hd/redo fix inputregistering domainreload (#4358) * Update InputRegistering.cs * Remove asset modification if InputSystem package is in use * Prevent missuse of InputRegistering while InputSystem package is in use * cleaning * more cleaning * Update code to use Next instead of GetArrayElementAtIndex * Fix strange out of range error Co-authored-by: TomasKiniulis <50582134+TomasKiniulis@users.noreply.github.com> Co-authored-by: Remi Slysz <40034005+RSlysz@users.noreply.github.com>
* [10.x.x HDRP] Fix issue with raytracing resources not properly initialize (#4268) * draft * Fix build DXR * Match behavior with the one in master * Updated ray tracing settings and added it to the table of contents (#4279) * Updated ray tracing settings and added it to the toc * Removed hyphens from rtss toc entry * Fix shader warning when using Lanczos upsampling (#4280) * Added DOTS_INSTANCING_ON variants to the "HDRP/Decal" shader #4284 * [HDRP] Fp16_PPAlpha Panini projection, Motion Blur test and moved other Fp16Alpha scenes to one #4290 * [HDRP] Fix undo of some properties on light editor #4293 * Opt-out of builtin auto baking of ambient/reflection probe. #4324 * Revert "Opt-out of builtin auto baking of ambient/reflection probe. #4324" This reverts commit 719e835. * Hd/redo fix inputregistering domainreload (#4358) * Update InputRegistering.cs * Remove asset modification if InputSystem package is in use * Prevent missuse of InputRegistering while InputSystem package is in use * cleaning * more cleaning * Update code to use Next instead of GetArrayElementAtIndex * Fix strange out of range error Co-authored-by: Lewis Jordan <lewis.jordan@hotmail.co.uk> Co-authored-by: Pavlos Mavridis <pavlos.mavridis@unity3d.com> Co-authored-by: Andrew Saraev <67020478+AndrewSaraevUnity@users.noreply.github.com> Co-authored-by: TomasKiniulis <50582134+TomasKiniulis@users.noreply.github.com> Co-authored-by: Adrien de Tocqueville <adrien.tocqueville@unity3d.com> Co-authored-by: JulienIgnace-Unity <julien@unity3d.com> Co-authored-by: Remi Slysz <40034005+RSlysz@users.noreply.github.com>
This is a redo of PR #3373
Prior PR was tested and validated by QA. Then we got an info from Serialization team that next should use even less resource. Changing it, it let appear a really strange out of range issue. (see fix 3b68c97 )
Launched test locally and all is green except 1225_Lit_SpeedTree8SG:
Expected: less than or equal to 9.99999975E-05f
But was: 0.000778189569f
This seams unrelated with change in this PR.
Purpose of this PR
Improve performance for DebugMenu input registering system at each domain reload. FB: https://fogbugz.unity3d.com/f/cases/1311108/
I found that there is way more call to serialization (which is the bottle neck here) than needed. So I reworked the checking algorithm to use the less possible serialization. Checking was here to be sure we do not register duplicate entry. Now it is done once on all the cached entry instead of one time per new entry.
So basically we previously had a complexity of roughly O(n.m) where n was the pre existing entry amount and m the inserted entry amount. And with this change we are in O(n+k) where k is the entry that we can safely insert without creating duplicate (k < m, often 0). (Complexity is computed against serialization call to get element in the array)
Also I delayed the actual register to next editor frame. This in order to let a chance to pack in case of several class request to register an input at initialization. The costly operation of the new algorithm is the caching of pre existing entries. If we can afford to prevent being done several time, it worth it.
And finally, the InputSystem package is supported by the DebugMenu but in this case, it still try to add inputs in the asset used by the legacy system. I skipped this operation as it is not needed.
In addition, I added warnings when using InputRegistering while the InputSystem package is in use.
Testing status
In test project given with the FB above, there is 529 entries in ProjectSettings > InputManager
Before fix


After fix (without delaying next frame to have meaningful results) without InputSystem package


Tested it is totally skipped when InputSystem is in use. Tested with the project from FB https://fogbugz.unity3d.com/f/cases/1306751/ which use a lot more entry in the input asset. Times goes to ~1min (2754 entries) to almost nothing.
And DebugMenu still working after change in both project (with and without the InputSystem package).
Note: tests have been done before changing from using GetArrayElementAtIndex to Next wich use even less computation time. So expect even better results.
Comments to reviewers
Note: the perfect fix should be to have a C# API for this. Now that there is a dedicated package for this, I dunno if any development on the subject is something to think of or not.