Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: 393 lod #462

Closed
wants to merge 138 commits into from
Closed

feat: 393 lod #462

wants to merge 138 commits into from

Conversation

JaimeTorrealba
Copy link
Member

@JaimeTorrealba JaimeTorrealba commented Aug 7, 2024

close #393
Hi, I choose a different approach from r3f (because I personally don't like so much the details component or how it works in drei). Please feel free to give feedback.

Note: right now the component is not reactive (what do you think about this component been reactive anyway, I believe just for ... is useful) the reason for this is that is a little difficult to handle the remove-create new levels because there's no removeLevel method in LOD class.

I create a new issue in Threejs mrdoob/three.js#29074 to addres this one.

TWO questions.

  1. Should I merge this one for v4? Or wait until v4 is merged to main?
  2. Should I create a new section of performarce in cientos? We have some component that could be moved there (stats, statsGl, bakedShadow, etc.)

andretchen0 and others added 30 commits January 11, 2024 22:31
Copy link

stackblitz bot commented Aug 7, 2024

Review PR in StackBlitz Codeflow Run & review this pull request in StackBlitz Codeflow.

@JaimeTorrealba JaimeTorrealba changed the base branch from v4 to main August 24, 2024 15:39
Copy link

netlify bot commented Aug 24, 2024

Deploy Preview for cientos-tresjs failed.

Name Link
🔨 Latest commit 5ceca1a
🔍 Latest deploy log https://app.netlify.com/sites/cientos-tresjs/deploys/670434632c71750008292cd8

Copy link
Member

@alvarosabu alvarosabu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is exciting, nice addition @JaimeTorrealba

docs/guide/misc/lod.md Outdated Show resolved Hide resolved
docs/guide/misc/lod.md Outdated Show resolved Hide resolved
docs/guide/misc/lod.md Outdated Show resolved Hide resolved
src/core/misc/LOD.vue Show resolved Hide resolved
@JaimeTorrealba JaimeTorrealba self-assigned this Sep 16, 2024
@JaimeTorrealba JaimeTorrealba added the p2-nice-to-have Not breaking anything but nice to have (priority) label Sep 16, 2024
@alvarosabu
Copy link
Member

Hey @JaimeTorrealba when you have a chance can you check the conflicts, I think we are good to go after that

@JaimeTorrealba
Copy link
Member Author

@alvarosabu Please mark as resolved all the comments and give me an approval if you think is ready, I already solved the conflicts.

@alvarosabu
Copy link
Member

Hi @JaimeTorrealba when you are back can you please check the conflicts and check why the docs and the linter are broken? Thanks

Copy link
Contributor

@andretchen0 andretchen0 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @JaimeTorrealba ,

Sorry for the delay here.

I think this is a welcome feature for Cientos.

But I think there need to be some changes to the API and the implementation to allow multiple LODs.

See comments.

Copy link
Contributor

@andretchen0 andretchen0 Oct 17, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comparison with Drei's API

I think the Drei API avoids a fair amount of repetition.

Drei

<Detailed distances={[0, 10, 20]} {...props}>
  <mesh geometry={highDetail} />
  <mesh geometry={mediumDetail} />
  <mesh geometry={lowDetail} />
</Detailed>

Cientos

<LOD :distance="0" v-bind="props">
  <TresMesh :geometry="highDetail">
</LOD>

<LOD :distance="10" v-bind="props">
  <TresMesh :geometry="mediumDetail">
</LOD>

<LOD :distance="20" v-bind="props">
  <TresMesh :geometry="lowDetail">
</LOD>

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi thanks for going so in deep with this, my implementation here was different for the R3F, because I don't feel intuitive how they handle it, I think it is very confusing an error-prone to match the LOD for the order. That is why I propose something different :)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the R3F approach is a good compromise.

Matching distances to children like below is a bit of a pain, but I think it's doable:

<Detailed distances={[0, 10, 20]} {...props}>
  <mesh geometry={highDetail} />
  <mesh geometry={mediumDetail} />
  <mesh geometry={lowDetail} />
</Detailed>

I think the proposed API has bigger problems.

Breaks the usual Tres mapping: 1 <LOD /> maps to 1 THREE.LOD

In the THREE source, every THREE.LOD is an Object3D. For most Object3Ds, Tres/Cientos maps 1 template tag to 1 Object3D. That would look something like the R3F API where an <LOD> has children for each "level".

But the proposed API here has multiple <LOD /> tags for a single THREE.LOD. That leads to problems like reparenting.

Record keeping

We need to be able to have multiple THREE.LODs. Since many <LOD />s map to 1 THREE.LOD, you'll have to specify that mapping. Maybe like:

    <LOD id="lod-0"></LOD>
    <LOD id="lod-0"></LOD>

    <LOD id="lod-1"></LOD>
    <LOD id="lod-1"></LOD>

I find this to be more inconvenient than managing distances in the R3F API.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Understood, I'll close this PR for now, I would try some ideas here maybe re-open it in the future.

const LODInstance = shallowRef()
const wrapperRef = shallowRef()

LODInstance.value = seek(scene.value, 'type', 'LOD')
Copy link
Contributor

@andretchen0 andretchen0 Oct 17, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Implementation

Cientos probably needs to handle multiple THREE.LOD instances.

It seems to me that there's only ever a single THREE.LOD instance here, but unless I misunderstand how the THREE class works, a single instance won't work for scenes where some elements are close to the camera and some are far away.

And that's the main use case for supporting LOD, as far as I can see.

Looking at this THREE example code, there is a THREE.LOD instance for each object.

https://github.com/mrdoob/three.js/blob/37d6f280a5cd642e801469bb048f52300d31258e/examples/webgl_lod.html#L71

https://threejs.org/examples/#webgl_lod

const wrapperRef = shallowRef()

LODInstance.value = seek(scene.value, 'type', 'LOD')

Copy link
Contributor

@andretchen0 andretchen0 Oct 17, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

API

It seems to me that there's no way to signify that 2 Cientos LODs should be handled differently.

Below, I want Box and Sphere to have separate LOD settings and to be drawn on screen at the same time. But the result is that Box and Sphere are never on screen at the same time.

Screen.Recording.2024-10-17.at.14.05.29.mov

Here's what I tried:

<script setup lang="ts">
import { CameraControls, LOD, Sphere } from '@tresjs/cientos'
import { shallowRef, watch } from 'vue'
import { TresCanvas } from '@tresjs/core'
import { NoToneMapping, SRGBColorSpace } from 'three'

const gl = {
  clearColor: '#111',
  outputColorSpace: SRGBColorSpace,
  toneMapping: NoToneMapping,
}
</script>

<template>
  <TresCanvas v-bind="gl">

    <TresPerspectiveCamera :position="[0, 2, 5]" />
    <CameraControls :distance="50" />

    <LOD :distance="0" name="high">
      <Sphere name="sphere" :args="[1, 16]" :position="[-1, 0, 0]">
        <TresMeshNormalMaterial wireframe />
      </Sphere>
    </LOD>
    <LOD :distance="15" name="medium">
      <Sphere name="sphere" :args="[1, 8]" :position="[-1, 0, 0]">
        <TresMeshNormalMaterial wireframe />
      </Sphere>
    </LOD>
    <LOD :distance="30" name="low">
      <Sphere name="sphere" :args="[1, 4]" :position="[-1, 0, 0]">
        <TresMeshNormalMaterial wireframe />
      </Sphere>
    </LOD>

    <LOD :distance="0" :position="[0, 0, -10]">
      <TresMesh name="box" :position="[2, 0, 0]">
        <TresBoxGeometry :args="[1, 1, 1]" />
        <TresMeshBasicMaterial wireframe :color="0xFF0000" />
      </TresMesh>
    </LOD>
    <LOD :distance="5" :position="[0, 0, -10]">
      <TresMesh name="box" :position="[1, 0, 0]">
        <TresBoxGeometry :args="[1, 1, 1]" />
        <TresMeshBasicMaterial wireframe :color="0xFF0000" />
      </TresMesh>
    </LOD>

  </TresCanvas>
</template>

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can handle this differently, thanks for reaching this.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reparenting bug

The current implementation reparents objects wrapped with LOD.

The 2 visible objects in this scene should move, because they're in a group that moves. But the object in the LOD doesn't move.

Screen.Recording.2024-10-18.at.02.10.08.mov

Here's the code.

<script setup lang="ts">
import { CameraControls, LOD, Sphere } from '@tresjs/cientos'
import { shallowRef, watch } from 'vue'
import { TresCanvas } from '@tresjs/core'
import { NoToneMapping, SRGBColorSpace } from 'three'

const gl = {
  clearColor: '#111',
  outputColorSpace: SRGBColorSpace,
  toneMapping: NoToneMapping,
}

const groupRef = shallowRef({ position: { x: 0 } })
setInterval(() => groupRef.value.position.x = Math.cos(Date.now() * 0.001), 100)
</script>

<template>
  <TresCanvas v-bind="gl">
    <TresPerspectiveCamera :position="[0, 2, 5]" />
    <CameraControls :distance="50" />

    <TresGroup ref="groupRef">
      <TresMesh>
        <TresCylinderGeometry :args="[1, 1, 1]" />
        <TresMeshBasicMaterial wireframe :color="0xFF0000" />
      </TresMesh>
      <LOD :distance="100" :position="[0, 0, -10]">
        <TresMesh name="box" :position="[2, 0, 0]">
          <TresBoxGeometry :args="[1, 1, 1]" />
          <TresMeshBasicMaterial wireframe :color="0xFF0000" />
        </TresMesh>
      </LOD>
    </TresGroup>
  </TresCanvas>
</template>

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unmm, this behavior is only applied for the LOD? It comes for Tres or for Threejs?

There are other similar components that use as wrappers, for example scrollControls have you tried if this behavior is only for Tres and LOD? Or get repeated in other contexts?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @JaimeTorrealba ,

Unmm, this behavior is only applied for the LOD? It comes for Tres or for Threejs?

Afaik, the reparenting happens because the <LOD> implementation only ever creates one THREE.LOD instance. THREE.LOD is a subclass of Object3D. When the <LOD> children are added to the single THREE.LOD, they're removed from their original parent:

Here's the add in THREE.LOD code:

https://github.com/mrdoob/three.js/blob/beab9e845f9e5ae11d648f55b24a0e910b56a85a/src/objects/LOD.js#L71

Here's where Object3D.add(child) removes the child from its current parent:

https://github.com/mrdoob/three.js/blob/841ca14e89f3ec925e071a321958e49a883343c0/src/core/Object3D.js#L336

There are other similar components that use as wrappers

I don't know about the others.

But the behavior here doesn't match what I expected to happen.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
p2-nice-to-have Not breaking anything but nice to have (priority)
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

<LOD />
4 participants