Conversation
ShaMan123
commented
Jul 26, 2026
ShaMan123
left a comment
Contributor
Author
There was a problem hiding this comment.
ready for review
ShaMan123
force-pushed
the
fix/getFullPick#775
branch
from
July 26, 2026 06:19
79569ef to
82f4570
Compare
ShaMan123
commented
Jul 26, 2026
ShaMan123
left a comment
Contributor
Author
There was a problem hiding this comment.
3 commits:
- the fix
- introduce vitest, fix (dedicated) circular deps, remove jest
- add test
getFullPick data evalgetFullPick data eval + vitest
Contributor
Author
|
Working on this PR made me think: |
ShaMan123
marked this pull request as draft
September 15, 2026 08:57
getFullPick data eval + vitestgetFullPick concurrency + vitest
ShaMan123
marked this pull request as ready for review
September 16, 2026 04:41
ShaMan123
force-pushed
the
fix/getFullPick#775
branch
2 times, most recently
from
September 16, 2026 05:47
99ec925 to
71f61f7
Compare
ShaMan123
force-pushed
the
fix/getFullPick#775
branch
from
September 16, 2026 05:49
71f61f7 to
c6ff3d5
Compare
ShaMan123
commented
Sep 16, 2026
Contributor
Author
There was a problem hiding this comment.
Ready for review.
Please read the description and provide feedback regarding these points:
- should I cherry-pick vitest setup to a separate PR? I think I should #805. Once that merges I can rebase and drop the commit.
-
Proposal: should the picker resolve the unknown hits via ray casting and keep the methods async? I think it should since it makes the picker what it is - a performance optimization, not a hack with edge cases. If so I will restore the async signatures (now possible due to the picker consolidation refactor) and follow up on the rest in a dedicated PR.those methods are noise - unused and should can be removed IMO.
Contributor
Author
|
finalized description |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
fixes #775
getFullPickused to callgetItemAtto resolvelocalId. That seemed right for code reuse, but it introduces a bug: resolvinglocalIdis async (a round trip to the fragments thread), and during that time mutable deps such as the camera can change, makingpointandnormalevaluation wrong.Fixing it meant looking at how the picker renders, which raised a second question: the id, depth and normal passes each walked the scene and rendered it again, so a single
getFullPickcostK + 2renderer.rendercalls (K = loaded models) and 3 blocking GPU readbacks. This PR fixes the drift and consolidates the three passes into one render.During work I discovered I bug fixed in
fix(): respect local clipping planes(local clipping planes were not respected, global clipping planes were added twice).I also noticed:
getModelAt,getPointAtandgetNormalAtare not used anywhere and can be dropped (they were the targets of the dead discussion around refactoring them to being sync methods - since the initial approach was to fix by moving to sync)debugModeshould be private or a getter/setterPicker logic
THREE.Cameracopied from the world camera, and the decoders unproject through that copy, so a camera that moves whilelocalIdis in flight can no longer driftpoint,normalordistance.K + 2. A single privaterenderPick(ndc, request, target, buffers)renders the BIM scene once into a multiple-render-target with one RGBA8 attachment per output (id, depth, normal). One depth test now decides all three outputs, so they always describe the same fragment by construction.getPointAtreads one attachment andgetFullPickreads three. Per-request shader variants were considered and rejected: they would cost a program compile each.scene.overrideMaterialand the model byte is set per draw frommaterial.onBeforeRender. Because the meshes keep their own material arrays, per-item draw groups, hidden items and highlight slots behave exactly as they do on screen, and the id shader can no longer leak onto a mesh (the failure mode behind Delta models become unpickable after editor.edit() because FastModelPicker hides parent model root #746).traverseOwn/_ownMeshesbookkeeping), and hides everything else. Invisible subtrees are skipped, leaving fragments' own tile visibility untouched.Line,PointsandSpriteobjects are now hidden during the pick. Previously onlyisMeshobjects were, so a non-BIM line or point under the cursor (annotation lines, for example) wrote its own colour into the pick buffers and decoded to a wrong item and a bogus point.P' = M · P, the classic pick matrix) and the target is 1×1. Frustum culling then skips every mesh whose bounds miss that pixel, and the pixel centre is exactly the cursor. This removes the scissor box, the device-pixel-ratio workaround and the render-target resize listener, and it samples the cursor position exactly rather than the centre of the pixel under it. The world camera is never modified, and custom projections still work.finally, so a throwing render (context loss, for example) can no longer leave hidden objects or an override material behind.render()refreshes them on every call; the pick now suppresses that and restores a refresh the app had requested for its next frame.decodeId,decodeDepth,decodeNormal,unprojectToWorld) that take a pixel and a frame, testable without a GL context. An unreachable zero-length-normal guard was dropped along the way: byte quantization cannot land all three channels within 1/255 of the origin.debugModeis on, instead of the picker keeping a viewport-sized target alive for every pick.Net effect on the file: 1210 → 880 lines, with
runIdPass,renderIdPass,renderPickPass,renderWithTileMaterial,renderDepthPass,renderNormalPass,applyIdMaterial,restoreOriginalMaterials,traverseOwnandgetModelRootsgone, and the three shaders merged into one.Test setup
// @vitest-environment happy-dom, which also brings invitest-canvas-mockand the@vitest/web-workershim viavitest.setup.ts.tests/canvas-snapshot.tsrecords what is drawn to a 2D canvas and renders it as one character per pixel, so the debug overlay can be snapshotted and eyeballed.fast-model-picker.tsis at 100% statements, lines and functions (99.16% branches; the one remaining branch has no source location, a v8 remapping artifact).Verification
Unit tests cover: one render per
getFullPick; only the requested attachments read back; the scene and renderer fully restored even whenrenderthrows; non-BIMLine/Pointshidden; nested delta models drawn with their own byte; the pick camera narrowed to the cursor with depth untouched;MAX_MODELS;allowOverride; the guards; and the original camera-drift regression. Two snapshots pin the decoded output of a synthetic frame and the debug overlay (which also pins the WebGL→canvas Y-flip).The shader itself needs a real GPU, so old and new were run side by side in headless Chrome against fragments' own worker raycast (Apple M2 Max, 1280×713,
school_arq+school_str, 160 cursor positions), with identical results at pixel ratio 1 and 2:render()calls pergetFullPickPointsin front of the modelKnown divergence from the raycaster
3 of 131 positions resolve to a different item than the worker raycast. All 3 sit on a 50 mm mullion that fragments currently draws at a reduced LOD — a line mesh carrying no
idattribute, which the pick pass hides, because drawing it would read the missing attribute as(0, 0, 0, 1)and write a false hit on item 0 while occluding the shell behind it.Triaged on
school_arqat NDC (-0.787, -0.074):getVisible([141085])returns[true].So the rule today is what can be picked is what is drawn, which is recorded in the class docs and pinned by a test.
Proposal: full compatibility with raycaster - identify everything the pick draws
FastModelPickerexists to avoid ray casting, so it should be a drop-in replacement for it. Today it isn't, in two places:LODMesh) that carry noidattribute, so the pick hides them and returns whatever is behind. This is the 3/131 divergence above.SimpleRaycastermakes up for it by raycastingworld.mesheswith three'sRaycasteron everycastRayand keeping the closer hit.This proposal gives both an id in the existing encoding, so every pixel the pick draws decodes to an answer and no fallback resolver is needed. It replaces the earlier "placeholder id + fallback resolver" idea (see Alternatives considered).
Encoding
The id output keeps its layout: R names the owner, GBA holds a 24-bit
id + 1, and 0 means nothing.itemId + 1, per-vertexidattribute{ modelId, itemId }itemId + 1, per-instanceidattribute{ modelId, itemId }index + 1into a per-pick object list{ object }Raycaster, over those objects onlynullMAX_MODELSis 254, so 255 is free. 24 bits allow 16,777,215 non-BIM objects per pick.LOD segments
A per-draw id isn't enough here. A LOD draw is a whole tile: one
InstancedBufferGeometrywith one instance per segment, packing segments from many items. The main thread also has no segment → item map.tile.idsis only allocated for shell tiles, and LOD visibility and highlight data are bare ranges.The worker has the map. LOD tiles go through the same
setupTileSampleAttributesas shells, with the sample's location anditemId(sample)in scope; only theobjectClass === SHELLcheck keeps it from writing ids. A segment's index is its vertex location / 2, which is howLodHelper.setLodFilteralready addresses instances.Fragments change
constructTile: allocateidsfor LINE tiles too, at 4 bytes per segment (positionCount / 6 * 4).setupTileSampleAttributes: for LINE samples, write the same big-endianitemId + 1into every segment of the sample.LODManager.createMesh: bindrequest.itemIdsas anInstancedBufferAttribute(ids, 4)namedid.MeshManagerfeedsrequest.itemIdsintomodel.visibleItemsone byte at a time, so for shell tiles that set already holds byte values rather than item ids. LOD tiles would start feeding it the same way, so fix it in the same change: decode 4 bytes per entry, and don't drop an item when one of several tiles drawing it is deleted.Picker change
LodShaders.vertexstretches into a screen-space line, soscene.overrideMaterialcan't draw it. During the pick eachLODMeshgets a pick variant withallowOverride = false: the LOD line expansion,vId = id, theitemFilter == 0early-out for hidden items, and the shared pick outputs.LODMeshhas a single material and one full-range group, so swapping it has none of the draw-group problems that ruled material swaps out for shells. It is restored in the samefinallyas the rest of the scene state.lodSizefollows the pick target.LODMesh.onBeforeRenderwritesrenderer.getSize()intomaterial[0].lodSize. Under the narrowed pick projection one viewport pixel spans the whole target, so the variant resetslodSizeto the target size ((1, 1)for a pick) in its ownonBeforeRender, which three calls after the object's. Without that, the line shrinks to a fraction of a pixel.decodeNormalalready decodes asnull.Cost.
idsadds 4 bytes per LOD segment, next to 24 bytes of positions. LINE tiles already allocate afaceIdBuffer(8 bytes per segment) and send a Float32 face-id payload (24 bytes per segment) that the LOD path throws away. Skipping both for LINE tiles more than pays forids.Non-BIM objects
castRay(position, items)ignores non-BIM objects outsideitems(defaultworld.meshes). If the pick drew every object in the scene, an unlisted grid could hide the model. So the pick takes the same list and hides everything else.overrideMaterial, anything whose on-screen shape comes from its own vertex shader lands in the wrong place. Eligible:Mesh,Line,LineSegmentsandLineLoopwith a built-in material, noonBeforeCompile, no displacement map, not instanced, skinned or morphed, and depth-tested. Everything else stays hidden as today, includingSprite,Points(point size),Line2/LineMaterial,InstancedMesh,SkinnedMesh,ShaderMaterialanddepthTest: falsehelpers.SimpleRaycasterraycasts only what the pick couldn't draw. That is usually nothing, so the per-castRaycasteroverworld.meshesgoes away in the common case.normalattribute, write normal alpha 0. These are uniforms rather than defines, so they add no shader variants.What it closes and what it doesn't
object,point,normalanddistance, but notface,faceIndex,uvorinstanceId.Alternatives considered
{ modelId, itemId: null }and trigger a raycast scoped to that model; non-BIM pixels decode to 255 and trigger three'sRaycaster. It leaves fragments untouched, but every LOD hit still pays a worker round trip and every non-BIM hit a main-thread raycast.gl_InstanceID, resolved to an item on the worker. It still needs a new fragments API, and has to split 24 bits between tile and instance. The per-segmentidsbuffer is less code and reuses the existing decoder unchanged.Things to decide
getFullPickneeds a way to return non-BIM hits: a discriminated union ({ kind: "item", … } | { kind: "object", … }), or an optionalobjectfield. Related: shouldSimpleRaycasterraycast the single returned object to fill inface,faceIndexanduv, socastRay's return type stays the same?LodShaders.vertexin the picker would drift from fragments. Recommended: fragments exports the line expansion as a shared chunk, or a ready-madeLodPickMaterial, used by both materials.PointsandSprite. Keep them hidden with a raycast fallback, or give them their own pick variants (point size, billboarding) in a follow-up.idon LOD meshes. The picker change would follow in its own PR, separate from this one.Follow-up: synchronous picking via synchronous
localIdresolutionThis is separate from the id buffer and can ship before or after it. It helps shell hits as much as LOD hits.
The id buffer holds
itemId, notlocalId, so LOD hits get theirlocalIdthe way shell hits do today: onegetLocalIdsFromItemIdsround trip to the worker per pick. WritinglocalIdinto the buffer instead isn't an option. Snapping needsitemIdto fetch an item's geometry directly, and the pick keeps only 24 bits for the id. That boundsitemId, which is limited by the item count, but notlocalId, which is an IFC id.On the worker the mapping is two flatbuffer lookups:
meshes.meshesItems(itemId), thenlocalIds(index). Fragments could send it to the main thread as aUint32Arrayindexed byitemId:FragmentsModel, next to the asyncgetLocalIdsFromItemIds, which stays for compatibility.itemIdToLocalIdbecomes synchronous, so a pick no longer waits on the worker at all. This is a latency gain, not a correctness fix: fix(FastModelPicker):getFullPickconcurrency + vitest #777 already decodes everything before the lookup starts. It would also make synchronous picker methods possible, if they are ever wanted.setupTileSampleAttributessays a main-threaditemIdToLocalIdMapalready keeps this synchronous. No such table exists. This follow-up would make the comment true; otherwise the comment should be corrected.Additional context
Provided in #775.
Commits in this PR:
fix(FastModelPicker): getFullPick data eval— the fixvitest setup— vitest/vite toolchain, config and setup file, jest removed, can and probably should be its own PR ci(): vitest setup #805.fix(): dedicated circular deps- needed for tests to run.add test- cover the fixperf(): a single render pass— the consolidation, its tests and the canvas-snapshot helperrevert to async- revert breaking signature changes safeguarded by moving decoding ownership topickfix(): respect local clipping planes- fixes a latent bug: local clipping planes were not respected, global clipping planes were added twice.What is the purpose of this pull request?
Before submitting the PR, please make sure you do the following:
feat(examples): add hello-world example).fixes #123).🤖 Generated with the help of Claude Code