Repository navigation
Fix the gatehouse geometry, and build subassemblies off the model - #9
Merged
Merged
Conversation
Every fault in the gatehouse came from one confusion in scripts/make-demos.mjs: LDraw puts a part's origin on its *top* face, and the generator treated it as the bottom. So the towers and the span were referenced one plate above the courtyard instead of one brick above it, and sank sixteen units through the plate they were supposed to stand on, poking out the underside. The battlements went on at `-6 * BRICK`, which is where a seventh brick would go, not where a plate lands on the sixth, so every tower had two 1x1 plates floating sixteen units over it. And the corners at (+/-140, +/-100) put three of each tower's four columns off the plate entirely, with the fourth straddling the edge by half a stud, none of them on the stud grid. The span was rotated to run along an axis the courtyard is only 200 deep on, so half its 240 hung over nothing. The towers now sit at (+/-90, +/-70), where every column is on the grid and inside the footprint, and the span runs unrotated down the middle, where it spans the full width and clears all four towers. Numbers alone would just be a different guess, so `validate` now reads the emitted file back and fails the run unless every part rests on the ground or on another part's top face, lands on the stud lattice, and shares space with nothing. Reintroducing any of the three original bugs fails it, as does nudging a submodel half a stud off the grid. The wall demo goes. It existed to cover inferred build order, and that path keeps its unit tests in src/ldraw/steps.test.ts, but it had the same class of bug: coping tiles floating sixteen units up, and an asymmetric end brick embedded inside the 2x4 beside it.
An LDraw file records every brick at its position in the finished model, submodels included. Replayed literally, a subassembly assembles itself in mid-air inside the silhouette of a model that does not exist around it yet, which is the one thing in the whole build that cannot be what happened. Instruction booklets take you off to a corner of the page for exactly this reason. So the watch flow does the same. A submodel occurrence of five to forty bricks, at most a quarter of the model, built over more than one step and not standing on the ground, is displaced clear of the model while it is built and slides in on the step its last brick goes on. Occurrences, not files. The gatehouse references tower.ldr four times: one node in the submodel panel, because isolating "the towers" should light up all four, but four separate things to build, each with its own steps and its own place to be built. flattenModel now records both. The size bounds are where the work was. A real set nests whole stages inside one file, and staging those would mean building most of the model somewhere it does not belong, so the walk takes the outermost occurrence that looks like a subassembly and otherwise descends into it. Taking the outermost is also what keeps a brick in exactly one subassembly, so there is one displacement to apply rather than a chain of them. The floor of five is measured: the Saturn V nests forty-six three-brick submodels, and at three this staged eighty per cent of the set in units too small to register as units. The displacement is a pure translation, so the thing being built off to the side is recognisably the thing that will slot in, and it takes the cheapest of the five ways out of the model's silhouette. Escaping along the subassembly's own outward direction was the obvious rule and the wrong one: it sent the gatehouse span 271 units and a Galaxy Explorer hatch 825, because a piece over the middle of a model has to cross the whole of it. Choosing the shortest axis instead cuts those to 171 and 467, and for a piece sitting over a wide flat model it picks up, and the piece is lowered in. Down is excluded, because that is where the loose bricks are poured. Posing is now two legs: floor to staging on the brick's own step, then staging to model over the back half of the install step, so a subassembly is finished before it is fitted. A brick outside a subassembly has a zero displacement and a zero install, which is the same single flight from the floor as before. Framing is split. The camera has to take in the staging ground while watching or the subassembly is built where it cannot be seen, but build mode puts every brick straight where it belongs, so framing that empty ground there would only push the model away from the viewer. Build mode is otherwise untouched: ghosts, snapping and the carry all still aim at the final position.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
thebuilder
marked this pull request as ready for review
August 28, 2026 08:35
One conflict, in the bundled-models prose. Both sides had edited the same sentence: this branch because the wall demo is gone and the gatehouse is now the model that covers submodels, main because the README no longer has the OMR section sitting directly below it. Kept both, so the sentence describes one generated demo and no longer points "below". Note for anyone grepping: main landed src/scene/subassembly.ts, which is free build working out what is holding up what so picking a brick up brings what it was carrying. It is unrelated to src/ldraw/subassemblies.ts on this branch, which is submodel occurrences built off the model in the watch flow. No symbols collide; only the words do.
`fallow audit` gates on findings the changeset introduces, and this branch introduced four. Three were mine outright and one was an artefact worth fixing anyway. flattenModel was already long, and threading submodel occurrences through its walk pushed it over the cognitive threshold. The walk was doing two jobs: reading the loaded hierarchy, and deciding what a submodel boundary is. It is now `collectBricks`, with the boundary decision named as `descend`, which returns the path and occurrence a node's children belong to. Measuring a brick's own collider comes out too, as `measureCollider`; it was the deepest nesting in the function and reads better with a name on it than as the tail of a loop that is otherwise about world bounds. In the demo validator, `walk` and `collisions` both sat at cyclomatic 6. Nothing under scripts/ is covered, so CRAP collapses to cyclomatic² plus cyclomatic and six is over the ceiling. Parsing the reference lines is now `referencesIn`, building the box is `boxOf`, and the three-way overlap test is `sharesSpace`, which each read better named anyway. The fourth was disposeModel's traverse callback, unchanged by this branch but shifted down four lines, which is enough for the gate to read it as new. Pulling the material collection out drops it well under the ceiling. Everything else the gate reports in these files is inherited and stays that way: eight findings across pack-models.mjs, loadModel and the test fixtures, none of them touched here, all correctly attributed and excluded. Nothing is suppressed.
thebuilder
added a commit
that referenced
this pull request
Aug 28, 2026
Every decision in here came with the reasoning that produced it: why bags exist before what they are, why a hand-written palette would go stale before what the rules yield, a paragraph on instruction booklets before the subassembly rule. Read once it is interesting. Read while you are trying to find the threshold you came for, it is in the way. The facts stay, numbers included. What goes is the argument around them: 4,457 words to 3,234. Also fixes the test counts, which had drifted to 513 across 32 files, and drops the Running Bond Wall row, which left with #9.
This branch was successfully deployed
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
1 participant
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.
Two changes, from asking whether the invalid-looking build stages were the model files or how we parse them.
The answer was the model file, and we generate it.
The gatehouse was not buildable
Every fault in it came from one confusion in
scripts/make-demos.mjs: LDraw puts a part's origin on its top face, and the generator treated it as the bottom.-PLATEinstead of-BRICK, sinking 16 LDUonBase = -BRICK-6 * BRICKis where a seventh brick goes, not where a plate lands on the sixth-(TOWER_LEVELS - 1) * BRICK - PLATE(±140, ±100), none on the stud grid(±90, ±70)Confirmed against the real loader before and after. The repacked model now has 0 parts placed with nothing already built to attach to, and everything inside the base footprint.
Numbers alone would just be a different guess, so
validatereads the emitted file back and fails the run unless every part rests on the ground or another part's top face, lands on the stud lattice, and shares space with nothing. I mutation-tested it: reintroducing any of the three original bugs fails it, as does nudging a submodel half a stud off the grid.The wall demo is removed, as requested. It covered inferred build order, which keeps its unit tests in
src/ldraw/steps.test.tsbut no longer has a bundled model exercising it. It had the same class of bug anyway: coping tiles floating 16 LDU, and an asymmetric end brick embedded inside the 2x4 beside it.Subassemblies are built off the model
An LDraw file records every brick at its position in the finished model, submodels included. Replayed literally, a subassembly assembles itself in mid-air inside the silhouette of a model that does not exist around it yet. Instruction booklets take you off to a corner of the page for exactly this reason.
New
src/ldraw/subassemblies.ts. A submodel occurrence of 5–40 bricks, at most a quarter of the model, built over more than one step and not standing on the ground, is displaced clear of the model while built and slides in on the step its last brick goes on.Occurrences, not files: the gatehouse references
tower.ldrfour times, which is one node in the submodel panel but four things to build.flattenModelnow records both.Two decisions worth a reviewer's attention, both changed after measuring:
Posing is now two legs: floor → staging on the brick's own step, then staging → model over the back half of the install step, so a subassembly is finished before it is fitted. A brick outside a subassembly has a zero displacement and a zero install, which is the same single flight as before.
Camera framing is split (
stagedFramesvsbagFrames) so build mode is not pulled back to frame ground nothing uses.Selection on the bundled models: gatehouse 5, Galaxy Explorer 3, Saturn V 93, car and pyramid 0.
Build mode is unchanged — ghosts, snapping and the carry all still aim at the final position. Staging there would mean reworking all three plus the physics carry, which is a larger piece of work and not in this PR.
A correction to my own diagnosis
I initially reported that Saturn V had "185 parts supported only by a later step" and used that to argue loaded models were broken. That figure came from a vertical-support test, which flags every sideways/SNOT attachment as broken. Under a proper omnidirectional contact test the OMR sets are essentially clean: 0 of 368 parts in Galaxy Explorer and 16 of 1845 in Saturn V are placed with nothing already built to attach to.
So staging is here as a feature, for the reason booklets do it, not as a repair of broken OMR data. Worth knowing before reviewing it as a bug fix.
Parsing and packing were cleared and are untouched:
LDrawLoader.computeBuildingStepsalready numbers steps with a global depth-first counter, and the packer passes0 STEPthrough verbatim.Verification
One incidental note:
jsdomis declared inpackage.jsonbut was missing from the sharednode_modules, so 10 test files were erroring untilpnpm installwas re-run in the worktree. Nothing to fix in the repo.