brianzinn / react-babylonjs

React for Babylon 3D engine
https://brianzinn.github.io/react-babylonjs/
818 stars 105 forks source link

state not changing in callback function #139

Closed hupeky closed 3 years ago

hupeky commented 3 years ago

Hi Brian, Im having an issue where the data in a callback function is not accessing the current value in state

say i have a call back function whos data changes with state

const [data, setData] = useState(null);

set data somewhere in component

setData(someData);

Make a callback function which uses the data state

  const onSettingsClick = useCallback(() => {
    if (data) {
      console.log(data.settingsPath);
      dispatch(setPath(data.settingsPath));
    }
    // eslint-disable-next-line react-hooks/exhaustive-deps
  }, [data]);

then that callback is passed to the react-button

  <babylon-button
      name={`${name}_menuButton`}
      onPointerClick={onSettingsClick}

the data getting logged out onClick is always null even though in the react component the state is changing.

ive needed to resort to accessing the button via raw babylon to finish the job

  if (button && callback) {
    // add the new callback if it exists
    callbackRef.current = button[pointerClickObservable]?.add(callback);
  }

Any help is really appreciated. thank you.

brianzinn commented 3 years ago

I would have to see more code, but is it possible that your variable is being brought in via closure (and stale during the callback)?

brianzinn commented 3 years ago

actually... sorry. it's because onPointerClick.add(..) is only called once. it's a problem with this library not detecting the new function and removing old and adding new.

hupeky commented 3 years ago

Nice one,

Is that a problem across all the observables? is this something you can fix?

brianzinn commented 3 years ago

Yes, it is a problem across all. The fix needs to go here: https://github.com/brianzinn/react-babylonjs/blob/master/src/UpdateInstance.ts#L47

I think I would just need to get the observer being returned and to remove the existing one for that property (if it exists) and then set the new one. I can use the hostIntance to track the observables with Record<string, Observer<any>>.

Once that is in place then I can undo where I hard-coded that it would only set the observable once here: https://github.com/brianzinn/react-babylonjs/blob/master/src/PropsHandler.ts#L369

It should be change so it can be changed!

if (oldProp !== newProp) {

I remember now when I added it - was done really quickly just to get GUI buttons working and then I never revisited....

brianzinn commented 3 years ago

hi @kyehuelin I have updated the repository, but it's not ready for NPM. I needed this kind of syntax for it to work:

const onClick = useCallback(() => {
  console.log('toggle clicked');
  toggleShown(isShown => !isShown)
}, []);

One major issue with current implementation is that inline arrow functions are detected as a change on every render (as they are different). It was causing an infinite loop in the reconciler (same with useCallback without specifying a dependency array). I don't know if you can work from source on storybook to verify, but otherwise I'll try again soon, but really busy this weekend. Cheers.

brianzinn commented 3 years ago

hi @kyehuelin . it's a good thing I wrote 100% coverage on the observables for my state machine project, so that it actually clicked what was happening 🤦‍♂️ I was adding to the observable during the callback and creating an infinite-loop. What I do now is update the callback and I suspect that it should be working as expected. One thing to note is that when the function changes it will re-assign the callback, so it works with useCallback and even with this kind of syntax (even though that is often inneficient):

const onClick = () => {
  // ...    
};
...
return (
    <babylon-button ...  onPointerDownObservable={onClick}>
      <textBlock text="click me" />
    </babylon-button>
);

hope it's working for you on: 3.0.16 📦 Can you give it a go and report back?

hupeky commented 3 years ago

thank you very much for looking into this and doing the fix. i will go ahead and test the update after the weekend when I get back to it.

hupeky commented 3 years ago

hi @brianzinn I checked my code and things are now working well for the above issue. thanks.