tizzle / aframe-orbit-controls-component

An Orbit Controls Component for A-Frame VR
https://tizzle.github.io/aframe-orbit-controls-component/
MIT License
75 stars 25 forks source link

Check for already at desiredPosition returns false positives sometimes #28

Open UXVirtual opened 7 years ago

UXVirtual commented 7 years ago

The below snippet in the update() method causes camera rotations near the desiredPosition to incorrectly match, preventing rotateTo() from firing:

if (!this.desiredPosition.equals(rotateToVec3)) {
        this.desiredPosition.copy(rotateToVec3);
        this.rotateTo(this.desiredPosition);
}

removing the !this.desiredPosition.equals(rotateToVec3) check results in more consistent behavior, with the exception that the camera will automatically rotate to face the very top of the model on load if rotateTo is not set.

tizzle commented 7 years ago

I guess we could remove the default value of the rotateTo and set it to undefined. This way the if-clause ⬆️ only fires when a value is present, which should be the intended behaviour. I tried this out and all the examples still work as expected. What do you think?

Edit: I dove into the rotateTo functionality a little and feel there are multiple things not ideal: First: the check in the update function shouldn't compare the rotateToVec3 and desiredPosition vector. I feel a comparison between desiredPosition and dolly.position (the actual position of the camera dolly) is better suited. Second: The example is causing some troubles in this. Whenever one of the three buttons is clicked, the component updates it's data for the rotateTo props. If this data is not different to what is in place, the component does not enter the update lifecycle. This means when i clicked the second button, wait for the rotateTo to finish, then move the camera by hand and click the second button again, it won't fire, because the manual camera movement did not invalidate the rotateTo data and the component does not enter the update cycle, as no data has changed.

May this behaviour be what you where referring to with this issue?

tizzle commented 7 years ago

After talking to dmarcos from the aframe team i made a branch using an exposed function to change the rotation, as this gets rid of some of the side-effects of the setAttribute approach. Feel free to check: https://github.com/tizzle/aframe-orbit-controls-component/tree/setRotateTo-function