fix(Html): update occlusion when occlude prop changes#2703
fix(Html): update occlusion when occlude prop changes#2703chris-xinhai-li wants to merge 1 commit intopmndrs:masterfrom
Conversation
|
Someone is attempting to deploy a commit to the Poimandres Team on Vercel. A member of the Team first needs to authorize it. |
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. |
travisbreaks
left a comment
travisbreaks
left a comment
There was a problem hiding this comment.
Good catch on the root cause. The useFrame gate skipping recalc when only occlude changes is a real bug, and the ref-based tracking pattern is the right approach for useFrame callbacks (no deps array to work with).
One concern with the reference equality check: when occlude is passed as an inline array (which is the common pattern from the issue), e.g.:
<Html occlude={enableOcclusion ? refs : undefined}>...where refs is [meshRef1, meshRef2] created inline or via a hook that returns a new array each render, prevOcclude.current !== occlude will be true on every single frame. That bypasses the entire early-exit optimization and forces full position + visibility recalculation every frame, even when nothing actually changed.
For primitive values (true, "blending", "raycast", undefined), reference equality works fine. But array occluders need something like a length + identity check, or serializing to a stable key. A simple approach:
function occludeChanged(prev: typeof occlude, next: typeof occlude): boolean {
if (prev === next) return false
if (Array.isArray(prev) && Array.isArray(next)) {
return prev.length !== next.length || prev.some((ref, i) => ref !== next[i])
}
return true
}This keeps the fix correct for the toggle case (the actual bug) while avoiding a per-frame penalty for stable array occluders that happen to get a new array reference each render.
Minor: initializing prevOcclude with occlude means the very first frame won't detect a "change" from the initial prop. That's fine for this use case since the existing occlusion logic handles the initial state already.
|
Hi @travisbreaks: My thinking when proposing the simpler reference check was based on the typical React convention of treating props as immutable. In most React patterns (useState, useMemo, useEffect, etc.), reference equality is used as the signal for change, and developers usually ensure immutability / stability when passing arrays or objects. (e.g., via useMemo, in my projects I normally use a custome state management bases on immer.js) From that perspective, assuming a immutable reference for occlude felt consistent with common React practices, and it keeps the patch minimal. If users generate a new array every render, they can usually handle it at a higher level according to their application level state management / change detection mechanisms. So I agree with the concern (which is not specific to this Html component), it mostly comes down to where this should be handled. Personally I lean toward letting the upper-level logic deal with it, since that’s the usual pattern in React. I totally agree with this one: I will fix this, thanks a lot for pointing out. Cheers |
chris-xinhai-li
force-pushed
the
fix/html-occlude-update-issue
branch
from
6a2b86a to
a7890b7
Compare
|
@chris-xinhai-li can you check this against the v11 version? https://github.com/pmndrs/drei/tree/v11-working/src/core/UI/Html There was some changes and has the new useFrame logic. |
|
Hi @DennisSmolek, I checked the v11 branch and reviewed the recent changes to the Html component. It looks like:
Is there anything I should do for v11? Should I open a separate PR targeting the v11 branch? |
I would move this to the v11 branch. I can merge it faster and we are likely merging those over faster than smaller v10 changes. |
3 participants
fixes #2702
Problem
Changing the
occludeprop dynamically does not update Html visibilityuntil the camera moves.
This happens because the occlusion logic inside
useFrameonly runswhen camera zoom or screen position changes.
Fix
Track changes to the
occludeprop and force recalculation when it changes.Result
Occlusion updates immediately when the prop changes without requiring camera movement.
Checklist