Skip to content

FIX: remove check that triggered NotImplementedException for Usage bindings (ISXB-373) #1625

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

Merged
merged 3 commits into from
Jan 10, 2023

Conversation

jamesmcgill
Copy link
Collaborator

Description

If a Usage (e.g. Submit) was bound then an NotImplemented exception could occur because it hit a case that wasn't implemented yet.

  • The sample project I had was extremely simple to reproduce: just bind an action to Submit (then attempt to call InputControlPath::ToHumanReadableString() to trigger the failure).
  • In that case the control will be Key/Enter and control.device will be Keyboard.
  • Usages are stored at device level, not in the control subhierarchy AFAIK.

There is a TODO: support scavenging a subhierarchy for usages there, so my first instinct was to implement the missing case, however, at least in this case I couldn't see why we would want to search from Key/Enter downwards in the subhierarchy. And I couldn't come up with another use-case. I'm also unsure how this could be implemented if we wanted to (without moving usages out to be on per control basis which seems redundant?).

Perhaps I haven't understood the intention of the comment correctly, so happy to hear if someone knows better?

Even if we do have future plans for this, I think we should still remove this Exception today as the reproduction was extremely easy to trigger and I don't see any issue with looking at the root device for Usages in the meantime.

Changes made

  • Removed the check that triggered the exception and allow the code to fall through and use the device.

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.

Copy link
Collaborator

@lyndon-unity lyndon-unity left a comment

Choose a reason for hiding this comment

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

I don't understand this area well enough to understand side effects but I feel that removing this exception is reasonable.

Perhaps a debug warning could be valuable but I'm comfortable clicking approve for the error case reported.

@jamesmcgill
Copy link
Collaborator Author

Yeah, I'm also not 100% on what is intended missing feature here. Unless there is a rush to get this in, I think we can leave it open for the others to review incase I'm missing something.

I don't even think the warning is necessary, as my understanding at the moment is that this works as-is (i.e. continues to use the device when looking for the Usage in this case should be correct).

@jamesmcgill jamesmcgill changed the title FIX: remove check that triggered NotImplementedException for Usage bindings. FIX: remove check that triggered NotImplementedException for Usage bindings (ISXB-373) Jan 5, 2023
Copy link
Contributor

@jimon jimon left a comment

Choose a reason for hiding this comment

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

I couldn't came up with counter example. Given that nobody complained for years about this lets merge and see what breaks down the line.

@jamesmcgill jamesmcgill merged commit 60b198d into develop Jan 10, 2023
@jamesmcgill jamesmcgill deleted the ISXB-373_usage_binding_NotImplementedException branch January 10, 2023 13:27
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.

3 participants