Html element flickering (new)

DenysAshikhin commented 1 year ago

Hi,

First of all, thank you for creating + maintaining this library, and being pretty quick + helpful when questions/issues arise. That being said, I have an issue that wasn't present before where the Html element flickers (gets rerendered?). Demo: https://youtu.be/JuEgflt4C60

This wasn't an issue before, I'm not sure when it started exactly but it wasn't always there until more recently. I tried recreating a code sandbox for this, but I couldn't get it working with the exact version of react, babylonjs and react-babylons. Here is the demo with older versions demo: https://codesandbox.io/s/suspicious-pond-5xjdw9

Current library versions:

 "react": "^18.2.0",
    "react-babylonjs": "^3.1.13",
"react-scripts": "^5.0.1",
"@babylonjs/core": "^5.27.1",
    "@babylonjs/gui": "^5.27.1",
    "@babylonjs/loaders": "^5.27.1"

That said, in the older demo, the bug doesn't seem to reproduce, but it should at least show what is going on in my code. Where I have a parent component that fetches data and passes it to my scene + individual models (which have and Html element much like the box in demo) that follows them around. However, every time it fetches the data there is a refresh that causes a flicker. In the demo, I use the setInterval to replicate the setState trigger, which does cause a rerender of the Html element, but it is no longer flickering.

My guess is maybe it has something to do with the library versions on the demo being older?

dennemark commented 1 year ago

I have just created a PR (independently of your issue :D) that changes the render hook of HTML from onBeforeRender to onAfterRender. Maybe this solves already the issue? https://github.com/brianzinn/react-babylonjs/pull/261 Otherwise I will need to have a look at it again.

Your sandbox does not show an html component.

DenysAshikhin commented 1 year ago

Really sorry about that, I updated the sandbox - also I would really appreciate if you can fix/tell me how to make the package versions I outlined above work in the sandbox!

dennemark commented 1 year ago

Can you reproduce it in a codesandbox? I updated dependencies and could not reproduce: https://codesandbox.io/s/affectionate-hypatia-w7rqz6?file=/src/App.tsx

But the way you describe it with the refetch, I am wondering, if the Html component is being remounted every time? Any chance you could keep the wrapping Html alive?

const ComponentThatFetches = ()=>{
const fetchedData = useQuery()

return <Html>
   <PopupDiv>
    {fetchedData && <MyInnerHtmlComponent data={fetchedData}/>}
   </PopupDiv>
</Html>
}

DenysAshikhin commented 1 year ago

When you say keep the wrapping Html alive, do you mean not behind a conditional render?

  <box
      name={name}
      ref={boxRef}
      size={1}
      position={internalPosition}
      scaling={scaling}
      width={1}
      height={1}
      depth={1}
      visibility={1}
    >
      <standardMaterial
        name={`${name}-mat`}
        diffuseColor={color}
        specularColor={Color3.Black()}
      />
      {/** Sits on top of outer box */}
      <box
        name={name}
        size={0.1}
        position={new Vector3(0, factoryDimensions.yShift ,factoryDimensions ? factoryDimensions.zMax * 0.065: 0)}
        visibility={0}
        color={new Color3(255, 0, 0)}
      >
            <Html
            name="html"
          >
            {
              <MachineStatus
              cameraMode={cameraMode}
                threeD={true}
                cameraRadius={cameraRadius}
                machine={machine}
                activeMachine={activeMachine}
                setActiveMachine={setActiveMachine}
                scalePoint={scalePoint}
                zoomSnap={zoomSnap}
                resetInfoHover={resetInfoHover}
              />
            }
          </Html>
      </box>
    </box>

That's what it looks like from my current code - so I don't think (?) I'm loosing the wrapping html

Update sandbox (just added a nested box to closer match what I am doing): https://codesandbox.io/s/blissful-lamport-jxeugm?file=/src/App.tsx However, it doesn't seem to be happening here...

dennemark commented 1 year ago

What happens, if you just display some div with text, instead of MachineStatus within that component? You are not using occlude on Html right? As soon as @brianzinn pushes the next version, we could try it with the updated Html component.

DenysAshikhin commented 1 year ago

Yea, I'm not using occlude. Also I tried simly:

  <Html name="html">
          <div style={{ width: "64px", height: "64px", backgroundColor:'white'}}>
Test
          </div>
   </Html>

and it's the same result. I've also added a few more conditional renders to better show how my code is structured - but I still can't cause it to flicker on the codepen (https://codesandbox.io/s/blissful-lamport-jxeugm?file=/src/App.tsx)

brianzinn commented 1 year ago

sorry I didn't publish earlier. can you try 3.1.14 with the render update?

DenysAshikhin commented 1 year ago

Sorry for the delay, but I managed to make the issue reproducible (with 3.1.14)! https://codesandbox.io/s/blissful-lamport-jxeugm?file=/src/App.tsx (same link).

Just keep scrolling out and moving your camera, I'm sure you'll see it

It seems as the number of html elements increases the flickering gets progressivley worse. However, even with 4 divs you can get the issue.

I'm hoping this isn't going to be a technical limitation, but something that can be optimised with relative easy (🙏 )

dennemark commented 1 year ago

Try this: https://codesandbox.io/s/priceless-khorana-uusur0?file=/src/App.tsx

It has its own implementation of Html. It uses Tunneling - I just named it this way, since it sends react components from one renderer to another... If you prefer a library, stick to tunnel-rats, but their version does not include hot module reloading as well as the order of Html Elements could change. You can also create a tunnel the other way around. Send babylon data from react-dom to react-babylonjs scene. It has benefits over the current Html implementation, since it does not create a react renderer for each html element, but places the elements within the existing react-dom renderer. But it has few more dependencies: zustand and a uuid generator (@thi.ng/random) to track each Html element

Some older version can be found in storybook or here: https://github.com/brianzinn/react-babylonjs/pull/168

DenysAshikhin commented 1 year ago

That looks promising! I tried to incoporate the solution as the extra libraries aren't a problem, however, I have never worked with typescript (comfortable with typings, just not typescript itself). And my whole project is just JS. I'm running into some issues trying to make it work.

I did my best in my codepen: https://codesandbox.io/s/blissful-lamport-jxeugm?file=/src/Html.jsx I would greatly appreciate it if you could do the final touches to show me where I'm going wrong!

DenysAshikhin commented 1 year ago

Just an update that I used this to auto convert the TS to JS. Did some code tweaking on my local machine and it worked!

I can mark this as solved, but I don't know if this would be considered a proper solution or a band-aid one?

Also a final question regarding the random generation of ID's. Can I get rid of that dependency if I have my own way of uniquely ID'ing each html element?

dennemark commented 1 year ago

Very nice! I tried to convert, but there were more issues than expected, so I had to postpone it. Nice that it worked.

Sure you can use your own random ID´s. I just used it out of convenience.

I guess it should be also possible to deal with the ids without use of external dependencies, i.e. just by using a counter. Anyhow would be nice to know, if @brianzinn would prefer to have additional dependencies (zustand? - its really small) within react-babylonjs or an additional package. Zustand could also be a good replacement, when using useScene or useEngine. Currently they cannot be consumed outside of Engine context. It would lead to issues though, if using multiple engines or scenes.

DenysAshikhin commented 1 year ago

Right, in my case each element is already tied to a unique ID so I don't strictly speaking need the uuid. But I will leave it anyway in case I need to use the HTML for something else. I won't mark it as resolved just yet as I would like to get @brianzinn opinion on it and we can close it then. Just for conveniency, my working (JS + TS hybrid repo) HERE And pure, clean TS from @dennemark HERE

brianzinn commented 1 year ago

I am open to zustand for sure. It's inconvenient to need to declare components in the Scene (to get Scene access), but like you said there is a trade-off and probably just need to doc some recipes. The other disadvantage of right now is needing to bridge across context boundaries, so I always recommend zustand over redux, for example.

DenysAshikhin commented 1 year ago

In that case, I'll leave this open for a few days in case I notice something else odd with this implementation. If not, I will silently close it. Thanks for all your help guys!

brianzinn commented 1 year ago

Looks to work well what you have @DenysAshikhin - just the setInterval once/second I initially thought was a performance issue! Were you not able to get it working with the <Html ../> component in the library?

DenysAshikhin commented 1 year ago

@brianzinn to confirm I am understanding correctly, you're asking if the react-babylonjs HTML component doesn't work for me? In which case that is correct. As in, when there is more than a handful of built-in htmls, it starts flickering as per video.

@dennemark looks like I have to bug you once more. While the solution does work, it introduces a new issue as per this video: https://youtu.be/Pxiwse1gaqc

I'm hoping it's a simpler fix since it seems like something is being put to x-0 y-0 absolute position. Without changing any of my code, I just used the react-babylonjs HTML and it doesn't do the weird moving, but the flicker does come back. https://youtu.be/0OJ3b5nHoQM

So I'm guessing it's something with either the new Html file or Tunnel. I did my best to recreate the exact scenario in the code pen, but I can't get it to happen. Essentialy, I have a value that tells me which html is active (and it opens up, the others become collapsed). And this get's changed when you click them. It happens during this change. But the codepen isn't showing that. I'm hoping maybe you can take a look at the code you provided and catch something I missed? Since I'm not grasping all of the intracies you've provided 😅

DenysAshikhin commented 1 year ago

@dennemark I would also be open to getting into a call + screen share since the code on my end isn't anything overly complex. And the problem happens right away, so perhaps we can do some on-the-spot logging to try and recreate it inside a code pen?

DenysAshikhin commented 1 year ago

I've narrowed the problem on my side to my div's being position: 'absolute'. However, this behavior is not being replicated in the code pen. When I remove the absolute then it fixes it from being moved to the bottom (0,0) but instead, it seems to take up space in a difference div (a scrollbar appears) and then immediately disappears.

My impression is when the div gets mounted/changed it gets placed outside the normal div? hierarchy which makes the DOM act funny, and then after the first render it is placed correctly. Again, I am very open to hopping into a call whenever you are available to better show this as I am struggling to recreate this interaction of (modified tunnel + HTML) in a small codepen.

dennemark commented 1 year ago

Hi Denys, these days I am a bit busy. Not sure when I find time. If I manage, I will let you know! The HTML element itself is placed as absolute element. It has a center={true} attribute to be centered. Maybe interesting for you?

In my case I have placed the tunnel out next to the engine within a relatively positioned div container, which should deal with absolute positioned elements in it. Roughly like the following code

<div
      id="engine-wrapper"
style={{
      position: 'relative',
      width: "100%",
      height: "100%",
      overflow: 'hidden'  //not sure if you need this
     zIndex: 1  //not sure if you need this
}}
    >
      <Engine>
{children}
</Engine>
<Tunnel.Out />
</div>

DenysAshikhin commented 1 year ago

Sorry for late response, got some other priorities at work. I have tried overflow: 'hidden' as per your suggestion and it helped in the sense that it's less jarring when he div get puts to 0,0 for a moment as it doesn't lead to the layout shifting and showing scroll bars.

I've also completely narrowed it down to the following:

  {(activeMachine === machine.ID) && (
          <Html
          name={`html-${machine.ID}`}
            zIndexRange={[96767552, 99767452]}
          >
            {
              <MachineStatus
                threeD={true}
                cameraMode={cameraMode}
                cameraRadius={cameraRadius}
                machine={machine}
                activeMachine={activeMachine}
                setActiveMachine={setActiveMachine}
                scalePoint={scalePoint}
                zoomSnap={zoomSnap}
                resetInfoHover={resetInfoHover}
              />
            }
          </Html>
        )}
        {activeMachine !== machine.ID && (
          <Html
            name={`html-${machine.ID}`}
          >
            {
              <MachineStatus
              cameraMode={cameraMode}
                threeD={true}
                cameraRadius={cameraRadius}
                machine={machine}
                activeMachine={activeMachine}
                setActiveMachine={setActiveMachine}
                scalePoint={scalePoint}
                zoomSnap={zoomSnap}
                resetInfoHover={resetInfoHover}
              />
            }
          </Html>
        )}

When the activeMachine variable is switched and the Html is switched, the window moves the new html down to the bottom left as per video.

I would be okay getting rid of this if I had a way to control the zIndex based on a switch (I need the activeMachine element to always render on top of all the others no matter their distance to the camera). And the issue is, if I use a basic x < y ? a : b where a and b are zIndex ranges.

So my question is this, if there is no way to loose reference(s) when switching active elements, is there a way to update the zIndex post initial loads so change which HTML element is on top?

brianzinn commented 1 year ago

You could potentially flow in the zIndex as a prop here - I am not familiar 100% with that code though.... https://github.com/brianzinn/react-babylonjs/blob/master/packages/react-babylonjs/src/customComponents/Html.tsx#L335 {zIndexProp ?? objectZIndex(...)} You'd need to add it to the dependency array for after render.

DefaultV commented 1 year ago

I've also experienced this issue, it seems to be when updating the root scene / engine component while having a <Html> component as a child. (I use a global variable context in my project)

...
const [clearColor, setClearColor] = useState<Color4>(
    Color4.FromHexString("#000000FF")
  );

setInterval(() => setClearColor(Color4.FromHexString("#000000FF")), 500);
...

<Scene clearColor={clearColor}>
...
 <Html name="testing" position={Vector3.ZeroReadOnly} id={"1"}>
              <div>Placeholder text goes here</div>
            </Html>
...
</Scene>

Reproducible branch: https://github.com/DefaultV/create-react-app-typescript-babylonjs/tree/bug-htmlflicker

Video:

https://user-images.githubusercontent.com/14123880/207896506-d46df015-d97a-4a36-9dc8-ba22b2206ce3.mp4

DenysAshikhin commented 1 year ago

Oh that looks good, and seems like it has another source of the issue, that's not just a ton of html's being rendered at once.

Also @brianzinn I am still waiting to implement your suggestion, work just shifted some priorities around so I'm fighting to get some time to return to this

dennemark commented 1 year ago

@DefaultV does this also happen, when you place the Html in its own component? So the component with scene just has something like ?

It might be better to update clearColor on the scene directly, and not on the virtual react component. I.e. via useRef

DefaultV commented 1 year ago

@dennemark I haven't tested that no, but I'm sure it's the same (That's the setup on my project).

The thing is, if you want to interact with a HTML UI, e.g.

div
   canvas
   ui
/div

<Context>
   <Engine>
      <Scene>
   <UI>
</Context>

You will need some sort of UI/global context, which will cover the <Engine and <Scene components, thereby forcing a react render when you update the context.

I can imagine that this use-case is quite prominent, but I'm open to hear other ways of dealing with a HTML UI

dennemark commented 1 year ago

@DefaultV I would have to check. But the tunnel approach that is linked above in a codesandbox uses zustand and is pretty smart concerning rerenders. So I guess the Engine and Scene wont be rerendered.

brianzinn commented 5 months ago

closing from inactivity. looks like there is a workaround. if there is an issue or feature request - kindly re-open or preferably create new issue, since this one will have unrelated history. Thank-you for your contributions.

brianzinn / react-babylonjs

Html element flickering (new) #260