-
Notifications
You must be signed in to change notification settings - Fork 296
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
Update examples: add controller
option
#358
Conversation
controller
option
Through working on the quest 2 controller in parallel, I have realized that only some model changes are handled in Another option is to offer a pull request upstream to modify those controller components so that they truly do allow separate manipualtion of the controller without emitting an event. It could be doable without creating any breaking changes, just separation of concerns. But that's more work, anotehr pull request on another project, etc. Kind of annoying. The architecture and flow of the events for controllers is more than a bit awkward. |
This is particularly relevant for the quest 2 controllers, which I will be making a pull request to core for to enable full joystick and trigger button representation. I'd like those to be visible over the wire, and with the current design, that stuff would be missed. I'll come back with more updates soon, need to run some experiments on that. |
… EXPERIMENTAL QUEST 2 CONTROLLER MODEL COMPONENT THAT SHOULD BE REMOVED. Also has some logs that will be removed. Pushing up to do a live play test with on actual quest.
Alright, so:
|
examples/js/meta-touch-controls.js
Outdated
@@ -0,0 +1,507 @@ | |||
/* global AFRAME, THREE */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
note: I added this file only so that I could test with it on glitch: https://naf-wip.glitch.me/tracked-controllers.html
it is removed in a later commit
renderer="antialias: true; | ||
colorManagement: true; | ||
sortObjects: true; | ||
physicallyCorrectLights: true;" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
these make seeing the controller buttons much easier, they are really hard to see without this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
okay
examples/tracked-controllers.html
Outdated
@@ -5,6 +5,8 @@ | |||
<meta name="description" content="Tracked Controllers — Networked-Aframe" /> | |||
|
|||
<script src="https://aframe.io/releases/1.3.0/aframe.min.js"></script> | |||
<!-- <script src="/js/meta-touch-controls.js"></script> --> | |||
<!-- TODO: REMOVE ME!!! TESTING PURPOSES ONLY, REMOVE BEFORE MERGE --> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can remove those commented lines now.
this.el.components[this.data.controllerComponent].remove(); | ||
} catch(e) { | ||
console.error("NAF: possible problem while updating handModelStyle to controller", e) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This try catch can be removed completely right? removeAttribute below is already doing the job.
} | ||
}, | ||
|
||
remove() { | ||
this.el.removeObject3D('mesh'); | ||
this.el.removeObject3D(this.str.mesh); | ||
this.removeEventListeners(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you sure you want this.removeEventListeners();
here? It seems wrong. this.removeEventListeners();
is already in pause()
for local player symmetrically with the this.addEventListeners()
for the local player in play()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, that's correct. I hadn't touched A-Frame almost at all since last fall until the past month or so. Not sure if I forgot or never knew, but I've either learned or re-learned from you about the removeAttribute calling remove which calls pause pattern. when I started I assumed it was just incomplete. makes sense, though.
thanks for the review, I think everything I wrote would have worked fine either way, but I definitiely gained knowledge about writing better a-frame code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for your patience. I'm glad you learned some things from this review.
I also relearned yesterday that when adding a component, it runs init
and then update
!, then play
.
See https://github.com/aframevr/aframe/blob/795edfd65841d54aa2b186a02f660ffee606b50c/src/core/component.js#L306-L336
I had a code in update
that was replacing an outfit material for an avatar, and I saw it did the logic twice.
My code was this:
schema: {
outfit: { default: "" },
},
update: function (oldData) {
if (!oldData) return; // Normally always a {} if it's an object based component when adding the component locally, but can be undefined some times when NAF is creating it.
if (oldData.outfit !== data.outfit) {
this.loadOutfit();
}
}
To fix the issue I just added an extra condition:
update: function (oldData) {
if (!oldData) return; // Normally always a {} if it's an object based component when adding the component locally, but can be undefined some times when NAF is creating it.
if (oldData.outfit && oldData.outfit !== data.outfit) {
this.loadOutfit();
}
}
That works because I know I'm setting the outfit when creating the component, I don't want a naked avatar. :)
My latest update for the oculus-touch-controls (in aframe core, not here) should also fix the quest 1 (and all non-quest-2 oculus controllers, I think) controllers having the 'all buttons light up when you touch one button' issue, and that'll instantly start working with this component at that time if/when it gets merged in. |
controller
option!createdByMe()
to remove need for classthis.Y[this.Z]
pattern by more correctly utilizing A-Frame prototype patternsee live updated demo: https://naf-wip.glitch.me/tracked-controllers.html