-
Notifications
You must be signed in to change notification settings - Fork 862
Hd/fix input registering domain reload #3373
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/fix input registering domain reload #3373
Conversation
| s_PendingInputsToRegister.AddRange(entries); | ||
|
|
||
| //delay the call in order to do only one pass event if several different class register inputs | ||
| EditorApplication.delayCall += DelayedRegisterInput; |
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 line is replaced by DelayedRegisterInput(); in the test to compare algorithm in same conditions.
| public static void RegisterInputs(List<InputManagerEntry> entries) | ||
| static void AddEntriesWithoutCheck(SerializedProperty spAxes, List<InputManagerEntry> newEntries) | ||
| { | ||
| int startRange = spAxes.arraySize; |
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.
note for next time: i got confused because expected a range here. so could you confirm startRange is basically firstEntryIndex and endRange is lastEntryIndex?
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.
Yes I was refering it at range like in the list.AddRange(...) method.
So startRange = firstEntryPositionInInputListIndex and endRange = lastEntryPositionInInputListIndex
Just first/lastEntryIndex is unclear if it is in the newEntries list or in the spAxes array. So I'll keep range and add a comment.
| s_PendingInputsToRegister.AddRange(entries); | ||
|
|
||
| //delay the call in order to do only one pass event if several different class register inputs | ||
| EditorApplication.delayCall += DelayedRegisterInput; |
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.
does this mean we could end up calling this function multiple times in the same frame?
also when do we unregister the call?
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.
No it means you can call it multiple time in a frame, it will all be batched in delayed call.
Example:
Frame1, two different classes call register input
RegisterInputs(new List<InputManagerEntry>
{
new InputManagerEntry { name = "cmd1", kind = InputManagerEntry.Kind.KeyOrButton, btnPositive = "left ctrl", altBtnPositive = "joystick button 8" },
new InputManagerEntry { name = "cmd2", kind = InputManagerEntry.Kind.KeyOrButton, btnPositive = "backspace", altBtnPositive = "joystick button 9" }
});
RegisterInputs(new List<InputManagerEntry>
{
new InputManagerEntry { name = "cmd3", kind = InputManagerEntry.Kind.KeyOrButton, btnPositive = "left alt", altBtnPositive = "joystick button 1" }
});
Delayed calls:
- register cmd1, cmd2 and cmd3 at the same time
- nothing to register, so exit early
So we inspected the already registered inputs only once instead of two time when done at same frame.
And for unregistering, the mechanism where not here prior and was never requested. I assume it is not needed in this case. But it can be implemented in a similar way. I assume this was done to add inputs in the InputManager.asset only and not remove them.
|
I have seen Martin comment on the thread of the bug that suggest to use Next instead of GetArrayElementAtIndex for better performances. I'll amend this PR with this. |
TomasKiniulis
left a comment
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.
|
@RSlysz : There is a problem with this PR, I have reverted it from hd/bugfix. |


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.