Skip to content
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.

Sign up for GitHub

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

Jump to bottom

NEW: ProfilerMarkers for measuring enabling/disabling of InputActions and for binding resolution. #2056

Merged
merged 9 commits into from
Dec 12, 2024
Merged

NEW: ProfilerMarkers for measuring enabling/disabling of InputActions and for binding resolution. #2056

merged 9 commits into from
Dec 12, 2024

Conversation

surfnerd
Copy link
Collaborator

@surfnerd surfnerd commented Nov 20, 2024 •
edited
Loading

Description

Exposing more timing data of the InputSystem by adding ProfilerMarkers to InputAction.Enable() and InputActionMap.ResolveBindings(). This is necessary to track frame times in the performance test project I am working on.

Testing status & QA

I have run test projects against this code in Unity 6.000.0.27f1 and 2019.4.40f1.

Overall Product Risks

The risk of this change is very low given that they only add profiler information.

  • Complexity: 0
  • Halo Effect: 0

Comments to reviewers

To continue being accountable for performance, I am adding more profiler markers to parts of the code that previously did not have them. These changes will be tracked in a separate performance test project.

Checklist

Before review:

  • Changelog entry added.
    • Explains the change in Changed, Fixed, Added sections.
    • For API change contains an example snippet and/or migration example.
    • JIRA ticket linked, example (case %%). If it is a private issue, just add the case ID without a link.
    • Jira port for the next release set as "Resolved".
  • Tests added/changed, if applicable.
    • Functional tests Area_CanDoX, Area_CanDoX_EvenIfYIsTheCase, Area_WhenIDoX_AndYHappens_ThisIsTheResult.
    • Performance tests.
    • Integration tests.
  • Docs for new/changed API's.
    • Xmldoc cross references are set correctly.
    • Added explanation how the API works.
    • Usage code examples added.
    • The manual is updated, if needed.

During merge:

  • Commit message for squash-merge is prefixed with one of the list:
    • NEW: ___.
    • FIX: ___.
    • DOCS: ___.
    • CHANGE: ___.
    • RELEASE: 1.1.0-preview.3.

After merge:

  • Create forward/backward port if needed. If you are blocked from creating a forward port now please add a task to ISX-1444.

@surfnerd surfnerd changed the title ADD: ProfilerMarkers for measuring enabling/disabling of InputActions and for binding resolution. NEW: ProfilerMarkers for measuring enabling/disabling of InputActions and for binding resolution. Nov 20, 2024
@surfnerd surfnerd requested a review from ekcoh November 20, 2024 23:08
@surfnerd surfnerd marked this pull request as ready for review November 20, 2024 23:08
@surfnerd surfnerd force-pushed the input/action-actionmap-markers branch from 1a8721d to 37e8453 Compare November 25, 2024 20:48
surfnerd
surfnerd commented Dec 4, 2024
View reviewed changes
Assets/Tests/InputSystem/Plugins/UITests.cs
@@ -1793,13 +1793,15 @@ public IEnumerator UI_CanReleaseAndPressTouchesOnSameFrame()
.Matches((UICallbackReceiver.Event e) => e.pointerData.pointerType == UIPointerType.Touch).And
.Matches((UICallbackReceiver.Event e) => e.pointerData.position == secondPosition));

#if UNITY_2021_2_OR_NEWER
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

EventType.PointerMove was wrapped in #if UNITY_2021_2_OR_NEWER in its declaration.

stefanunity
stefanunity approved these changes Dec 5, 2024
View reviewed changes
Copy link
Collaborator

@stefanunity stefanunity left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the help!

Assets/Tests/InputSystem/CorePerformanceTests.cs
"FixedUpdate.NewInputFixedUpdate"
"FixedUpdate.NewInputFixedUpdate",
"InputAction.Enable",
"InputActionMap.ResolveBindings"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please also add "InputAction.Disable" here, thanks

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added in 539258b

@surfnerd surfnerd force-pushed the input/action-actionmap-markers branch from d19472e to 539258b Compare December 5, 2024 21:12
ekcoh
ekcoh approved these changes Dec 6, 2024
View reviewed changes
Copy link
Collaborator

@ekcoh ekcoh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code changes LGTM. @surfnerd I noticed there are a lot of formatting diffs on this branch. Just wondering whether its corrections by formatting tool or accidental formatting diffs? I also noticed you got a CI error that seem to be a recent regression we have noticed also on other branches so likely unrelated to this addition.

@surfnerd
Copy link
Collaborator Author

surfnerd commented Dec 6, 2024 •
edited
Loading

Code changes LGTM. @surfnerd I noticed there are a lot of formatting diffs on this branch. Just wondering whether its corrections by formatting tool or accidental formatting diffs? I also noticed you got a CI error that seem to be a recent regression we have noticed also on other branches so likely unrelated to this addition.

@ekcoh I think the formatting changes were due to the indentation from the using block. If you turn off whitespace changes, the diff will be easier to parse. If it's too noisy, I can tweak my changes.

Yes, I've noticed the CI failure on windows in the CanInstantiateActionMap test. I don't believe it's related to my changes. If you're ok with this getting merged, then by all means.

@ekcoh
Copy link
Collaborator

ekcoh commented Dec 11, 2024

@surfnerd Recommend updating this based on develop and resolving the conflict since the unreliable test have now also been temporarily disabled until fixed. If adressed I believe this can land.

@surfnerd surfnerd force-pushed the input/action-actionmap-markers branch from 539258b to 2e38b41 Compare December 11, 2024 20:46
@surfnerd
Copy link
Collaborator Author

@surfnerd Recommend updating this based on develop and resolving the conflict since the unreliable test have now also been temporarily disabled until fixed. If adressed I believe this can land.

@ekcoh updated. Thank you

@stefanunity stefanunity merged commit 102b004 into develop Dec 12, 2024
77 checks passed
@stefanunity stefanunity deleted the input/action-actionmap-markers branch December 12, 2024 16:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@stefanunity stefanunity stefanunity approved these changes

@ekcoh ekcoh ekcoh approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@surfnerd @ekcoh @stefanunity