OrbitControls has no update function, also shows null when accessed from context

zkobrinsky commented 1 year ago

Describe the bug

I'm trying to enableDamping on my OrbitControls and I'm unable to call controls.update in the render loop for two reasons: When using the Cientos OrbitControls, if I expose it with a ref it has no update function, and if I access it from context it returns null.

There's also a separate issue where orbitControls has no camera attached to it when you expose it unless you explicitly v-bind one to the component (also in stackBlitz attached).

Reproduction

https://stackblitz.com/edit/tresjs-basic-nj82xh?file=src%2Fcomponents%2FTheScene.vue

Steps to reproduce

No response

System Info

No response

Used Package Manager

npm

Code of Conduct

[X] I agree to follow this project's Code of Conduct
[X] Read the Contributing Guidelines.
[X] Read the docs.
[X] Check that there isn't already an issue that reports the same bug to avoid creating a duplicate.
[X] The provided reproduction is a minimal reproducible example of the bug.

stackblitz[bot] commented 1 year ago

Start a new pull request in StackBlitz Codeflow.

tresjs-basic-nj82xh (Open reproduction)

andretchen0 commented 1 year ago

@zkobrinsky Thanks for the issue with the reproduction link.

Context

Cientos' <OrbitControls /> doesn't currently expose a camera or the update method from the underlying three component, as far as I can see. Unlike a lot of other Cientos' components, <OrbitControls /> doesn't use Vue's defineExpose to expose a ref. Doing so would allow the camera and update() to be accessed like this:

[... in setup]
const controls = ref(null);

onMounted(() => {
    // Three's OrbitControls' camera is on the 'object' field:
    // https://threejs.org/docs/?q=camera#examples/en/controls/OrbitControls.object
    console.log(controls.value.value.object.isCamera) // true
    console.log(controls.value.value.update) // function update()
}

Proposed solution

@alvarosabu @JaimeTorrealba

To be consistent with other Cientos' components, we should expose a ref using defineExpose. I'll submit a PR shortly.

Feel free to ignore if you'd like to go a different direction here.

Background

@JaimeTorrealba was wondering about an alternative to using value.value, but there hasn't been any follow up.

andretchen0 commented 1 year ago

@alvarosabu

I've submitted a PR exposing the camera and update in Cientos' <OrbitControls />.

There's a separate TresJS issue here: useTresContext().controls is null. Can we transfer that to TresJS?

alvarosabu commented 1 year ago

Yes we should open it on the core package, thanks for taking care of it

zkobrinsky commented 1 year ago

Thanks for looking into this.

Tresjs / cientos