Skip to content

Conversation

@ekcoh
Copy link
Collaborator

@ekcoh ekcoh commented Nov 27, 2024

Description

Visualizer sample has problems adopting to device disconnects and lack convenience for multiple devices. (Reacting to current device). Relates to bug report ISXB-1243.

Changes made:

  • Fixed a bug where transition from an InputDevice instance to null would recreate visualizers that do not require a valid control instance. Previously all of these visualizer "factories" where incorrectly gated on having an available control instance.
  • Added a new serialised property/setting to InputControlVisualizer that allows overriding Control index property to bind the visualisation to the current device (This is what is shown in the current device visualization). Note that without this setting each individual visualisation component is unsynchronised with the others and only base its source on the control index integer.
  • Updated GamepadVisualizer to use this setting "Use Current Device" to true for all its control instances in the scene.
  • Some minor refactoring of existing code to support the above.

Note that this PR doesn't update other visualizers in a similar way. This will be handled separately unless requested.

Testing status & QA

Test visualisers with no, one and multiple devices connected to see that behavior makes sense.

Overall Product Risks

Small, its an example.

  • Complexity: Small
  • Halo Effect: Small

Comments to reviewers

Note that when having two gamepads, e.g. DualSense and Dualshock and disconnecting one, Gamepad.current will be null until the previously connected Gamepad is actuated. This relates to the current behavior of current on device removal (separate concern).

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.

…his was previously incorrectly skipped, but only value control visualizer needs a device to show anything useful.
…l of the current device of the associated device interface type. Updated GamepadVisualizer to use this setting (ON) for all relevant controls.
@ekcoh ekcoh marked this pull request as ready for review November 27, 2024 14:30
Copy link
Contributor

@AlexTyrer AlexTyrer left a comment

Choose a reason for hiding this comment

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

Changes look good to me - will test out these changes when I get a chance with my DualShock4 and DualSense.

@Pauliusd01
Copy link
Collaborator

Pauliusd01 commented Nov 28, 2024

Is the intention that with these changes I can use multiple controllers and they would both be picked up correctly? Because right now if I connect a second controller that controller is picked up by "gamepad.current" when I click any of its buttons BUT they don't actually do anything in the visualiser. This behaviour is identical without your changes as well

TLDR: should multiple controllers work with your changes if so they're not working

@ekcoh
Copy link
Collaborator Author

ekcoh commented Nov 28, 2024 via email

@Pauliusd01
Copy link
Collaborator

Did you by any chance test on Windows where the BT disconnect bug is also present? I will test there again since I implemented the sample changes on Mac where that problem is not present. All visualizers should reflect the current. Otherwise it’s not working as intended. tors 28 nov. 2024 kl. 09:36 skrev Paulius Dervinis @.***

: Is the intention that with these changes I can use multiple controllers and they would both be picked up correctly? Because right now if I connect a second controller that controller is picked up by "gamepad.current" when I click any of its buttons BUT they don't actually do anything in the visualiser. This behaviour is identical without your changes as well TLDR: should multiple controllers work with your changes if so they're not working — Reply to this email directly, view it on GitHub <#2063 (comment)>, or unsubscribe https://github.com/notifications/unsubscribe-auth/ACEO54B7KG3J76YAG3VHUP32C3IXZAVCNFSM6AAAAABSSVIHJSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDKMBVGUZTSNRTHA . You are receiving this because you authored the thread.Message ID: @.***>

I did indeed use win 11. Will try mac

@ekcoh
Copy link
Collaborator Author

ekcoh commented Nov 28, 2024

@Pauliusd01 I will try Win :)

@Pauliusd01
Copy link
Collaborator

Pauliusd01 commented Nov 28, 2024

Seeing same behaviour on Mac.

Steps: Connect dualsense controller and use it in the visualiser -> connect an xbox controller in addition to that -> try to use it in the visualiser -> gamepad.current shows that you're using an xbox controller but none of the buttons light up (Both connected on bluetooth)

Note that it works exactly the same in develop so I had to ask if the goal of this PR was to change that

@ekcoh
Copy link
Collaborator Author

ekcoh commented Nov 28, 2024

Fixed a mistake of an unfortunate semicolon.

Second fix adresses a problem with Gamepad.current changes not being detected due to how Gamepad.current is modified on device removal (separate concern). This fix scans for changes to current while not having a resolved control.

…after being assigned to support using multiple gamepads simultaneously. Note that it may still be a problem if gamepads are noisy.
@ekcoh
Copy link
Collaborator Author

ekcoh commented Nov 29, 2024

Added proper support to sample for mixing usage between multiple active connected gamepads.

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 multiple controllers being connected/disconnected in play mode and player (mac/win) There are some minor things still but they are known and will be fixed in the future.

@ekcoh ekcoh merged commit 59c7c40 into develop Nov 29, 2024
77 checks passed
@ekcoh ekcoh deleted the isxb-1243-fix-visualizer branch November 29, 2024 15:03
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