Invalidate is not behaving as expected

[AlaricBaraou]

Right now the documentation of invalidate says

Calling invalidate() will not render immediately, it merely requests a new frame to be rendered out. Calling invalidate multiple times will not render multiple times. Think of it as a flag to tell the system that something has changed.

Since it's made to be used with frameloop="demand", we would expect calling invalidate to "call useFrame once more after this one or call a useFrame now if none is currently running"

But now, it's queuing a new frame on each call up to 60.
https://docs.pmnd.rs/react-three-fiber/advanced/scaling-performance#triggering-manual-frames
So if multiple components / third party library are calling invalidate, we queue a lot more frame than necessary.

Here is when it changed to this logic of queuing frame request a5f6322 while the previous version was requesting 2 frames instead of one but at least without queuing them. So it didn't matter if many third party library were calling invalidate()

It was changed to address this issue but I'm not sure I understand how it address it.

Personally I believe it should never queue more than 2 frames.
It can be called any amount of times in the code, in the end it should just mean "I need a new frame because something updated" not "I need a separate frame on top of any other already requested frames"

The current version can potentially stack 59 unnecessary frames and don't bring any upside for that ( that I can see ).

I believe invalidate should be reverted to the previous logic at least and why not even improved to call a single new frame and not 2.

@drcmda You're the last one who changed it, maybe you see something that I'm missing ?

Happy to do a PR to reflect those change if I get the go from the team.

I made a draft PR to illustrate, I wanted to make a PoC using codesanbox but looks like the demo are unusable at the moment,

[CodyJasonBennett]

Implemented in #3185.

[AlaricBaraou]

Sorry the PR I linked previously (#3187) wasn't about this issue, it was a relatively unrelated issue and duplicate of my previous PR (#3185).
That PR was just about fixing how the parameter was passed but doesn't fix anything here.

This is the PR I intended to do and that would fix this issue.
#3197

edit: I updated the initial issue description to link to the correct PR

[AlaricBaraou]

Can I do anything to help this one get merged ?

Like a demo or analysis on the impact on the eco-system etc

Let me know!

[AlaricBaraou]

Here is a demo using invalidate and Drei Orbitcontrols that perfectly showcase the difference.
When using the controls it request a render. Same when clicking the cube.
At the top of the demo I display the current number of frames queued in the root state.
We can see that the new version never goes over 1 and therefore never call unnecessary renders.

New version using the build from the linked PR
https://codesandbox.io/p/sandbox/basic-demo-forked-92gy2c?file=%2Fsrc%2FApp.js
Old version ( you can see how the frames stack up to 60 )
https://codesandbox.io/p/sandbox/invalidate-fix-forked-7sqggj?file=%2Fsrc%2FApp.js%3A43%2C28

I believe the majority of the ecosystem that use invalidate will keep working fine as long as they were already calling invalidate correctly.

[CodyJasonBennett]

Out since v8.16.1. This was the intended behavior, so I'm considering it a bugfix.

[drcmda]

@AlaricBaraou i missed this, but i can explain now, this was by design:

it prevented some edge cases and browser jerks, like maxing a window could skip frames and many other quirks. allowing something like invalidate(30) made sure it could render a whole bunch of frames. the idea was to be rather lenient and less greedy with frame render on demand, to a limit (60). invalidate is a flag that informs the system to push out a frame, or multiple, and it should be allowed to accumulate. this is different from advance, which explicitely renders one single frame.

imo it should be reverted, esp since it now causes other issues like #3228

the root cause here, i'm sure we find another solution.

[AlaricBaraou]

I don't know about edge cases, I personally like the strict way of not accumulating frames by default.
Especially since the function still accept the second parameter for those who want to request more than one frame out of safety.

My guess would be that the majority of bugs are caused by wrong implementations of invalidate() since it wasn't strictly one frame for a while.

I would personally prefer using the frames parameter in the libs that seem to have issues at the moments and leave the rest as is.

like
invalidate(undefined,60)

This way both use case are still possible, while reverting make it harder to request a single frame.

[gkjohnson]

I'm not familiar with the internals here but the prior behavior was incredibly odd and undocumented. It doesn't make sense that if you have 5 components call invalidate in a frame that you'd suddenly render 5 subsequent frames. You should only be rendering one. If there are issues with things like window or canvas resizing then that seems like an order of operations issue in the project. At most I'd expect at most two frames to have to be rendered - though even then it should only have to be one once the resize takes place. This isn't an issue I have when performing on demand rendering with vanilla three.

Overall this seems like a very heavy-handed bandaid to fix timing issues in other components.

[CodyJasonBennett]

I think if we're seeing sweeping breakage across the ecosystem, then we have no choice but to revert in 8.x and revisit in 9.x. It's possible there is a race condition produced from React itself, which has its own frame loop, or a dependency on broken behavior to begin with. Unless I see a strong technical reason against this, I can only think of it as conservative compared to the breaking changes from three we already have to reconcile in 9.x if at all.

[gkjohnson]

I understand if this needs to be reverted because ramifications aren't clear but I'm hoping this is something that can be reopened as a higher priority bug. Are there other other examples of failures beyond react-spring? Is it possible that that project is relying on the incorrect, undocumented behavior?

In the mean time it would be nice if there a temporary, user-configurable way enable the correct behavior or do something like cap the frames to something like 1 instead of 60.

[CodyJasonBennett]

I'm not aware of any breakage beyond react-spring after extensive integration tests, and I need to see more examples before doing anything here. I agree this is a major regression to walk back, and intend to keep this behavior with 9.x at the minimum.

[gkjohnson]

Is there an update on the state of this? Was it reverted? Or fixed? If it was reverted can we keep this issue open?

[CodyJasonBennett]

I don't believe I backported anything to call this resolved. Let's open this until a fix is available on v8 and v9.