AltspaceVR / AltspaceSDK

Software Development Kit for AltspaceVR
https://developer.altvr.com/
MIT License
161 stars 73 forks source link

Fixing JointCollisionEvents.js to support more than one Object3D in the scene #177

Closed Fasani closed 7 years ago

Fasani commented 7 years ago

It seems that the JointCollisionEvents.js would only work for the last loaded Object in the scene.

After several hours of digging I noticed that the skeleton was not available inside the update method and was exiting the loop straight away.

if(!skeleton) return;

I resolved this by attempting to get the Skeleton repeatedly inside the update loop until it is successful.

I will be honest, I am unsure if this is the correct fix, I would be happy to hear some input. But it does seem to resolve the issue.

You can demo the behaviour using the new example: joint-collision-push-two-cubes.html

brianpeiris commented 7 years ago

Hey @Fasani, upon further investigation, this issue seems to have been caused by an internal problem with getThreeJSTrackingSkeleton. The first promise fails to resolve if you request the skeleton more than once in quick succession.

So it turns out your fix will be unnecessary once the root cause is fixed. So please go ahead an undo your changes to JointCollisionEvents.

Your example is still very useful so I'd like to merge it, but I'd ask that you delete the old example (since the new one is mostly identical) and instead add a reset button to your new one, so that we still have that function.

If you can do those two things, I'd be happy to merge.

NGenesis commented 7 years ago

The fix for getThreeJSTrackingSkeleton was applied on April 7th so this issue should not be occurring now.

brianpeiris commented 7 years ago

Yup, I'll still try to merge the examples in though.

Fasani commented 7 years ago

@brianpeiris I will look into if this example, if it is still needed or if the code would make sense still. If not I will close the PR.

Fasani commented 7 years ago

Examples need reworking, I will close the PR for now.