Skip to content

Conversation

@stefanunity
Copy link
Collaborator

@stefanunity stefanunity commented Feb 12, 2025

Description

  • see title.
  • similar test to MeasureInputSystemFrameTimeWithProfilerMarkers_FPS() just with touch instead of mouse&keyboard input

Testing status & QA

  • test runs
  • data submitted to hoarder

Overall Product Risks

  • none, internal tests

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.

@stefanunity stefanunity changed the title ADD: new touch test measuring input system profiler markers NEW: add touch test measuring input system profiler markers Feb 12, 2025
@jfreire-unity jfreire-unity self-requested a review February 13, 2025 10:41
Copy link
Collaborator

@jfreire-unity jfreire-unity left a comment

Choose a reason for hiding this comment

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

Looks good to me.
Maybe add some top level comments to explain what it does for this particular test?

One suggestion will to be clear on of what we want to measure this against to. I know we already do test doing nothing, doing keyboard and mouse input in Performance_MeasureInputSystemFrameTimeWithProfilerMarkers_FPS but it contains actions so it's not a 1 to 1 comparison. Should this test also contain some actions setup?

performedCallCount++;
};

using (Measure.ProfilerMarkers(allInputSystemProfilerMarkers))
Copy link
Collaborator

Choose a reason for hiding this comment

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

I guess changes look ok here but I wonder what scenario we are trying to replicate here? It seems that there is not many touch points per frame here, essentially looks like one touch update per frame. That is fine given a e.g. 30 or 60 Hz touch sensor , but I believe most these days generate higher frequency of touch. It might be meaningful to measure a decently modern touch screen and take the actual achievable frequency. I wouldn't trust just reading specs since the HW frequency might be higher than what you get out of the driver.

Copy link
Collaborator

Choose a reason for hiding this comment

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

It be good to outline the intended scenario as clearly as possible in the test description or inline (Seems like @jfreire-unity suggested something similar.

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.

Approving this as is but I think we should consider relevance of scenario.

@ekcoh
Copy link
Collaborator

ekcoh commented Feb 26, 2025

Is this still work in progress or why Draft status?

@stefanunity stefanunity marked this pull request as ready for review February 27, 2025 13:13
@stefanunity stefanunity merged commit 2f84ef8 into develop Feb 28, 2025
94 checks passed
@stefanunity stefanunity deleted the performance/touchperformancetest branch February 28, 2025 14:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants