Skip to content

Commit 8eb7b7f

Browse files
authored
Merge pull request #3629 from DennisSmolek:v10-fix-canvas-override
V10-fix-canvas-override
2 parents f98e0c6 + 13702e2 commit 8eb7b7f

File tree

2 files changed

+212
-17
lines changed

2 files changed

+212
-17
lines changed

packages/fiber/src/core/renderer.tsx

Lines changed: 60 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -125,6 +125,19 @@ export function createRoot<TCanvas extends HTMLCanvasElement | OffscreenCanvas>(
125125
let onCreated: ((state: RootState) => void) | undefined
126126
let lastCamera: RenderProps<TCanvas>['camera']
127127

128+
// Track last configured props for diffing - prevents imperative setter values
129+
// from being overwritten when Canvas re-configures (e.g., on resize)
130+
let lastConfiguredProps: Partial<{
131+
dpr: RenderProps<TCanvas>['dpr']
132+
frameloop: RenderProps<TCanvas>['frameloop']
133+
performance: RenderProps<TCanvas>['performance']
134+
shadows: RenderProps<TCanvas>['shadows']
135+
linear: boolean
136+
flat: boolean
137+
legacy: boolean
138+
textureColorSpace: THREE.ColorSpace
139+
}> = {}
140+
128141
let configured = false
129142
let pending: Promise<void> | null = null
130143

@@ -304,19 +317,30 @@ export function createRoot<TCanvas extends HTMLCanvasElement | OffscreenCanvas>(
304317
if (!is.equ(size, state.size, shallowLoose)) {
305318
state.setSize(size.width, size.height, size.top, size.left)
306319
}
307-
// Check pixelratio
308-
if (dpr && state.viewport.dpr !== calculateDpr(dpr)) state.setDpr(dpr)
309-
// Check frameloop
310-
if (state.frameloop !== frameloop) state.setFrameloop(frameloop)
320+
// Check pixelratio - only update if the PROP changed (not if state differs from prop)
321+
// This preserves imperative setDpr() changes across Canvas re-configures
322+
if (dpr !== undefined && !is.equ(dpr, lastConfiguredProps.dpr, shallowLoose)) {
323+
state.setDpr(dpr)
324+
lastConfiguredProps.dpr = dpr
325+
}
326+
// Check frameloop - only update if the PROP changed
327+
// This preserves imperative setFrameloop() changes across Canvas re-configures
328+
if (frameloop !== undefined && frameloop !== lastConfiguredProps.frameloop) {
329+
state.setFrameloop(frameloop)
330+
lastConfiguredProps.frameloop = frameloop
331+
}
311332
// Check pointer missed
312333
if (!state.onPointerMissed) state.set({ onPointerMissed })
313334
// Check dragover missed
314335
if (!state.onDragOverMissed) state.set({ onDragOverMissed })
315336
// Check drop missed
316337
if (!state.onDropMissed) state.set({ onDropMissed })
317-
// Check performance
318-
if (performance && !is.equ(performance, state.performance, shallowLoose))
338+
// Check performance - only update if the PROP changed
339+
// This preserves any runtime performance changes across Canvas re-configures
340+
if (performance && !is.equ(performance, lastConfiguredProps.performance, shallowLoose)) {
319341
state.set((state) => ({ performance: { ...state.performance, ...performance } }))
342+
lastConfiguredProps.performance = performance
343+
}
320344

321345
// Set up XR (one time only!)
322346
if (!state.xr) {
@@ -361,7 +385,9 @@ export function createRoot<TCanvas extends HTMLCanvasElement | OffscreenCanvas>(
361385
}
362386

363387
//* Shadow Map ==============================
364-
if (renderer.shadowMap) {
388+
// Only update if the shadows PROP changed (not just if state differs)
389+
if (renderer.shadowMap && !is.equ(shadows, lastConfiguredProps.shadows, shallowLoose)) {
390+
lastConfiguredProps.shadows = shadows
365391
const oldEnabled = renderer.shadowMap.enabled
366392
const oldType = renderer.shadowMap.type
367393
renderer.shadowMap.enabled = !!shadows
@@ -386,8 +412,13 @@ export function createRoot<TCanvas extends HTMLCanvasElement | OffscreenCanvas>(
386412

387413
// Only execute legacy color management if could be using the webgl renderer is true
388414
if (R3F_BUILD_LEGACY) {
389-
//We only notifiy its depreciation for default and legaqcy. webgpu imports dont get noticed
390-
if (isDefaultBuild) {
415+
// Check if any of the color management props changed
416+
const legacyChanged = legacy !== lastConfiguredProps.legacy
417+
const linearChanged = linear !== lastConfiguredProps.linear
418+
const flatChanged = flat !== lastConfiguredProps.flat
419+
420+
//We only notifiy its depreciation for default and legacy. webgpu imports dont get noticed
421+
if (isDefaultBuild && legacyChanged) {
391422
if (legacy)
392423
notifyDepreciated({
393424
heading: 'Legacy Color Management',
@@ -396,22 +427,34 @@ export function createRoot<TCanvas extends HTMLCanvasElement | OffscreenCanvas>(
396427
})
397428
}
398429

399-
THREE.ColorManagement.enabled = !legacy
430+
// Only apply if props changed
431+
if (legacyChanged) {
432+
THREE.ColorManagement.enabled = !legacy
433+
lastConfiguredProps.legacy = legacy
434+
}
400435

401-
// Set color space and tonemapping preferences
402-
if (!configured) {
436+
// Set color space and tonemapping preferences - only on first configure or when prop changes
437+
if (!configured || linearChanged) {
403438
renderer.outputColorSpace = linear ? THREE.LinearSRGBColorSpace : THREE.SRGBColorSpace
439+
lastConfiguredProps.linear = linear
440+
}
441+
if (!configured || flatChanged) {
404442
renderer.toneMapping = flat ? THREE.NoToneMapping : THREE.ACESFilmicToneMapping
443+
lastConfiguredProps.flat = flat
405444
}
406445

407-
// Update color management state
408-
if (state.legacy !== legacy) state.set(() => ({ legacy }))
409-
if (state.linear !== linear) state.set(() => ({ linear }))
410-
if (state.flat !== flat) state.set(() => ({ flat }))
446+
// Update color management state only if PROP changed
447+
if (legacyChanged && state.legacy !== legacy) state.set(() => ({ legacy }))
448+
if (linearChanged && state.linear !== linear) state.set(() => ({ linear }))
449+
if (flatChanged && state.flat !== flat) state.set(() => ({ flat }))
411450
}
412451

413452
// Update textureColorSpace state (color space for 8-bit input textures)
414-
if (state.textureColorSpace !== textureColorSpace) state.set(() => ({ textureColorSpace }))
453+
// Only update if the PROP changed
454+
if (textureColorSpace !== lastConfiguredProps.textureColorSpace) {
455+
if (state.textureColorSpace !== textureColorSpace) state.set(() => ({ textureColorSpace }))
456+
lastConfiguredProps.textureColorSpace = textureColorSpace
457+
}
415458

416459
// Set gl props
417460
if (glConfig && !is.fun(glConfig) && !isRenderer(glConfig) && !is.equ(glConfig, renderer, shallowLoose))

packages/fiber/tests/renderer.test.tsx

Lines changed: 152 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -866,4 +866,156 @@ describe('renderer', () => {
866866
await act(async () => root.render(<TestComponent />))
867867
await act(async () => updateSynchronously(1))
868868
})
869+
870+
//* Props vs Setters Conflict Resolution Tests ==============================
871+
// These tests verify that imperative setter changes (setDpr, setFrameloop, etc.)
872+
// are preserved when Canvas re-configures (e.g., on resize)
873+
874+
it('should preserve setFrameloop changes across re-configure (no prop passed)', async () => {
875+
// Test scenario: Canvas has no frameloop prop (defaults to 'always')
876+
// User calls setFrameloop('demand'), then Canvas re-configures (resize)
877+
// Frameloop should stay 'demand', not reset to default
878+
879+
// Initial configure with default frameloop (no prop passed explicitly uses 'always')
880+
const store = await act(async () =>
881+
(await root.configure({ size: { width: 100, height: 100, top: 0, left: 0 } })).render(<group />),
882+
)
883+
const state = store.getState()
884+
885+
// Verify initial state - frameloop should be 'always' (the default)
886+
expect(state.frameloop).toBe('always')
887+
888+
// User imperatively changes frameloop to 'demand'
889+
await act(async () => state.setFrameloop('demand'))
890+
expect(store.getState().frameloop).toBe('demand')
891+
892+
// Simulate resize by re-configuring with new size (but same frameloop prop)
893+
// This is what happens when the Canvas container resizes
894+
await act(async () => root.configure({ size: { width: 200, height: 200, top: 0, left: 0 } }))
895+
896+
// Size should have updated
897+
expect(store.getState().size.width).toBe(200)
898+
expect(store.getState().size.height).toBe(200)
899+
900+
// Frameloop should STILL be 'demand' - not reset to 'always'
901+
expect(store.getState().frameloop).toBe('demand')
902+
})
903+
904+
it('should preserve setDpr changes from child component across re-configure', async () => {
905+
// Test scenario: Canvas has dpr={[1, 2]} prop
906+
// A child component calls setDpr(1) after mount
907+
// Canvas re-configures (resize) - dpr should stay at 1, not reset to [1, 2]
908+
909+
let setDprFromComponent: (dpr: number) => void = () => {}
910+
911+
function DprSetter() {
912+
const state = useThree()
913+
React.useEffect(() => {
914+
// Expose setDpr for test to call
915+
setDprFromComponent = (dpr: number) => state.setDpr(dpr)
916+
}, [state])
917+
return null
918+
}
919+
920+
// Initial configure with dpr={[1, 2]}
921+
const store = await act(async () =>
922+
(
923+
await root.configure({
924+
dpr: [1, 2],
925+
size: { width: 100, height: 100, top: 0, left: 0 },
926+
})
927+
).render(<DprSetter />),
928+
)
929+
930+
// Verify initial dpr was applied (calculateDpr picks based on device pixel ratio)
931+
const initialDpr = store.getState().viewport.dpr
932+
expect(typeof initialDpr).toBe('number')
933+
934+
// Child component imperatively changes dpr to 1
935+
await act(async () => setDprFromComponent(1))
936+
expect(store.getState().viewport.dpr).toBe(1)
937+
938+
// Simulate resize by re-configuring with new size (but same dpr prop)
939+
await act(async () =>
940+
root.configure({
941+
dpr: [1, 2],
942+
size: { width: 200, height: 200, top: 0, left: 0 },
943+
}),
944+
)
945+
946+
// Size should have updated
947+
expect(store.getState().size.width).toBe(200)
948+
expect(store.getState().size.height).toBe(200)
949+
950+
// DPR should STILL be 1 - not reset based on [1, 2] range
951+
expect(store.getState().viewport.dpr).toBe(1)
952+
})
953+
954+
it('should update dpr when the PROP changes (controlled mode)', async () => {
955+
// Test scenario: Canvas dpr prop actually changes from [1, 2] to 1
956+
// In this case, the new prop value should be applied
957+
958+
const store = await act(async () =>
959+
(
960+
await root.configure({
961+
dpr: [1, 2],
962+
size: { width: 100, height: 100, top: 0, left: 0 },
963+
})
964+
).render(<group />),
965+
)
966+
967+
const initialDpr = store.getState().viewport.dpr
968+
969+
// Re-configure with a DIFFERENT dpr prop value
970+
await act(async () =>
971+
root.configure({
972+
dpr: 0.5, // Changed from [1, 2] to 0.5
973+
size: { width: 100, height: 100, top: 0, left: 0 },
974+
}),
975+
)
976+
977+
// DPR should now be 0.5 (the new prop value was applied)
978+
expect(store.getState().viewport.dpr).toBe(0.5)
979+
})
980+
981+
it('should preserve multiple setter changes across re-configure', async () => {
982+
// Test scenario: Both dpr and frameloop are changed imperatively
983+
// Both should be preserved after re-configure
984+
985+
const store = await act(async () =>
986+
(
987+
await root.configure({
988+
dpr: [1, 2],
989+
frameloop: 'always',
990+
size: { width: 100, height: 100, top: 0, left: 0 },
991+
})
992+
).render(<group />),
993+
)
994+
995+
const state = store.getState()
996+
997+
// User imperatively changes both dpr and frameloop
998+
await act(async () => {
999+
state.setDpr(1)
1000+
state.setFrameloop('demand')
1001+
})
1002+
1003+
expect(store.getState().viewport.dpr).toBe(1)
1004+
expect(store.getState().frameloop).toBe('demand')
1005+
1006+
// Simulate resize by re-configuring with same props
1007+
await act(async () =>
1008+
root.configure({
1009+
dpr: [1, 2],
1010+
frameloop: 'always',
1011+
size: { width: 200, height: 200, top: 0, left: 0 },
1012+
}),
1013+
)
1014+
1015+
// Both values should be preserved
1016+
expect(store.getState().viewport.dpr).toBe(1)
1017+
expect(store.getState().frameloop).toBe('demand')
1018+
// Size should have updated
1019+
expect(store.getState().size.width).toBe(200)
1020+
})
8691021
})

0 commit comments

Comments
 (0)