Skip to content

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

Merged
merged 7 commits into from
Apr 30, 2021

Conversation

RSlysz
Copy link
Contributor

@RSlysz RSlysz commented Apr 28, 2021

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
image
image

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

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.

@RSlysz RSlysz marked this pull request as ready for review April 29, 2021 19:33
@RSlysz RSlysz requested review from TomasKiniulis and jenniferd-unity and removed request for sebastienlagarde April 29, 2021 19:34
Copy link
Contributor

@arttu-peltonen arttu-peltonen left a 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);
Copy link
Contributor

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?

Copy link
Contributor Author

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.

Copy link
Contributor Author

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

@sebastienlagarde sebastienlagarde merged commit c11b7b8 into master Apr 30, 2021
@sebastienlagarde sebastienlagarde deleted the hd/redo-fix-inputregistering-domainreload branch April 30, 2021 17:02
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants