Fix errors when buttons are pressed on Quest 2 controllers before the model is loaded

[vincentfretin]

Description:
Fix errors when buttons are pressed on Quest 2 controllers before the model is loaded.
I don't have a Quest 2, please someone test the changes.
I tested on Quest 1, I didn't break it. :)

Changes proposed:

• Check if buttonObjects or buttonMeshes is defined before accessing it, this fixes Quest 2 buttons highlighting errors when the model is not loaded yet (fix Uncaught TypeError: Cannot read properties of undefined (reading 'originalColor') #5212 and oculus-touch-controls.js:347 Uncaught TypeError: Cannot read properties of undefined (reading 'trigger') #5220)-

[dmarcos]

Thinking this code block doesn't belong here (function should be model agnostic) and probably we should listen to a triggermoved listener event like here:

https://github.com/aframevr/aframe/pull/5223/files#diff-2ee48b207a426377ea32a1749d345fd8c9b1adbe0b9d971eccb7860e1fb41cd2L214

[vincentfretin]

I don't understand what you want to change here. onButtonChangedV3 is called from onButtonChanged that is a listener of buttonchanged already. If you have a refactoring in mind, you should probably merge this PR first and do a refactoring in another PR?

[dmarcos]

This line https://github.com/aframevr/aframe/pull/5223/files#diff-2ee48b207a426377ea32a1749d345fd8c9b1adbe0b9d971eccb7860e1fb41cd2R343 if (!buttonObjects) { return; } is needed because the model is accessed here https://github.com/aframevr/aframe/pull/5223/files#diff-2ee48b207a426377ea32a1749d345fd8c9b1adbe0b9d971eccb7860e1fb41cd2R352 to update the trigger / grip. Those updates should be done in a different method than clickButtons that should remain model agnostic.

My proposal is that this check should not be necessary https://github.com/aframevr/aframe/pull/5223/files#diff-2ee48b207a426377ea32a1749d345fd8c9b1adbe0b9d971eccb7860e1fb41cd2R343 and grip / trigger and button should be updated in a separate method similar to how we do with thumbstick in onThumbStickMoved

[dmarcos]

Alternatively move the logic to updateButtonModel. Goal is to consolidate model updates in fewer places. Less redundant checks and easier to maintain.

[vincentfretin]

clickButtons is not a method, it's just an array, I think you meant onButtonChangedV3 here?
onThumbstickMoved is for thumbstickmoved event
onButtonChange/onButtonChangedV3 is for buttonchanged event.
onThumbstickMoved and onButtonChange have similar check for buttonMeshes.
You probably want to keep a separate listener for each event, right?
You want to remove onButtonChangedV3 and put the code back to onButtonChange and slightly refactor it, not sure what can be refactored exactly though? Is that what you want?
Or simply do the if (buttonMeshes) check before calling this.onButtonChangedV3(evt) and remove it from onButtonChangedV3?. I'll do the latter that in a commit now.

updateButtonModel is called from updateModel which is called by onButtonEvent, used to define listeners onButtonDown onButtonUp onButtonTouchStart onButtonTouchEnd, but not for onButtonChanged currently, not sure what is particular about it currently and if we can define onButtonChanged to use onButtonEvent instead and move all the current logic of onButtonChanged/onButtonChangedV3 to updateButtonModel. If this is what you want, I really prefer we do it in a separate PR, because it's not a trivial change, I don't want to break anything in this PR.

[vincentfretin]

I tried to put the condition in onButtonChanged but I got a failing test

 oculus-touch-controls
    buttonchanged
      ✖ can emit triggerchanged

I thought it was a simple change though I don't understand why it failed. I reverted it.
Feel free to merge it as is or not. I'll try doing a refactor in two weeks maybe, not right now.

[vincentfretin]

@dmarcos you should merge it really, it's a minimal set of changes to fix the errors of the two linked issues.
I don't know when I'll have the time to do a refactoring like you suggested, but I think it should be in a separate PR anyway.

[dmarcos]

Super thanks!