Hd/redo fix inputregistering domainreload #4358
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
RSlysz
requested review from
TomasKiniulis and
jenniferd-unity
and removed request for
sebastienlagarde
JulienIgnace-Unity
approved these changes
Apr 30, 2021
arttu-peltonen
approved these changes
Apr 30, 2021
jenniferd-unity
approved these changes
Apr 30, 2021
| spAxes.arraySize = endOfCurrentInputList + newEntries.Count; | ||
|
|
||
| SerializedProperty spAxis = spAxes.GetArrayElementAtIndex(endOfCurrentInputList - 1); | ||
| spAxis.Next(false); |
There was a problem hiding this comment.
are we doing this because spAxes.GetArrayElementAtIndex(endOfCurrentInputList) would not be valid?
There was a problem hiding this comment.
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.
Also previous version was doing it and it was not raising issue. So I am not 100% sure why it complained
sebastienlagarde
pushed a commit
that referenced
this pull request
Apr 30, 2021
* 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
sebastienlagarde
pushed a commit
that referenced
this pull request
Apr 30, 2021
* 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
sebastienlagarde
added a commit
that referenced
this pull request
May 1, 2021
* [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>
sebastienlagarde
added a commit
that referenced
this pull request
May 1, 2021
* [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>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.