InputSystem icon indicating copy to clipboard operation
InputSystem copied to clipboard

[Draft] Modified OnScreenControl to support a customisable update mode

Open ekcoh opened this issue 1 year ago • 1 comments

Description

Added a new "update mode" property on OnScreenControl.

Updated the OnScreenControl example in /samples.

Changes made

Feature addition to OnScreenControl:

  • Added new enumeration type to public API OnScreenControlUpdateMode which allows the user to specify whether the OnScreenControl should output its input data by queuing input events to the system (OnScreenControlUpdateMode.Queue, default) or whether the OnScreenControl should output its input data by changing the underlying control state via InputState.Change(...) directly (OnScreenControlUpdateMode.StateChange, new optional mode).

Updated the OnScreenControl example:

  • Added OnScreenControlSample.inputactions defining actions corresponding to OnScreenControls used in example.
  • Added OnScreenControlSample.cs hooking up performed handlers to log when action events fire.
  • Modified A, B to correspond to Gamepad controls instead of Keyboard to not confuse users.
  • Added Mode 1 and Mode 2 buttons mapping to Keyboard controls instead and allowing these to be used to switch update mode interactively when running the sample.
  • Added inline documentation explaining how the sample is rigged up.

Removed previous FIXME relating to this.

Notes

Note that PR still contains debug logging in OnScreenStick and OnScreenButton.

Note that updating via InputState.Change(...) avoids delaying generated until the next frame and effectively reduces latency of OnScreenControl by one frame or equivalently 1 / FrameRate seconds. However, this cannot be the default value since it would break backwards compability. However, OnScreenControlUpdateMode.StateChange have been highlighted as the recommended setting.

This PR needs significant testing and QA before considered to be merged since potential side-effects or detection of improper control layouts for this update pattern are currently not clear.

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.
    • FogBugz ticket attached, example ([case %number%](https://issuetracker.unity3d.com/issues/...)).
    • FogBugz is marked as "Resolved" with next release version correctly set.
  • [ ] 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.

ekcoh avatar Nov 05 '23 09:11 ekcoh

Agree @jamesmcgill that I think it should replace previous behaviour. However I think it could regress existing implementations relying on this, but frankly it should be seen as a bug since previous implementation is not providing what you want. Will revisit this again when it may be prioritised and review API contracts in light of making this the default behavior. For the sake of the example I also believe there would be much to gain to make it realistic from a UI point of view, at least from a physically scaled and alignment perspective since the current UI representation isn't usable for cross-device usage and doesn't even have a usable layout to begin with. The sample improvement could however be separated from the functional change, as long as the functional change comes with test coverage which is currently missing.

ekcoh avatar Nov 06 '23 21:11 ekcoh