Skip to content

Conversation

@vincentfretin
Copy link
Collaborator

This closes #5

@vincentfretin
Copy link
Collaborator Author

I tested VR on Quest and AR on Chrome Android on this glitch https://glitch.com/edit/#!/vfretin-aframe-xr-starterkit
no regression as far as I can tell, only bugfixes. :)

@vincentfretin
Copy link
Collaborator Author

Previously in VR with movement-controls right joystick didn't work to move the camera, now it works since c-frame/aframe-extras#373 was merged but if I move position with left joystick afterwards it doesn't go in the correct direction. Funny I don't have this issue at all in my app and the code is the same as far as I can tell except my camera has id="camera" and yours id="head".
There is another thing that bother me, it moves forward now when I press trigger when movement-controls is enabled, I'm not sure what changes introduced that.

@vincentfretin
Copy link
Collaborator Author

I removed your hack that reset the cameraRig rotation to y=0 when switching to movement-controls.
My guess is that you added this hack because of the issue:

that I now fixed in the PR:

I used the build of this PR here.

@vincentfretin
Copy link
Collaborator Author

If I comment the main.js file, I don't have the issue anymore where the left joystick goes into the wrong direction, so there is something in main.js that does something on cameraRig or camera rotation.

@vincentfretin
Copy link
Collaborator Author

It's your xr-follow component that causes the issue, but I don't understand why. This component is set on the parent of the sword and watergun. I don't understand why it impacts cameraRig/camera heading when moving.

@AdaRoseCannon
Copy link
Owner

Oh that’s weird it shouldn’t change anything else. I’ll take a quick look.

@vincentfretin
Copy link
Collaborator Author

I found it. It's the following code:

const cameraObject = scene.camera;
const camera = scene.is('vr-mode') ? scene.renderer.xr.getCamera(cameraObject) : cameraObject;

scene.renderer.xr.getCamera is overriding the active camera scene.camera I think.

If I replace those line by just

const camera = scene.camera;

it seems to work properly. I see the sword and watergun, I can grab both.

@AdaRoseCannon
Copy link
Owner

That was to get it working in VR glad it is no longer needed but weird that it would have side effects

@vincentfretin
Copy link
Collaborator Author

Yes it's a bit weird, I don't fully understand it, but if the code is simpler, still work in VR and this fixes the movement issue, then I won't dig it further. :)

@vincentfretin
Copy link
Collaborator Author

Ok so there is only one issue remaining before we can merge. This is this issue:

@vincentfretin
Copy link
Collaborator Author

Yeah I really don't understand how the issues are related :D How it can impact the heading calculation here
https://github.com/n5ro/aframe-extras/blob/5b1339d86f59e871931a7e94f51288f59364aa6a/src/controls/movement-controls.js#L191-L194

I just wanted to mention getCamera has no argument in threejs r144
https://github.com/mrdoob/three.js/blob/68daccedef9c9c325cc5f4c929fcaf05229aa1b3/src/renderers/webxr/WebXRManager.js#L560-L564
so the previous code was slightly wrong, in this threejs version anyway.

@vincentfretin
Copy link
Collaborator Author

FYI in threejs r144 we have this change mrdoob/three.js@777f97a
that's probably why we don't need the previous fix now.

@vincentfretin
Copy link
Collaborator Author

Alright, I fixed the issue c-frame/aframe-extras#392 with trigger moving forward, that was a regression when I merged the changes in touch-controls for Chrome Android Cardboard button.
I currently use an aframe-extras build with c-frame/aframe-extras#396 changes. Let's wait a few days if someone has any feedback on those changes. Once this is merged, you will be able to merge this PR too.

You can test the changes in my glitch https://glitch.com/edit/#!/vfretin-aframe-xr-starterkit

@AdaRoseCannon
Copy link
Owner

FYI in threejs r144 we have this change mrdoob/three.js@777f97a
that's probably why we don't need the previous fix now.

Nice find!

@AdaRoseCannon
Copy link
Owner

Thank you for all the fixes, glad to see so many hacks removed

@vincentfretin
Copy link
Collaborator Author

I merged the aframe-extras PR, now using again a build from aframe-extras master.
I updated to use an aframe master build with three r147 (r144 previously), there are no new warning or error. I tested in VR, grabbing the sword, climbed the ladder, and tested again AR mode on Chrome Android.
You can merge now or wait for aframe 1.4.0 in a week or two as you wish.

@AdaRoseCannon
Copy link
Owner

Fantastic! Let’s merge:D

Thank you so much for all of your work in this!

@AdaRoseCannon AdaRoseCannon merged commit 638c4c3 into AdaRoseCannon:glitch Dec 14, 2022
@vincentfretin
Copy link
Collaborator Author

Cool, thanks. Please don't forget to update the glitch as well. I'm not sure what it your workflow here, you just open the glitch terminal and git pull and refresh I guess?

@vincentfretin vincentfretin deleted the update-all-packages branch December 14, 2022 13:44
@AdaRoseCannon
Copy link
Owner

Updated

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.

Compatibility with aframe master threejs r141

2 participants