Parse through gcode-ast instead of the hand-rolled scanner - #487
Draft
sophiedeziel wants to merge 3 commits into
Draft
Parse through gcode-ast instead of the hand-rolled scanner#487sophiedeziel wants to merge 3 commits into
sophiedeziel wants to merge 3 commits into
Conversation
parseCommand delegates to gcode-ast parseLine plus its GCodeCommand compat view, and the local GCodeCommand class, GCodeParameters interface and isAlphaCode scanner are re-exported or deleted. Nothing else moves: thumbnails, slicer detection, layer metadata and the whole interpreter keep consuming the same GCodeCommand shape. Geometry is unchanged on every demo fixture. Two tokenizing cases regress and the ingestion cost roughly doubles; both are covered in the PR body.
The handler registry is keyed on the AST command type instead of a lower-cased mnemonic, and each handler receives its node already narrowed. Registering a handler under a type the parser cannot produce is now a compile error rather than an entry that never fires. What the types replace: probe re-derived the three G31 forms from a P word and an axis-word count, and now reads the node form; selectTool parsed the tool index out of the mnemonic, and now reads index, so T8 and above work instead of being dropped by a t0-t7 registry; setUnits is one handler because the node carries which unit it selects. G92 reads "bare" off words rather than the typed axis fields: the node names only X/Y/Z/E, so G92 F3000 would otherwise look bare and reset a workspace it should leave alone. Handler tests build their commands by parsing a real line instead of hand-pairing a mnemonic with a params bag, so they can no longer assert on a shape the parser would never produce. Geometry is unchanged on all seven demo fixtures. Modal continuation lines stay unregistered, so nothing that was ignored starts rendering.
Nothing here had a caller. - SceneManager._inches and _wireframe were assigned at their declaration and never read again. _wireframe was orphaned by the commented-out dev-gui control that used to drive it, removed here too. - A commented-out camera.updateProjectionMatrix() that has been inert since the commit that introduced it; the live call site is elsewhere in the same file. - A commented-out GridHelper alternative in BuildVolume.createGrid. - getAvailableParsers, reachable from its own test and nowhere else: it is not re-exported from the package entry, so no consumer can call it. Its test goes with it. - demo throttle, debounce and humanFileSize, none of them imported. home takes _command rather than carrying an eslint-disable: the registry fixes the argument position, so the parameter has to stay.
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.
Swaps
Parser.parseCommandto delegate togcode-ast'sparseLine+GCodeCommandcompat view, then rewires the interpreter to dispatch on the typed AST nodes instead of lower-cased mnemonics.Not for merge. Opened to make the tradeoff concrete.
Commit 1 — parse through gcode-ast
parseCommanddelegates toparseLine; the localGCodeCommandclass,GCodeParametersinterface andisAlphaCodescanner are re-exported or deleted. Thumbnails, slicer detection and layer metadata are untouched.Geometry is identical on all 7 demo fixtures (240 layers / 97,574 points on 3DBenchy, plus every
JobStatsfield and the bounding box).3DBenchy, parse → interpret:
spans: falsewere reachableraw: falsewere too (losessrc)Two tokenizing regressions — both real gaps in gcode-ast, both with existing tests here:
G 1 E 42 X 42(spaces between letter and number) →gcode: 'g'instead of'g1'G1 X1.2.3 Y5→ dropsxentirely; we take the leading1.2. Arguably better, still a behaviour change.The perf knobs are unreachable.
spans: falsedoesn't typecheck —toGCodeCommandrequires aspan.raw: falsedropsline.raw, andGCodeCommand.srcis public API asserted by two tests.GCodeCommand.nodeis most of the memory. The back-reference retains the whole typed node —words[],commandWord, spans — per line.Commit 2 — typed dispatch
The registry is keyed on the AST command type, and each handler receives its node narrowed. Registering a handler for a type the parser cannot produce is now a compile error instead of an entry that never fires.
probere-derived the three G31 forms from aPword and an axis-word count — now readscommand.formselectToolparsed the index out of the mnemonic — now readscommand.index, so T8+ works instead of falling off at0–t7registrysetInchUnits/setMillimeterUnitscollapse into onesetUnits; the node carries which unit it selectsGeometry is still identical on all 7 fixtures, and dispatch cost is neutral — the overhead is all in tokenizing. 873 / 875 tests pass (same 2 as commit 1). Coverage stays at 100%.
Two things worth review:
G92reads "bare" offwords, not the typed fields. The node names only X/Y/Z/E, soG92 F3000would look bare through them and reset a workspace it should leave alone — there is a test pinning exactly that.modalis deliberately unregistered. gcode-ast now emits a node for continuation lines likeF140; registering a handler would make CNC files gain geometry they don't render today. Separate decision, separate PR.Handler tests now build commands by parsing a real line instead of hand-pairing a mnemonic with a params bag, so they can no longer assert on a shape the parser would never produce.
Nothing here is public API —
Interpreter,handlersandCommandHandlerwere never exported.Still unsolved
Depends on
github:xyz-tools/gcode-ast#add-prepare-script— gcode-ast#14, needed because agithub:dep resolved to a package with nodist/. A real dependency means publishing to npm.Verdict
The swap works, the typed dispatch is a clear win on its own, and the geometry proves both. Still not mergeable at ~2.6× time and ~4× memory, and the remaining fixes are all upstream: the two tokenizing gaps, an opt-out-friendly
toGCodeCommandsignature, a way to skipnode, and npm.Assisted by Claude Code - Opus 5