Quest 2 controller fixes (fixes issue #5607) #5103
Conversation
…ort thumbstick model updates; added recycled euler for trigger updates, which are performed as a multi-dimensional rotation for v3 controller model; added thumbstickMoved event listener, and matching remove handler; slight comment cleanup in a few places; added onButtonChangedV3 and onModelLoadedV3 for handling non-conforming model cleanly while maintaining backwards compatibility unambiguously; added multiMeshFix to fix issue that all oculus controlers have had, where all buttons light up if any button is pressed; update to use model default color as button color when not being touched;
kylebakerio
changed the title
|
I looked quickly at your code changes, I don't have anything to say actually code wise, the changes looks good. |
|
Thanks for the review, I've made the requested changes. Out of curiousity: is there any chance of A-Frame allowing ES6 in the future? |
Thanks for the patience. I'll do another pass as soon as I can. No plans to move to ES6 at the moment. I like to keep the code consistent and the simplicity of ES5. Less stuff for newbies to learn. |
|
I see. I wonder at what point knowing what counts as ES5 vs what counts as ES6 will be considered more esoteric knowledge than knowing ES6. I imagine all newbies these days learn javascript with ES6 just taught as a normal part of the language without knowing that we had to live without it before 2017~. Appreciate your work as always, thanks for taking a look at this. |
|
(that last commit is not just a linter commit, it's also the fix in response to vincent's comment--I then had to make some linter recommended changes and didn't catch that my prior commit that triggered the linter didn't go through.) |
|
@dmarcos just giving a quick ping on this |
|
@dmarcos made requested changes on 3 out of 5, responses on the other two. |
| }); | ||
|
|
||
| /** | ||
| * A-Frame's material component makes a single unified shared material for all meshes. |
There was a problem hiding this comment.
Is this true? I thought is the controller glTF that has a shared material across meshes?
There was a problem hiding this comment.
Hm, personally, I'm not sure; Diarmid suggested this was a characteristic of A-Frame itself (perhaps a characteristic of the GLTF loader?), but I took his word on that and just worked backwards from there (and seeing how others had dealt with similar scenarios) to come up with this fix. I haven't personally deeply investigated what A-Frame does, or whether this is something that is encoded in the GLTF.
I do know that I can color button mesh materials individually in a babylon sandbox viewer, which suggested to me that it was about the gltf loader/ a-frame, and not something with the model.
Happy to update the comment with more correct information if you have any insight into what is going on more precisely here.
There was a problem hiding this comment.
@diarmidmackenzie if I've worded this poorly or incorrectly, or if you have more insight into explaining the issue you've encountered here, let me know.
There was a problem hiding this comment.
Also of note: all oculus models had this problem where all button meshes would get re-colored if you colored any of them.
fixes bug where all buttons would light up if any button was pressed
Perhaps all oculus controllers' models had the same issue? there are other controllers with other components with related code that did work (as in, individual buttons could be highlighted), but I haven't checked, maybe those were the older ones that weren't gltf.
There was a problem hiding this comment.
The comment is definitely wrong in this context.
@kylebakerio I think the comment references the issue described in this comment #5067 (comment) ? But I think this is a complete different issue.
The aframe geometry or material component are not even used here. The model is loaded via the gltf-model component that just use THREE.GLTFLoader. I don't think there is an issue with GLTFLoader either. If you open https://cdn.aframe.io/controllers/oculus/oculus-touch-controller-gen2-right.gltf in https://gltf-viewer.donmccurdy.com/ you will see 7 draw calls and 1 material, meaning 7 meshes and 1 material. If you import it in blender you will see that the same material is used in 7 meshes. Same issue with oculus-touch-controller-v3-right.glb where 1 material is used by 6 meshes.
The GearVR controller is an obj/mtl and has 3 meshes, each mesh with its own material.
There was a problem hiding this comment.
I see. So the 'problem' is the model, not the issue with a-frame mentioned there (if it actually exists?), then. Will update comment to reflect this, then.
There was a problem hiding this comment.
Seems it is not relevant here but there is a limitation in the A-Frame material component.
Specifically, a Three JS Mesh can have multiple materials specified as an array, as described here.
https://threejs.org/docs/index.html#api/en/objects/Mesh.material
When this happens, they are applied in turn to the different groups in the geometry.
The A-Frame material component doesn't support this, and always a single material for the whole object, which is applied to all the groups in the geometry.
However all this is only relevant when working with geometry and materials, so it sounds like it's not relevant in this context.
| * the color on every button mesh. | ||
| */ | ||
|
|
||
| function cloneMaterialToTHREE (object3d) { |
There was a problem hiding this comment.
cloneMeshMaterials probably a more descriptive name
There was a problem hiding this comment.
based on the conversation above, I agree. Will change.
| }); | ||
|
|
||
| /** | ||
| * A-Frame's material component makes a single unified shared material for all child meshes. |
There was a problem hiding this comment.
Can say something like this:
Some of the controller models share the same material for different parts (buttons, triggers...). In order to change their color independently we have to create separate materials.
There was a problem hiding this comment.
agreed, will change.
| // Set the controller display model based on the data passed in. | ||
| this.displayModel = CONTROLLER_PROPERTIES[data.controllerType] || CONTROLLER_PROPERTIES[CONTROLLER_DEFAULT]; | ||
| // If the developer is asking for auto-detection, see if the displayName can be retrieved to identify the specific unit. | ||
| // This only works for WebVR currently. |
There was a problem hiding this comment.
- This does not only work for WebVR, we do it for both WebVR and WebXR. This comment was out of date.
- This comment is still in the code:
// If the developer is asking for auto-detection, use the retrieved displayName to identify the specific unit., line 227
| var trackedControlsSystem = this.el.sceneEl.systems['tracked-controls-webvr']; | ||
| // WebVR | ||
| if (trackedControlsSystem && trackedControlsSystem.vrDisplay) { | ||
| // WebVR |
| } | ||
| } else { // WebXR | ||
| } else { | ||
| // WebXR |
There was a problem hiding this comment.
this should move to the line above as it was before
There was a problem hiding this comment.
I found having one to the side, and one above, to make the code confusing to read. Given that they are symmetrical comments describing symmetrical code blocks, this was a way to make them both visually and logically consistent.
The other way to make them logically consistent (both be inside the block they describe) would be to have them both to the side:
if { // webvr
// ...
} else { // webxr
// ...that feels a bit messier than what I went with imo (with the way I've written it, the webxr and webvr comment are vertically aligned as well as both within the respective blocks they describe), but would still be an improvement over the way it was. (Maybe you prefer this?)
As for the way it was, it feels like a comment outside of the entire if block describes the entire if/else chain (which wouldn't be right; only the 'if' is webvr), where as a comment inside the if block describes what is inside that block (which is more accurate). Having one be above all and one to the side/within felt like the least optimal way to position those comments to me.
As someone who was working on this code and having to grok those comments, are you willing to reconsider this reposition? I personally really feel it is an improvement, but it's of course subjective. Let me know.
| }, | ||
|
|
||
| updateButtonModel: function (buttonName, state) { | ||
| updateButtonModel: function (buttonName, evtName) { |
There was a problem hiding this comment.
keep previous state parameter name.
There was a problem hiding this comment.
Well, this was a conscious change. If you'll humor me, this feels like a mistake.
tldr:
updateModel(), which calledupdateButtonModel(), calls this argumentevtName- this is indeed only a name, and not state, which exists and has been stripped by this point
onButtonChanged()follows a different flow and actually does useevt.detail.state- calling this
stateseems distinctly like an oversight, I assumed it was likely a leftover from an older version of the code or from a copy of another component.
Detailed argument:
- oculus-touch-controls.js adds onButtonDown (etc.) event listeners, have them point to utils/tracked-controls.js/
onButtonEvent() - tracked-controls.js emits 'buttondown'
- oculus-touch-controls.js picks that up, and strips away the event and only passes
evt.detail.idand the stringdown; state is ignored - utils/tracked-controls.js/
onButtonEvent()calls oculus-touch-controls.js/updateModel() directly updateModel()calls the argument it receivesevtName(correctly, as it does not include state)- but then,
updateModel()callsupdateButtonModel(), which now calls thatstateinstead ofevtName
This is particularly confusing, because evt.detail.state actually is a thing that exists, but it isn't available here.
There is a different flow where state is made accessible:
- oculus-touch-controls.js adds onButtonChanged event listener
- this is different than the onButton Down / Up / TouchStart / TouchEnd events; it doesn't use that utils function, but instead passes the raw event, which includes event.detail.state, to its own function (not the utils
onButtonEvent()function) evt.detail.state.valueis then used to precisely set the mesh to a new position
Based on all that, it seemed like an oversight that this argument was called state instead of evtName, being that both are clearly defined concepts in this file and outside of this file.
If you disagree, I of course can change it back; let me know. Just wanted to explain my process and advocate for what I see as an improvement to the code.
There was a problem hiding this comment.
It's better not to change anything unrelated to the PR. It makes reviews more complicated. Also increases the chances of bugs that go unnoticed
|
This is really close. Just needs to change back some lines / code unrelated to the PR. Thanks for the patience and the effort. So glad we have this working again! Awesome job to everyone that participated |
|
@dmarcos do you have any insight into why that build would fail? When I run that test locally it passes.
It looks like this is the error in the test online: 13 09 2022 21:42:49.196:ERROR [framework.browserify]: bundle error
13 09 2022 21:42:49.197:ERROR [framework.browserify]: Error: Can't walk dependency graph: Cannot find module 'super-three/examples/js/loaders/KTX2Loader' from '/home/runner/work/aframe/aframe/src/lib/three.js'
required by /home/runner/work/aframe/aframe/src/lib/three.jsIs that related to some change on master, perhaps...? |
|
Looks like https://github.com/supermedium/three.js/tree/super-r144/examples/js/loaders is missing KTX2loader.js, while my npm install was on 141 before the bump we had a couple days ago to 144. |
|
Here's the change: supermedium/three.js@d5b300e |
|
Just followed vincent's comment all the way up the chain. I look forward to us switching to a more modern build tool in the form of webpack for the future health of the project. I think this is still safe to merge as-is, since it passes locally and was passing before the last non-substantial changes, but if it needs to wait until the upgrade to webpack and it starts passing again, so be it. Thanks (again) for doing that work Vincent. Build tool maintainence like that is for sure not the most exciting work out there. |
|
I think we can merge this after #5103 (comment). Thanks |
|
I addressed a couple of nits. Thanks so much for the patience and being accommodating. This is great work. So glad we have this working again. This kind of details matter a lot to make a great experience. Great job! 🥳 🏅 |
4 participants
See issue 5607.
Features:
All Oculus controllers:
Quest 2 controllers:
Description:
I've tested all four controller models using the this pre-merge version of the WebXR Emulator Extension: MozillaReality/WebXR-emulator-extension#278
I have run a build with this branch and use that built version of A-Frame that includes this change here:
https://glitch.com/~quest-2-controller-responsive-buttons