test: regression test for issue #287 (setSample visibility/highlight mixup) - #295
Merged
agviegas merged 1 commit intoSep 11, 2026
Conversation
3 tasks
…ghlight mixup) PR ThatOpen#288 fixed setSample() forwarding high === 0 as updateTile()'s visible argument instead of the real vis value - silently decoupling a sample's rendered tile visibility from setVisible()/toggleVisible()'s real intent, confirmed reproducible on a real 18,382-sample/12,338-item model but not yet covered by an automated test. The original repro file is a private client model and can't be committed here. Adds a purely synthetic replacement instead: resources/frags/ synthetic_large_grid.frag, a 13,824-item grid of plain boxes (2 materials each, so most items carry 3 samples - 41,472 samples total, similar order of magnitude and per-item sample ratio to the original repro) generated with OpenSKP's writer + Fragments exporter. No real geometry or client data of any kind. Extensive tracing (documented in the test's own comment) found that a single, synchronous setVisible() call can't actually land a sample on setSample()'s current===past fast path in an in-process test - fetchLodLevel() reads the live visibility flag directly, so `current` always immediately reflects a toggle, routing through the always-correct updateVisible() path instead. The real trigger likely needs actual worker/event-loop interleaving between a setVisible() RPC and a concurrently-firing update() RPC, which this synchronous harness structurally can't produce - tried across single toggles, rapid re-toggles, toggleVisible() x2, and overlapping batches, on both a single- and multi-representation synthetic fixture, none of which discriminated the bug. Given that, this pins the fixed line directly instead - the same way view-manager.test.ts pins its own guard in isolation: calling setSample() with the exact (vis=false, high=0) combination that silently passed under the bug, asserting on tile.visibilities (the buffer the bug actually corrupts) rather than getVisible() (correct even under the bug, which is why the symptom was invisible to state-only assertions). Verified to fail on the pre-fix code and pass on main.
iamahsanmehmood
force-pushed
the
test/large-model-visibility-repro
branch
from
September 10, 2026 11:22
8c9d858 to
a7eac22
Compare
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.
What
Adds the automated test PR #288 mentioned as a follow-up ("happy to add a synthetic large-model example/test if that's the preferred way to land this") - #288 fixed
setSample()forwardinghigh === 0asupdateTile()'svisibleargument instead of the realvisvalue, confirmed reproducible on a real 18,382-sample/12,338-item production model but not yet covered by an automated test.Fixture
The original repro file was a private client model and can't be committed here.
resources/frags/synthetic_large_grid.fragis a purely synthetic replacement instead: a 13,824-item grid of plain boxes (2 materials each, so most items carry 3 samples via OpenSKP's material-based primitive batching - 41,472 samples total), generated with OpenSKP's writer + Fragments exporter. No real geometry or client data of any kind - similar order of magnitude and per-item sample ratio to the original repro, following CONTRIBUTING.md's "drop it inresources/and reference it by relative URL."Why this is a unit test, not a full e2e reproduction
Stated honestly rather than glossed over: I spent a while trying to actually reproduce the original symptom end-to-end (load fixture →
setVisible()→update()loop → assert on the rendered tile buffer) and couldn't land a sample onsetSample()'scurrent === pastfast path through any synchronous call sequence. Traced it directly against the real code:fetchLodLevel()reads_items.visible(itemId)live, socurrentalways immediately reflects a just-applied visibility change, which routes through the always-correctupdateVisible()path instead of the buggysetSample()one. Tried single toggles at 13,824 and 41,472 samples, rapid hide→show,toggleVisible()called twice back-to-back, and overlapping-batch re-shows - all identical (correct) whether the fix is present or reverted, across both a single-representation and multi-representation version of the fixture.My best guess is the real trigger needs actual worker/event-loop interleaving between a
setVisible()RPC and a concurrently-firingupdate()RPC (e.g. fromcamera.controls' own continuous "update" events), which an in-process, single-threaded test can't produce without standing up a realWorker/postMessageboundary.Given that, this pins the fixed line directly instead - the same way
view-manager.test.tspins its own guard in isolation (see its own docstring: "The unit tests pin each guard in isolation"). It callssetSample()with the exact(vis=false, high=0)combination that silently passed under the bug, and asserts ontile.visibilities- the actual buffer the bug corrupts - rather thangetVisible(), which stays correct even under the bug (exactly why the symptom was invisible to state-only assertions in the first place).Test plan
main(post-fix)expected true to be false) - the third (highlight state) correctly passes either way, sincehigh !== 0there and is documented as suchyarn workspace @thatopen/fragments testrun: no new failures (one pre-existing, unrelatedifc-splittertest failure confirmed present on a cleanmaincheckout too, untouched by this PR)