Fix picking of labels by updating based on viewport instead of render…#457
Conversation
… target size. Also make the WebGPU path the same math, even though it doesn't handle picking yet.
There was a problem hiding this comment.
Thanks for working on this -- I think these changes are probably fine, although I can't say I recall how all the coordinate systems work to verify the math (is vertexPos really in NDC? iirc NDC is the final coordinate system that the GPU expects your gl_Position value to be in?)
One thing that would help quite a bit is updating the storybook for this repo -- since it seems like the primary change here is to properly support custom viewports, I think it would be cool to add a couple of sliders that tweak the renderer viewport in the storybook. Then we should be able to quickly move the viewport around and verify that the labels look correct.
| uLabelSize: { value: [0, 0] }, | ||
| uCanvasSize: { value: [0, 0] }, | ||
| uPixelRatio: { value: 1 }, | ||
| resolution: { value: new THREE.Vector2(0, 0) }, |
There was a problem hiding this comment.
Any reason not to follow the pattern of the other uniforms here with the u prefix and array type? Not really a big deal, but for consistency:
| resolution: { value: new THREE.Vector2(0, 0) }, | |
| uResolution: { value: [0, 0] }, |
There was a problem hiding this comment.
So the app does actually call "pickingMaterial.uniforms.resolution.value.copy(pickResolution);" in packages/viz/src/panels/ThreeDeeRender/Picker.ts .
There was a problem hiding this comment.
Interesting, so why was it working before? or you're saying it wasn't?
There was a problem hiding this comment.
In the app code there are numerous references to: X.resolution.value.copy(pickResolution);
I'd rather keep consistency with this so that we don't have versioning issues between these packages.
| this.mesh.onBeforeRender = (renderer, scene, camera, geometry, _material, group) => { | ||
| this.pickingMaterial.onBeforeRender(renderer, scene, camera, geometry, this.mesh, group); | ||
| }; |
There was a problem hiding this comment.
shouldn't this happen automatically when the picking material is used for rendering? Or what is it about the way we've implemented picking that makes this not get called automatically?
There was a problem hiding this comment.
This is legit as there are cases were the uCanvasSize wasn't being updated (I hit this), and the scale invariant text was suddenly stretching as the panel changed size.
Anyway working on your other suggestions...
|
@jtbandes , added picker system and visualization to the storybook, and looks like "resolution" is actually what is used in the app code. 
|
| lines.push(`viewOffset ${formatRect(pickResult.viewOffset)}`); | ||
| } | ||
| if (pickResult.pickViewportRect != undefined) { | ||
| lines.push(`pick viewport css ${formatRect(pickResult.pickViewportRect)}`); |
There was a problem hiding this comment.
seeing "CSS" info in the debug textarea stood out to me...maybe could use a better label?
| uLabelSize: { value: [0, 0] }, | ||
| uCanvasSize: { value: [0, 0] }, | ||
| uPixelRatio: { value: 1 }, | ||
| resolution: { value: new THREE.Vector2(0, 0) }, |
There was a problem hiding this comment.
Interesting, so why was it working before? or you're saying it wasn't?
2 participants
Changelog
None
Docs
None
Description
Labels with
sizeAttenuation=falseuse screen-space offsets so they keep a consistent pixel size. The WebGL shader was deriving those offsets from the renderer size, but that is the full render target size rather than the active viewport. In clipped views, split views, or other custom viewport rendering, the displayed label and its picking material could therefore use the wrong dimensions and drift out of alignment.This updates the WebGL label material to refresh its uniforms from
renderer.getCurrentViewport()and the renderer DPR before each render. The visible label mesh also forwards itsonBeforeRendercall to the picking material so picking receives the same current viewport state. The WebGPU material now uses the same pre-projection viewport math withviewportSizeandscreenDPR, keeping the two render paths consistent even though WebGPU picking is not implemented yet.Validation:
yarn testyarn typecheckyarn lint:ciPicking bounds for non-size-attenuated billboard labels were computed from the full renderer size and could be offset or scaled incorrectly inside a clipped viewport.
Label rendering and picking use the active viewport dimensions and device pixel ratio, so screen-space label offsets match the clipped view being rendered.
Fixes: FG-13915