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

Enable directly using custom editable elements through the editable object #240

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

AndrewPrifer
Copy link
Contributor

@AndrewPrifer AndrewPrifer commented Jun 29, 2022

Solution 3 for the need to create ad-hoc editable r3f components outside what's defined in the schema.

This solution allows you to directly use r3f elements not defined in the schema. Also works with TypeScript.

Example:

import {editable as e, SheetProvider} from '@theatre/r3f'
import {getProject} from '@theatre/core'
import React from 'react'
import type {Object3DNode} from '@react-three/fiber'
import {Canvas, extend} from '@react-three/fiber'
import type {BufferGeometry, Material} from 'three'
import {Mesh} from 'three'

class MyMesh extends Mesh {
  constructor(geometry: BufferGeometry, material: Material) {
    super(geometry, material)
    this.name = 'MyMesh'
  }
}

extend({MyMesh})

interface MyElements {
  myMesh: Object3DNode<MyMesh, typeof MyMesh>
}

// Augment the JSX.IntrinsicElements interface (only necessary if you also use myMesh without <e.)
declare global {
  namespace JSX {
    interface IntrinsicElements extends MyElements {}
  }
}

// Augment the ThreeElements interface. ThreeElements is the subset of JSX.IntrinsicElements used by r3f.
// See explanation/caveats below.
declare module '@theatre/r3f' {
  interface ThreeElements extends MyElements {}
}

function App() {
  return (
    <div
      style={{
        height: '100vh',
      }}
    >
      <Canvas
        dpr={[1.5, 2]}
        linear
        gl={{preserveDrawingBuffer: true}}
        frameloop="demand"
      >
        <SheetProvider sheet={getProject('Space').sheet('Scene')}>
          // Since myMesh is not defined in the schema, we need to provide an editableType that _does_ exist in the schema.
          // myMesh will be handled by theatre as if it were a mesh.
          <e.myMesh uniqueName="myMesh" editableType="mesh">
            <torusKnotGeometry args={[1, 0.3, 128, 64]} />
            <meshNormalMaterial />
          </e.myMesh>
          <ambientLight intensity={0.75} />
        </SheetProvider>
      </Canvas>
    </div>
  )
}

export default App

This solution works by checking if the referenced property is defined in the schema. If not, TypeScript will warn you to add the editableType prop, which must be a valid editable type defined in the schema. Theatre will treat your custom element as if it were the provided type.

What is the ThreeElements interface? In order to be able to provide auto-complete for valid r3f elements, it is necessary to have a catalogue of just the r3f elements. Currently, r3f doesn't provide this catalogue, instead extending JSX.IntrinsicElements directly. This interface represents what r3f should itself expose in the future to accommodate such use cases.

Blockers/caveats for this PR

While this solution seems to work without issues both in terms of functionality and in terms of DX, there are a couple things I'd prefer to wait for before merging it:

  • The ThreeElements interface should be something exposed by r3f. Manually replicating this interface couples us too tight to specific versions, and also introduces quite a maintenance burden.
  • Being able to directly reference elements on the editable objects that are not defined in the schema should in my opinion be opt-in. It would be confusing to new users that for some editable elements you don't need to provide editableType, while for some you do. How this opt in should happen in the future is through the editable schema (the thing that defines all editable types and how theatre should handle them, which I keep referring to in this PR). Since it is not clear yet what API exactly this schema should provide when we expose it to users, we might need to look for an alternative way to configure this for now.

@vercel
Copy link

vercel bot commented Jun 29, 2022

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated
theatre-playground ✅ Ready (Inspect) Visit Preview Jun 29, 2022 at 10:37AM (UTC)

@AndrewPrifer AndrewPrifer requested a review from AriaMinaei June 29, 2022 11:08
@AndrewPrifer
Copy link
Contributor Author

@donmccurdy could you please take a look at this and see if it solves your problem re: custom editables without having to mess with the schema?

@donmccurdy
Copy link
Contributor

@AndrewPrifer thanks! Nothing here I disagree with, but it's possibly more API surface than is necessary for our use case if you would rather pare that back. We need to support custom three.js objects, but not necessarily custom R3F elements. Using e.primitive gives us a lot of mileage, as our React codebase doesn't necessarily have knowledge of every incoming object type.

Ideally something like this should work — regardless of what userlandObject is, just as long as it extends THREE.Mesh.

function UserlandObject ({name, userlandObject}) {
  return <e.primitive uniqueName={name} object={userlandObject} editableType={'mesh'} />;
}

Another issue I'm wondering about ... the snapshot window clones the scene, does that have implications for these userland/custom objects? Do we just need to guarantee the objects implement .clone() properly?

/cc @juniorxsound does this all sound correct?

@AndrewPrifer
Copy link
Contributor Author

@donmccurdy there are indeed a couple easy ways already to accommodate custom threejs objects. e.primitive would be one, or if you want to control how theatre treats these objects, the following was already possible: extend({ MyMesh }); const MyEditableMesh = editable('myMesh', 'mesh');, I just needed to accommodate it in typescript as well (see #237).

The rationale for this PR was that from our talks it seemed like the requirement was not to just make this possible, but specifically to make it seamless. @juniorxsound @donmccurdy can you think of something that could better accommodate your use-case in this sense?

I do like this implementation because while it does slightly extend the API surface, it is fully backwards-compatible, would be opt-in, and for users who are fully committed to r3f and would use extend({...}) anyway, it is completely free as soon as I get r3f to expose the ThreeElements type (which would be super useful anyway not just for us, but for react-spring as well, whose types are completely broken right now because of the type complexity React.ElementType introduces).

Unless you think there could be other solutions here with better ergonomics (which I'm specifically targeting as my goal), I'd be inclined to merge this.

As for coining the scene, yes, the only requirement should be that you implement .clone(), we even peer-depend on threejs so that nothing breaks in the transition between your setup and the editor.

@donmccurdy
Copy link
Contributor

I'll defer to @juniorxsound on the remaining questions of API surface. As background, our use cases involve writing a limited amount of React / R3F code up front for tooling that is reused by many more end-users who will create three.js objects without interacting with the React codebase at all. So IMHO we can afford to declare the theatre/studio sheet object properties exhaustively ourselves, seamlessness there is not critical. Whereas doing something reasonable with current and future 'new' three.js objects (even if it's just exposing pos/rot/scale) without breaking is crucial.

As for coining the scene, yes, the only requirement should be that you implement .clone(), we even peer-depend on threejs so that nothing breaks in the transition between your setup and the editor.

I have to admit I'd prefer that theatre/r3f append a managed THREE.Group in its own layer to the existing scene, containing 3D UI elements and helpers for the snapshot view, rather than cloning a potentially large scene. Realtime sync would be easy in that case. But I'm off topic for the thread and this isn't currently a blocker for us. :)

@juniorxsound
Copy link
Contributor

Hi 👋 sorry for jumping into the discussion late.

Echoing @donmccurdy I think smaller API surface might be a beneficial thing overall (and for what we need, the <e.primitive> approach seems to be working well). Two potential use cases I do see for being able to extend the editable type is

  • Being able to add a custom helper to an editable object in the snapshot viewer
  • Being able to add an icon (similar to the camera icon) for invisible objects in the snapshot viewer

This is a little bit of a tangent to what's proposed above but might be worth considering?

@AndrewPrifer
Copy link
Contributor Author

@juniorxsound all of what you mention is going to be enabled by #166. This PR here is only for convenience. From our discussions I gathered that the idea was to enable seamless use of custom objects without any upfront configuration, hence this PR, but since you don't seem to need it right now, I'm going to sit on it a bit more. :)

@donmccurdy one of the first things on my plate is going to be a real-time architecture, I completely agree that it would bring massive value.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants