trytriplex / triplex

The visual IDE for the web.
https://triplex.dev
GNU General Public License v3.0
804 stars 28 forks source link

Objects that are rendered to the scene are wrapped in groups to power scene lookups resulting in unexpected behaviour at times #72

Closed itsdouges closed 7 months ago

itsdouges commented 1 year ago

E.g. if a component shouldn't be rendered (material, geometry) but is a custom component that does not include material or geometry, it will be wrapped in a group and break things unexpectedly with either a cryptic error, or no error at all.

This issue should investigate if we can remove the need for wrapping groups in the first place which would fix a lot of the odd behaviour.

krispya commented 1 year ago

Could you explain this more? What is the added Object3D doing? Why does your system need it for its functionality? Links to relevant code would help too!

itsdouges commented 1 year ago

Yep (I'll record a video about this later tonight not on a computer atm).

So this system is used for selecting components in the three.js scene. The groups are added as wrappers for components that are rendered to the scene. These wrappers get meta data such as what file they are in and what their line and column numbers are.

This works well for being able to intelligently figure out when clicking on a object in the three.js scene, what component it belongs to, and approximate what object should be selected for transformation via transform controls.

The weakness to this system though is scene objects that aren't rendered to the scene such as materials, if wrapped in a group will throw an error during render phase. There is a naive workaround that checks jsx element names. If it ends in e.g. material it doesn't render the group.

Can explain more, will make the video soon.

krispya commented 1 year ago

This will also mess up anything that expects the scene to be a specific way. For example, someone might run logic that gets data from the parent.

Could we instead just map Object3D to an object struct with all the metadata? Each Object3D also has a uuid that can be mapped to, though I think mapping by reference is just as good.

itsdouges commented 1 year ago

When you click on anything in the scene this gets called https://github.com/try-triplex/triplex/blob/main/packages/scene/src/selection.tsx#L406

Which then calls findEditorData() here https://github.com/try-triplex/triplex/blob/main/packages/scene/src/selection.tsx#L423 - if something returns we then select it. Else bail out.

findEditorData() traverses up the three scene until it finds a group with triplex metadata https://github.com/try-triplex/triplex/blob/main/packages/scene/src/selection.tsx#L153. Inside isInScene() it makes sure only elements that are in the current "open" component are used. Anything else (e.g. other files) are ignored.

Once found it then traverses back down (in findTransformedSceneObject()) to find a three.js object3d that is the most likely one we're interested in given the current transform controls state (translate/scale/rotate).

The wrapping is done here https://github.com/try-triplex/triplex/blob/main/packages/scene/src/scene-object.tsx#L224. FYI __meta is data added from a Babel plugin. It adds data like this to every JSX element:

__meta={{
  "path": "/box.tsx", // file it belongs to
  "name": "group", // name of the jsx element, always a string so custom components would look like "Custom"
  "line": 2,
  "column": 7,
  "translate": false, // if this jsx element takes a position prop (or spread props)
  "rotate": false, // if this jsx element takes a rotation prop (or spread props)
  "scale": true // if this jsx element takes a scale prop (or spread props)
}}

The current system exists because we have no guarantee that any of the custom components have refs we can assign to. I'd love to get rid of it though. Potentially we can move to a React Context based implementation I just don't have a clear picture of how to do it ATM.

krispya commented 1 year ago

I'm not sure I 100% understand but I think I am getting there. It seems to me that you need to map JSX elements and React components to Three Object3D so that when you raycast and pick out an Object3D in the Three scene, you can select the relevant React element in the editor. Is that correct?

I think a good place to start is to identify what we need to track and their relationships to each other. Seems to me there are:

Theoretically, we could trace the lineage Object3D --> FiberNode --> RC function --> JS/TS file.

itsdouges commented 1 year ago

Conceptually yes you're exactly right. Any thoughts on wiring it up? Is there any way we can abuse how r3f works to inject meta data even without having access to refs?

itsdouges commented 1 year ago

E.g can we make a custom three instance that proxies the direct child as "the ref" and keeps the parent child relationship the same as if it wasn't there.

I might explore this today.

krispya commented 1 year ago

My goals are to:

  1. Not rely on R3F so that this can work with all React nodes, including UI (DOM) elements.
  2. Not modify the Three or React virtual tree at all so that logic runs as expected.

Injecting metadata into the JSX element is an interesting idea, but it means you miss anything that is a React component with no JSX, which makes the editor only aware of RCs that render something declaratively. I don't know anything about Babel, would you be able to inject _medata into all RCs props? Every FiberNode has a property called memoizedProps which is all of the user props. You would be able to sniff something injected there from the code via the props.

R3F injects onto the instance object ___r3f, but they are able to do this because they have access to the reconciler.

For a look at how to access the FiberNode check out Cody's lib. The source is pretty simple: https://github.com/pmndrs/its-fine

And here is an example of me using it: https://codesandbox.io/s/context-proxy-5f29ms?file=/src/App.tsx

krispya commented 1 year ago

Yeah I was just reviewing this. FiberNode also has elementType which will tell you if it is a RC funcion or a JSX intrinsic. We can then map Object3D to FiberNode and when doing raycasting, just look up its node real quick. With the node we can, hopefully, get whatever metadata we need for editor usage.

itsdouges commented 1 year ago

Is that on the r3f metadata? What's the chance of breakage?

krispya commented 1 year ago

FiberNode and its properties are well established React internals. They wonโ€™t break without a major.

itsdouges commented 1 year ago

@drcmda @CodyJasonBennett any thoughts on how to have metadata be added to the three.js scene in a resilient way to link its owning jsx element? Currently Triplex uses an extra group added at runtime which, for all intents, works for:

Basically it transforms this:

<CustomComponent>
  <mesh />
</CustomComponent>

to this:

<group userData={{ __triplexMeta: ... }}>
  <CustomComponent>
    <group userData={{ __triplexMeta: ... }}>
      <mesh/>
    </group>
  </CustomComponent>
</group>

And connects the scene to jsx elements well... but comes with some issues listed above. Could there be a solution which adds to the r3f instance state instead?

Which makes it very easy to tie r3f elements back to the owning jsx element. Any help would be appreciated, we're looking for a resilient solution that ticks all the boxes.

CodyJasonBennett commented 1 year ago

These wrappers get meta data such as what file they are in and what their line and column numbers are

JSX contains this metadata in dev, but any compiler/lexer will have this.

Is that on the r3f metadata? What's the chance of breakage?

I would not be modifying the runtime as you will see fail cases with concurrency where subtrees are cloned and can be discarded.

krispya commented 1 year ago

I really think we should avoid relying on R3F details for this. Using plain props that can be sniffed in the FiberNode means any React component is targetable by Triplex, not just the ones R3F is aware of. For example, the Html component which renders to its own root.

itsdouges commented 1 year ago

It's only on scene look up for this, so it fundamentally can't work for both DOM and r3f (literally interacting with the scene and then tying it back to its jsx element / custom component owner).

Id rather lean in r3f internals in the sense that we can control them, vs. react which we can't really.

Also Cody the group being added around jsx elements is concurrent mode stable. It originates as a bagel plugin that wraps every jsx element with a custom component (SceneObject) which I linked the code to above.

krispya commented 1 year ago

I think this is more foundational though. It provides a standardized way to query any React component. For example, there are many valid RCs in the R3F ecosystem and even JSX that are not scene objects. By trying to make it scoped to only what can be raycasted you will come up with a solution that will have to change again anyways. Otherwise what you have is just fine.

itsdouges commented 1 year ago

What we currently have atm isn't fine long term though, I'm sure we all agree on that!

If we don't add metadata to the three.js scene I can't see how it's possible to link up custom components (e.g raycast on a three.js scene object and know what custom component rendered it), especially those not owned by source code. Currently that's all possible with the current (not ideal) system.

If we start from the three.js scene and work backwards what were you thinking for tying the scene object back with react internals?

CodyJasonBennett commented 1 year ago

Am I understanding correctly that you want to be able to select Object3Ds and relate them and their children to source code?

itsdouges commented 1 year ago

Source code, yeah.

So for example click a mesh, and then be able to tie it back to the owning component / line + col + filepath if it's first party code.

I really need to sit down and make a video to walk through the current system it will make much more sense. Will try tonight.

CodyJasonBennett commented 1 year ago

This will work at runtime, but I might prefer to intercept the ref prop as part of your JSX transform to weakly track all elements.

import { Component } from 'react'
import { traverseFiber } from 'its-fine'
import { createRoot } from 'react-dom/client'

let fiber = null

class FiberInspector extends Component {
  static getDebugSource(instance) {
    return traverseFiber(fiber, false, (node) => node.stateNode === instance)?._debugOwner?._debugSource
  }

  render() {
    fiber = this._reactInternals
    return this.props.children
  }
}

function Test() {
  return <div ref={(self) => console.log(FiberInspector.getDebugSource(self))} />
}

createRoot(root).render(
  <FiberInspector>
    <Test />
  </FiberInspector>,
)
krispya commented 1 year ago

Using the FiberNode, like Cody is here, is what I had in mind, with the addition of caching all the results in a map so you can just do const metadata = metaDataStore.get(sceneObject). The fiber API is pretty stable, I'm not sure what you are afraid of happening from using it? With scene objects mapped to FiberNodes you can also do things like track what context each requires so they can be automatically shimmed. Some kind of editor DSL might be helpful too.

Although, note that the _debug properties only work in dev mode. That's why I figured it would be best to write this metadata yourself into the props, but also if you can run the editor only in dev mode this should work great!

itsdouges commented 1 year ago

@CodyJasonBennett RE:

but I might prefer to intercept the ref prop as part of your JSX transform to weakly track all elements.

Does that work for custom components that don't forward ref? Can you go into a bit of detail around this please ๐Ÿ™

Thanks for the example code.

itsdouges commented 1 year ago

Here's a video going over the scene object selection, after watching let me know if you have any follow up questions!

https://www.youtube.com/watch?v=9NRgtjG7YUw

(YT still processing might take a bit)

CodyJasonBennett commented 1 year ago

Does that work for custom components that don't forward ref? Can you go into a bit of detail around this please

Yes, since it will capture all host components or returned children. If you have a custom JSX transform, this info should already be available.

// Currently rendering component (if any)
//   _owner: Fiber | null
// Line, column, and file path
//   _source?: DebugInfo

function Component() {
  console.log(<div />)
}
itsdouges commented 1 year ago

Ah okay, interesting! I'll take a look soon. Thanks Cody / Kris ๐Ÿ™.

itsdouges commented 9 months ago

@kenjinp procgen terrain is impacted by this implementation.

itsdouges commented 8 months ago

This also affects decals (from drei) on meshes.

CodyJasonBennett commented 8 months ago

Where did scene-object.tsx of https://github.com/try-triplex/triplex/issues/72#issuecomment-1683310432 go? I can try to prototype a Fiber based one.

itsdouges commented 7 months ago

This has been fixed by https://github.com/try-triplex/triplex/commit/64f3cd92f3394e12bdf891f7196b8cab0da2fa24 and will be released in the next version.

The intermediate groups are gone now.