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: UI generation of custom interactions of action properties when it rely on OnGUI callback (ISXB-886) #1957

Merged
merged 9 commits into from
Jun 30, 2024

Conversation

bmalrat
Copy link
Collaborator

@bmalrat bmalrat commented Jun 25, 2024

Description

The action editor was not displaying the content of custom interactions when it's done in IMGUI. ISXB-886
jira : https://jira.unity3d.com/browse/ISXB-886

When no custom editor exist the generated UI for enum doesn't work well. The value is lost when save + error in the console.

Changes made

Added an IMGUIContainer to handle IMGUI UI.
Before the change :
image

After the change :
image

Also added some early exit when IMGUI is not the default to prevent to UI to be drawn twice.

Also fixed the generation of UI for enum which loosed the value when saved. Now it use the index instead of trying to convert the string to the value.

Testing

Tested the repro project which contain IMGUI UI and buildin interaction in UITK + custom interactions without editor

Risk

Only in editor, should be safe

Checklist

Before review:

  • [X ] 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:

  • [X ] 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.

@Pauliusd01 Pauliusd01 self-requested a review June 26, 2024 05:49
@Pauliusd01
Copy link
Collaborator

Pauliusd01 commented Jun 27, 2024

It seems like I lose focus every time I interact with these fields from the custom interactions, is this a bug or something the user script lacks? I'm leaning towards a bug 😅
https://github.com/Unity-Technologies/InputSystem/assets/54306142/e39154cd-d191-45cb-8b94-78fc31b37cc7

Also, added @ekcoh for code review, feel free to add someone else if you're busy

@Pauliusd01 Pauliusd01 requested a review from ekcoh June 27, 2024 11:33
@Pauliusd01
Copy link
Collaborator

Pauliusd01 commented Jun 27, 2024

For some reason the Axis Deadzone processor is duplicated with your changes. This seems to be the only processor/interaction affected (All settings are completely default, all you have to do is create a new input actions asset and add the processor)
image

Copy link
Collaborator

@Pauliusd01 Pauliusd01 left a comment

Choose a reason for hiding this comment

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

Updating status

@bmalrat
Copy link
Collaborator Author

bmalrat commented Jun 27, 2024

ract with these fields from the custom interactions, is this a bug or something the user script lacks? I'm leaning towards a bug 😅

good catch I missed those ones, processor editor are also InputParameterEditor, I only checked Interaction.
They also have 2 UI Implementation.

@bmalrat
Copy link
Collaborator Author

bmalrat commented Jun 27, 2024

It seems like I lose focus every time I interact with these fields from the custom interactions, is this a bug or something the user script lacks? I'm leaning towards a bug 😅 https://github.com/Unity-Technologies/InputSystem/assets/54306142/e39154cd-d191-45cb-8b94-78fc31b37cc7

Also, added @ekcoh for code review, feel free to add someone else if you're busy
thanks I will take a look

@Pauliusd01 Pauliusd01 self-requested a review June 27, 2024 15:57
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. Do I understand it correctly that the focus issues are still an open issue? It sounds like that might need some attention. I am approving this now given that the code changes looks good but there might be side effects as called out by Paulius?

@Pauliusd01
Copy link
Collaborator

Pauliusd01 commented Jun 28, 2024

Code changes LGTM. Do I understand it correctly that the focus issues are still an open issue? It sounds like that might need some attention. I am approving this now given that the code changes looks good but there might be side effects as called out by Paulius?

We've discussed this with Benoit and decided to make a separate bug for that issue. I've since noticed it also affects some non custom interactions such as the multi tap one, but it is not as a big of a problem there as the custom ones could be. (Bug link: https://jira.unity3d.com/browse/ISX-2074)

Copy link
Collaborator

@Pauliusd01 Pauliusd01 left a comment

Choose a reason for hiding this comment

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

LGTM, checked that extisting interactions/processors function normally and that the custom ones show up correctly as well.

@bmalrat bmalrat merged commit 27322b1 into develop Jun 30, 2024
77 of 79 checks passed
@bmalrat bmalrat deleted the isxb-886-fix-for-ugui-interaction-inspector branch June 30, 2024 16:43
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