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.

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

FIX: ISXB-925 Fixed an issue where changing InputSettings instance fo… #1954

Merged
merged 10 commits into from
Jun 26, 2024

Conversation

ekcoh
Copy link
Collaborator

@ekcoh ekcoh commented Jun 21, 2024

…r the Input System wouldn't affect feature flags.

Description

The optimization flags within InputSettings were stored as static fields for performance reasons. This makes little sense from API perspective where these are set and evaluated using a non-static API. This also leads to switching InputSettings instance having no impact on current settings. As long as these are exposed via this API they should not be static. However, I added extraction of the settings into InputManager which is represented by a static and used direct access from where its used so expecting little if any impact.

Adressing:

  • Feature flags set and checked via InputSettings non-static interface but backed by static variables.

Changes made

  • Changed flags to be extracted in Input Manager instead when settings change.
  • Removed custom handling in InputSettings which allow IsFeatureEnabled to work.

Note that this fix also includes #1955 since the test case would otherwise fail with debug error log on non Windows platforms.

Testing

Added 1 test case for scenario Settings_ShouldStoreSettingsAndFeatureFlags(string featureName). I didn't add a new test category at this point since none of the existing fits well so I left the test case uncategorised.

Risk

Potential risk is features not being enabled correctly for some corner-case scenarios.

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.

…r the Input System wouldn't affect feature flags.
@ekcoh ekcoh requested review from timkeo and lyndon-unity June 21, 2024 07:01
@ekcoh ekcoh requested review from graham-huws and removed request for lyndon-unity June 24, 2024 11:50
@ekcoh ekcoh requested a review from ritamerkl June 24, 2024 12:25
Copy link
Collaborator

@timkeo timkeo left a comment

Choose a reason for hiding this comment

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

From my FEPM refactoring worksheet, I didn't touch these static fields, since determined it wasn't necessary for CoreCLR support. However, there may still be conflicts.

I recommend checking these changes against my FEPM refactor branch to make sure isn't anything too complicated, otherwise porting this to Neutron will be a pain.

Comment on lines 2121 to 2123
internal bool m_OptimizedControlsFeatureEnabled;
internal bool m_ReadValueCachingFeatureEnabled;
internal bool m_ParanoidReadValueCachingChecksEnabled;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could you please change these to private fields and add an internal read-only accessor property?

When refactoring for CoreCLR support, internal fields were very frustrating because I had to check the entire code base for any usages.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This have been adressed now in c93e565

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Regarding FWPM refactor I think this is straightforward since ApplySettings seem intact.

ekcoh added 2 commits June 25, 2024 11:16
…al getters instead. Removed WindowsGamingInputBackend feature flag from test since generating error. This is handled in a separate PR as well.
Copy link
Collaborator

@ritamerkl ritamerkl left a comment

Choose a reason for hiding this comment

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

Good change!

@@ -2116,6 +2117,33 @@ internal struct AvailableDevice
internal IInputRuntime m_Runtime;
internal InputMetrics m_Metrics;
internal InputSettings m_Settings;

// Extract as booleans (from m_Settings) because feature check is in the hot path
Copy link
Collaborator

Choose a reason for hiding this comment

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

nice!

@ekcoh ekcoh requested a review from stefanunity June 25, 2024 12:53
@ekcoh ekcoh merged commit f9d7240 into develop Jun 26, 2024
77 of 79 checks passed
@ekcoh ekcoh deleted the fix-isxb-925-feature-flags branch June 26, 2024 12:22
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.

5 participants