Skip to content

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

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.

Sign up for GitHub

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
dmarcos merged 3 commits into from
Mar 30, 2023
Merged

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

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
b569016
Check if buttonObjects or buttonMeshes is defined before accessing it…
vincentfretin Jan 21, 2023
7b2cda9
check this.buttonMeshes before calling this.onButtonChangedV3
vincentfretin Jan 26, 2023
cf102c8
Revert "check this.buttonMeshes before calling this.onButtonChangedV3"
vincentfretin Jan 26, 2023
File filter

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 6 additions & 3 deletions src/components/oculus-touch-controls.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -340,6 +340,7 @@ module.exports.Component = registerComponent('oculus-touch-controls', {
var buttonObjects = this.buttonObjects;
var analogValue;

if (!buttonObjects) { return; }
analogValue = evt.detail.state.value;
analogValue *= this.data.hand === 'left' ? -1 : 1;

Copy link
Member

@dmarcos dmarcos Jan 21, 2023

Choose a reason for hiding this comment

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

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
Copy link
Contributor Author

@vincentfretin vincentfretin Jan 25, 2023

Choose a reason for hiding this comment

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

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?
Copy link
Member

@dmarcos dmarcos Jan 25, 2023

Choose a reason for hiding this comment

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

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
Copy link
Member

@dmarcos dmarcos Jan 25, 2023

Choose a reason for hiding this comment

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

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

@vincentfretin vincentfretin Jan 26, 2023

Choose a reason for hiding this comment

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

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.
Expand Down Expand Up @@ -491,11 +492,13 @@ module.exports.Component = registerComponent('oculus-touch-controls', {

updateButtonModel: function (buttonName, state) {
// update the button mesh colors
var button;
var color = (state === 'up' || state === 'touchend') ? this.buttonMeshes[buttonName].originalColor || this.data.buttonColor : state === 'touchstart' ? this.data.buttonTouchColor : this.data.buttonHighlightColor;
var buttonMeshes = this.buttonMeshes;
var button;
var color;

if (buttonMeshes && buttonMeshes[buttonName]) {
if (!buttonMeshes) { return; }
if (buttonMeshes[buttonName]) {
color = (state === 'up' || state === 'touchend') ? buttonMeshes[buttonName].originalColor || this.data.buttonColor : state === 'touchstart' ? this.data.buttonTouchColor : this.data.buttonHighlightColor;
button = buttonMeshes[buttonName];
button.material.color.set(color);
this.rendererSystem.applyColorCorrection(button.material.color);
Expand Down