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

Use default slot props to get Canvas context #572

Closed
4 tasks done
Tracked by #541
alvarosabu opened this issue Mar 4, 2024 · 14 comments · Fixed by #712
Closed
4 tasks done
Tracked by #541

Use default slot props to get Canvas context #572

alvarosabu opened this issue Mar 4, 2024 · 14 comments · Fixed by #712
Labels
has-workaround wontfix This will not be worked on

Comments

@alvarosabu
Copy link
Member

alvarosabu commented Mar 4, 2024

Description

Idea from @userquin, the idea would be to use Vue named scoped slots to be able to get the canvas context on the parent like this:

<TresCanvas shadows alpha v-slot="state">
  <TresPerspectiveCamera :position="[5, 5, 5]" />
  <TresOrbitControls
    v-if="state?.renderer"
    :args="[state?.camera, state?.renderer?.domElement]"
  />
</TresCanvas>

Or even deconstruct it

<TresCanvas shadows alpha v-slot="{ camera, renderer }">
  <TresPerspectiveCamera :position="[5, 5, 5]" />
  <TresOrbitControls
    v-if="renderer"
    :args="[camera, renderer.domElement]"
  />
</TresCanvas>

Suggested solution

Add this to TresCanvas.vue

<canvas
    ref="canvas"
    :data-scene="scene.uuid"
    :class="$attrs.class"
    :data-tres="`tresjs ${pkg.version}`"
    :style="{
      display: 'block',
      width: '100%',
      height: '100%',
      position: windowSize ? 'fixed' : 'relative',
      top: 0,
      left: 0,
      pointerEvents: 'auto',
      touchAction: 'none',
      ...$attrs.style as Object,
    }"
 >
   <slot v-bind="context" />
</canvas>

Alternative

No response

Additional context

related #565

Validations

@alvarosabu alvarosabu mentioned this issue Mar 4, 2024
Closed
13 tasks
@userquin userquin changed the title Use named scoped slots to get Canvas context Use default slot props to get Canvas context Mar 4, 2024
@andretchen0
Copy link
Contributor

andretchen0 commented Mar 4, 2024

Is this a duplicate of #573 ? If so, can we close there?

Edit: Closed #573.

@userquin
Copy link
Member

userquin commented Mar 4, 2024

it is duplicated, #573 has the hack (can be closed)

@andretchen0
Copy link
Contributor

@alvarosabu @userquin

Are we talking about simply getting e.g., the renderer? Or also setting?

@userquin
Copy link
Member

userquin commented Mar 4, 2024

it is only about using the context via default slot props, rn the example is not working since state is undefined, cannot use useTresContext since it can be only used via custom TresCanvas sfc children. Check linked issue and the hack in #573

@andretchen0
Copy link
Contributor

it is only about using the context

So it's only for getting, right?

@userquin
Copy link
Member

userquin commented Mar 5, 2024

Yes (like inject or exposed context prop)

@andretchen0
Copy link
Contributor

Can we use defineExpose instead?

We're using defineExpose for this sort of thing in Cientos and Tres most other places. Is there a reason it won't work here?

I do think it would be a welcome improvement to simplify the current TresCanvas' context defineExpose for v4 so that accessing the useful bits doesn't require a Demeter violation.

Any thoughts/opinions on using and simplifying defineExpose here?


<slot>

Since we're not setting, only getting, I'm not sure about using the slot as suggested.

From the Vue docs:

We have learned that components can accept props, which can be JavaScript values of any type. But how about template content? In some cases, we may want to pass a template fragment to a child component, and let the child component render the fragment within its own template.

But we're not passing info to a child here, so it feels like a mismatch to use a <slot> to me.

@userquin
Copy link
Member

userquin commented Mar 5, 2024

Rn context is exposed via defineExpose, check the hack, maybe we only need to update the code snippet.

@userquin
Copy link
Member

userquin commented Mar 5, 2024

About the default slot: the problem is the custom renderer, it is adding custom stuff in the default slot, I will ask Kevin if we can retarget the slot using custom setup component when using the custom renderer.

@userquin
Copy link
Member

userquin commented Mar 5, 2024

@alvarosabu @andretchen0 using default slot props in ExtendExample:

imagen

imagen

imagen

imagen

@userquin
Copy link
Member

userquin commented Mar 5, 2024

Here the changes, update TresCanvas.vue in root and run build script from root, then start docs dev server once ExtendExample.vue and extending.md files updated:

src/components/TresCanvas.vue
// L84
const slots = defineSlots<{
  default(context: TresContext): any
}>()

const instance = getCurrentInstance()?.appContext.app

const createInternalComponent = (context: TresContext) =>
  defineComponent({
    setup() {
      const ctx = getCurrentInstance()?.appContext
      if (ctx) ctx.app = instance as App
      provide('useTres', context)
      provide('extend', extend)

      if (typeof window !== 'undefined' && ctx?.app) {
        registerTresDevtools(ctx.app, context)
      }
      return () => h(Fragment, null, slots?.default ? slots.default(context) : [])
    },
  })
docs/.vitepress/theme/components/ExtendExample.vue
<script setup lang="ts">
import { TresCanvas, extend } from '@tresjs/core'
import { OrbitControls } from 'three/addons/controls/OrbitControls'
import { TextGeometry } from 'three/addons/geometries/TextGeometry'

// Add the element to the catalogue
extend({ TextGeometry, OrbitControls })
// import { useTresContext } from '@tresjs/core'

const styles = {
  width: '100%',
  height: '550px',
  border: '1px solid #e2e2e2',
  borderRadius: '8px',
  overflow: 'hidden',
}

// const { camera, renderer } = useTresContext()
</script>

<template>
  <ClientOnly>
    <TresCanvas
      v-slot="{ camera, renderer }"
      shadows
      xwindow-size
      clear-color="#fff"
      :style="styles"
    >
      <TresPerspectiveCamera :position="[0, 2, 4]" />
      <TresScene>
        <TresOrbitControls
          v-if="camera.value && renderer.value"
          :args="[camera.value, renderer.value.domElement]"
        />

        <TresDirectionalLight
          :position="[0, 2, 4]"
          :intensity="2"
          cast-shadow
        />
        <TresMesh :rotation="[-Math.PI / 4, -Math.PI / 4, Math.PI / 4]">
          <TresTorusGeometry :args="[1, 0.5, 16, 32]" />
          <TresMeshToonMaterial color="#FBB03B" />
        </TresMesh>
      </TresScene>
    </TresCanvas>
  </ClientOnly>
</template>
docs/advanced/extending.md
# L36
<ClientOnly>
    <div style="aspect-ratio: 16/9; height: auto; margin: 2rem 0; border-radius: 8px; overflow:hidden;">
        <ExtendExample />
    </div>
</ClientOnly>

@andretchen0
Copy link
Contributor

andretchen0 commented Mar 5, 2024

Hey @alvarosabu @userquin !

Rn context is exposed via defineExpose, check the hack

Sorry, I wasn't clear earlier.

I know that Tres is currently using defineExpose to make context available. I was wondering if cleaning up context and continuing to use defineExpose would be an acceptable alternative here.

That would be my preference.

Rationale

The example of bad DX from #573 is indeed a bummer:

<script setup>
// [...]
const trescanvas = ref();
const state = ref();

onMounted(() => {
  setTimeout(() => {
    state.value = trescanvas.value?.context;
  }, 0);
});
// [...]
</script>

<template>
  <TresCanvas ref="trescanvas" shadows alpha>
    <TresPerspectiveCamera :position="[5, 5, 5]" />
    <TresOrbitControls
      v-if="state?.renderer"
      :args="[state?.camera, state?.renderer?.domElement]"
    />
    <!-- -->
  </TresCanvas>
</template>

But as far as I can see, it can be rewritten more simply and cover the presented use case. See below.

It seems to me that the code below isn't materially different from the bad DX example. It only needs one extra line in <script setup> – a ref. It works with the current main branch of Tres. (Edit: It doesn't allow destructuring as written though.)

(Fwiw, extend in the latest release is throwing errors for me, so I can't recreate the bad DX example as written, but I've tried to recreate the salient parts without extend below.)

<script setup lang="ts">
import { TresCanvas } from '@tresjs/core'
import { ref } from 'vue'

// NOTE: TS likes to know the "shape" or the editor will complain.
const r = ref({ context: { renderer: { value: null } } }) 
</script>

<template>
  <div style="height:50%">
    <TresCanvas ref="r" clear-color="#82DBC5">
      <TresMesh>
        <TresBoxGeometry :args="[1, 1, 1]" />
        <TresMeshNormalMaterial />
      </TresMesh>
    </TresCanvas>
  </div>
  <div>
    <div v-if="r.context.renderer.value">
      We have a renderer: {{ Object.keys(r.context.renderer.value) }}
    </div>
    <div v-else>
      We don't have a renderer.
    </div>
  </div>
</template>

StackBlitz

Preference: defineExpose

If I've understood the issue here correctly and if it's true that we can reduce the bad DX example to one extra line of code, my preference would be to stick with defineExpose and reserve <slot>s for use cases as defined in the Vue docs, e.g., we have a template fragment to send to a child for rendering.

Maybe we could smooth out what's defineExposed for v4 so that we can e.g., just use r.renderer in the code above.

Moving forward

I don't want to impede forward progress. If I've misunderstood something here, please let me know. Otherwise, @alvarosabu , I'll leave the final call up to you.

@userquin
Copy link
Member

userquin commented Mar 6, 2024

What's the problem using the default slot and exposing props? We can also add some logic to render children when camera and renderer there and provide just the camera and renderer without refs.

@andretchen0
Copy link
Contributor

What's the problem using the default slot

If you're responding to me, then please see my earlier comment.

We can also add some logic to render children when camera and renderer there and provide just the camera and renderer without refs.

I don't think it's a bad idea, but it has much bigger implications than this DX issue.

I'd like to let the team have a chance to discuss this in a broader context, so I've opened a new discussion about the core and mentioned you there. #578 I've laid out what I think are the related open issues, including this one.

@alvarosabu alvarosabu added p2-to-be-discussed Enhancement under consideration (priority) and removed enhancement v4 labels Mar 28, 2024
@alvarosabu alvarosabu added wontfix This will not be worked on has-workaround and removed p2-to-be-discussed Enhancement under consideration (priority) labels Jun 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
has-workaround wontfix This will not be worked on
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

3 participants