fix(react): remove redundant initial render redraw#10428
Conversation
33164a5 to
9f8522b
Compare
| // Force initial render if Deck is initialized | ||
| if (thisRef.deck?.isInitialized) { | ||
| // Force initial render once when Deck is initialized | ||
| if (thisRef.deck?.isInitialized && !thisRef.initialRenderDone) { |
There was a problem hiding this comment.
@ibgreen Why was this added in the first place?
87883cd#diff-7207aa699a54f22a73db1a29c5cfab4ae82000a14d2cd274f92f7631b997e13fR215-R219
redrawDeck() is called immediately before this.
I tested removing it and did not see any apparent issue. If there is no good reason I think it should be dropped.
There was a problem hiding this comment.
The only reason I can think of was problems with getting WebGPU rendering to work in the React binding.
There was a problem hiding this comment.
WebGPU is working fine too in website examples.
Regardless of the problem, it was the wrong fix.
There was a problem hiding this comment.
Thanks for checking this out. I went ahead and removed the redraw call and reworked the regression test.
missx-oai
left a comment
missx-oai
left a comment
There was a problem hiding this comment.
I'd recommend dropping the redraw call altogether.
9f8522b to
34b865b
Compare
jarnoharno
changed the title
|
Thank you for identifying the issue! |
d111c95 to
f62030e
Compare
|
I also pushed a fix for the CI failures. The old The tests now pass an |
The DeckGL layout effect called deck.redraw('Initial render') on every
React commit after Deck initialized, forcing unnecessary full scene
redraws and hurting interaction performance.
Per review feedback, drop the deck.redraw('Initial render') call from
the layout effect instead of gating it. redrawDeck() already flushes
any pending dirty flag immediately before, so the forced redraw is
redundant. Rework the regression test to assert that the React wrapper
never forces a redraw across re-renders.
The deckgl spec's waitUntilReady polled deck.needsRedraw, which races against the animation loop and timed out on slow CI runners once redraws are flushed by the loop instead of on every React commit. Replace it with a per-mutation render tracker: each mount or rerender passes a fresh onAfterRender callback to the element, and waitUntilReady resolves once a draw completes after that mutation. This waits on a monotonic, publicly observable condition, keeps an explicit "change has been rendered" barrier at every wait site, and is immune to concurrent tests observing each other's draws.
f62030e to
82dc1f6
Compare
|
coverage: 83.164% (-0.002%) from 83.166% — jarnoharno:fix/react-initial-render-redraw into visgl:master |
|
This PR has a failing test, please fix it. Thank! |
|
Hmm, it failed and now its passing. I wonder if its flapping? https://github.com/visgl/deck.gl/actions/runs/29306444817/job/87002921297 |
6 participants
Closes #10429
Background
The
DeckGLlayout effect callsdeck.redraw('Initial render')on every React commit after mount, forcing extra full scene redraws during interaction.Change List
deck.redraw('Initial render')call from the layout effect.DeckGLwith new props and asserts the wrapper never forces a redraw (aredraw()call with a reason argument). Verified the test fails against master.Note
Low Risk
Small React integration change with a targeted regression test; relies on Deck’s normal redraw loop instead of an extra forced redraw.
Overview
Stops the DeckGL React wrapper from calling
deck.redraw('Initial render')at the end of every layout effect after the deck is initialized. That forced a full scene redraw on each commit (including during interaction), on top of the existingredrawDeckpath and Deck’s animation loop.Tests now wait for readiness via an
onAfterRenderdraw counter instead ofneedsRedraw, and a new case asserts that prop-only re-renders do not triggerredraw(reason)calls with a reason argument.Reviewed by Cursor Bugbot for commit 95f90d4. Bugbot is set up for automated code reviews on this repo. Configure here.