NodeOps shares `scene` across multiple`TresCanvas`s

andretchen0 commented 8 months ago

Bug

NodeOps accidentally shares scene across multiple TresCanvass.

See the StackBlitz reproduction for details.

Problem

NodeOps's let scene is set whenever a new Scene is added to any TresCanvas – i.e., the variable points to the last Scene added to a TresCanvas. If elements from another TresCanvas read scene in nodeOps, the code expects that they will find their Scene, but they do not.

Solution?

Solution A

We could refactor nodeOps as (context: TresContext) => NodeOps.

Every new instance of TresCanvas could create a new nodeOps instance which contains a unique TresContext (i.e., with the appropriate Scene) within its scope.

Solution B

Otherwise, we could simply recurse up the parent chain until we reach a Scene or run out of parents.

Reproduction

https://stackblitz.com/edit/tresjs-basic-e6uzmm?file=src%2FApp.vue

System Info

System:
    OS: Linux 5.0 undefined
    CPU: (8) x64 Intel(R) Core(TM) i9-9880H CPU @ 2.30GHz
    Memory: 0 Bytes / 0 Bytes
    Shell: 1.0 - /bin/jsh
  Binaries:
    Node: 18.18.0 - /usr/local/bin/node
    Yarn: 1.22.19 - /usr/local/bin/yarn
    npm: 10.2.3 - /usr/local/bin/npm
    pnpm: 8.14.0 - /usr/local/bin/pnpm
  npmPackages:
    @tresjs/cientos: ^3.5.1 => 3.5.1 
    @tresjs/core: ^3.4.1 => 3.4.1 
    @tresjs/eslint-config-vue: ^0.2.1 => 0.2.1 
    vite: ^4.5.0 => 4.5.0

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.

alvarosabu commented 8 months ago

Brilliant catch @andretchen0 thanks for the detailed reproduction!

Every new instance of TresCanvas could create a new nodeOps instance that contains a unique TresContext (i.e., with the appropriate Scene) within its scope.

This was the original purpose, but it seems we didn't implement it correctly, I think your idea about refactoring nodeOps to a function will potentially solve this, but I will need to test it

The weirdest thing is that I managed to reproduce the behavior only in stackblitz, in the playground we have a demo for multiple canvases, and it is not happening @andretchen0.

I'm using a v-if to show/hide the box, here is working as expected

https://github.com/Tresjs/tres/assets/4699008/8c9a313a-47a8-4d70-9a92-a4b1f10c478a

alvarosabu commented 8 months ago

I spent more time reproducing it and the only reason why was working on the playground it was because it was wrapped with Suspense wtf 😂

andretchen0 commented 8 months ago

This was the original purpose, but it seems we didn't implement it correctly

No worries. Bugs happen.

In the hope that it's helpful to mention: looking at the code, I have a hunch that some of the Vue SFC model has rubbed off on the way some plain ES modules were written. (It happens to me, so I'm assuming others, too.) Like the code in src/core/renderer.ts:

import * as THREE from 'three'

import { createRenderer } from 'vue'
import { extend } from './catalogue'
import { nodeOps } from './nodeOps'

export const { render } = createRenderer(nodeOps)

If one has the Vue <script setup> mental model, it might look like render will be unique every time the script is imported. Because every time you create <MyVueComponent>, its <script setup> is run again.

But ES modules are something like singletons. They're only evaluated the first time they're imported. In this case, it means all render are the same.

Mostly that wasn't a problem because mostly the Vue renderer (nodeOps) functions just work on the arguments passed to them ... just not in the case of let scene. Lol.

Tresjs / tres