feat(kithree): real STEP geometry via occt-import-js + repair _append_lib_table_row (T10)#3
Merged
Merged
Conversation
Pulls the M4 "swap the placement box for the real STEP mesh" forward.
packages/kithree/src/step.ts decodes a .step into renderable buffers with
occt-import-js (OpenCASCADE compiled to wasm — the kernel KiCad itself uses,
so it tessellates the trimmed-NURBS B-rep real KiCad models carry). The wasm
loads lazily, once. loadThreeSceneWithModels() fetches + tessellates each
placement's model (cached by model_path so the N instances of one part share a
single parse) and renders real geometry, falling back to the colour-coded
placement box when a model is missing or fails to decode — the common offline
case, a real graceful degradation, not a stub.
kiserver GET /project/{id}/model3d resolves a footprint (model ...) reference
(KICAD7/8/9_3DMODEL_DIR + KICLAUDE_BUNDLED_LIBS prefixes, .wrl -> .step sibling
swap, path-traversal-safe) and streams the STEP bytes from the bundled mirror.
populate_libs.py seeds four real .step models into libs/packages3D/ (the parts
whose KiCad libraries ship a .step; ESP32-C6-MINI-1 and the U.FL connector ship
only .wrl upstream, so they box-fall-back).
Empirical gate: a real KiCad C_0402.step tessellates to non-trivial geometry
through the decoder, the route serves its exact bytes, and the scene loader
mounts it — 6 kithree (node) + 9 kiserver tests. Remaining for M4 is mounting
the async loader in the React Viewer/ThreePage (gateway model3d proxy + vite
wasm-asset serving) — pure UI wiring.
Also repairs a botched edit committed to main: _append_lib_table_row had a
half-applied threading change (a `with self._lock:` plus a duplicate docstring
and an unindented body) that left the kiserver module with an IndentationError,
breaking every kiserver test on import. Restored to a working state honouring
the thread-safety intent via a module-level _LIB_TABLE_LOCK (mirroring
library._CACHE_LOCK).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
Reviewer's GuideImplements real STEP-based 3D model support in kithree using occt-import-js, exposes an async scene loader that uses decoded geometry with caching and graceful fallback, adds a kiserver route to serve STEP models from the bundled mirror with safe path resolution, seeds .step models into the libs mirror and manifest, and repairs a broken threaded lib-table append helper. Sequence diagram for loading STEP geometry with fallbacksequenceDiagram
actor Client
participant Kithree as loadThreeSceneWithModels
participant Fetcher as ModelFetcher
participant Server as project_model3d
participant Occt as decodeStep
Client->>Kithree: loadThreeSceneWithModels(scene, opts)
loop placements
alt opts.fetchModel && placement.model_path
Kithree->>Fetcher: fetchModel(placement.model_path)
Fetcher->>Server: GET_/project/{project_id}/model3d(path)
Server->>Server: _resolve_model_relpath(path)
Server->>Server: bundled_libs_dir()
Server-->>Fetcher: STEP_bytes (Response)
Fetcher-->>Kithree: Uint8Array | null
alt bytes available
Kithree->>Occt: decodeStep(bytes)
Occt->>Occt: ReadStepFile(bytes, null)
Occt-->>Kithree: StepMesh[]
Kithree->>Kithree: mergeStepMeshes(meshes)
Kithree->>Kithree: buildPlacementModel(...)
else decode/fetch failed
Kithree->>Kithree: buildPlacementMarker(...)
end
else no fetchModel or no model_path
Kithree->>Kithree: buildPlacementMarker(...)
end
end
Kithree-->>Client: LoadedScene
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request implements task T10, pulling forward STEP geometry parsing from M4. It introduces 3D STEP model loading and rendering in the kithree package using occt-import-js (OpenCASCADE compiled to WebAssembly) and adds a corresponding /project/{project_id}/model3d endpoint to kiserver to serve model bytes from a bundled mirror. Feedback focuses on optimizing the loading process and fixing a few issues with asynchronous/lazy execution: resolving sequential fetch bottlenecks by loading models in parallel, avoiding blocking synchronous I/O in the server's async endpoint, reusing materials across component meshes to reduce WebGL overhead, and fixing a static import that was defeating the intended lazy dynamic loading of the large WASM module.
Comment on lines
+268
to
+283
| // Decode each distinct model once; many placements share a model_path. | ||
| const geomCache = new Map<string, BufferGeometry | null>(); | ||
|
|
||
| for (const placement of scene.placements) { | ||
| let mesh: Mesh | null = null; | ||
| if (opts.fetchModel && placement.model_path) { | ||
| const geom = await resolveModelGeometry( | ||
| placement.model_path, | ||
| opts.fetchModel, | ||
| geomCache, | ||
| resources, | ||
| ); | ||
| if (geom) { | ||
| mesh = buildPlacementModel(placement, geom, resources, centroid, scene.board_thickness_mm); | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Currently, the 3D models are fetched and decoded sequentially in a for...of loop using await. If a board has many components, this sequential execution creates a significant performance bottleneck because each model fetch and decode must wait for the previous one to complete.
We can dramatically improve performance by pre-fetching and decoding all unique model paths in parallel using Promise.all before the loop. Additionally, we can instantiate a single shared MeshStandardMaterial for the default model color and reuse it across all placements, rather than creating a new material instance for every single component.
// Pre-fetch and decode all unique models in parallel to avoid sequential network/decoding bottlenecks.
const geomCache = new Map<string, BufferGeometry | null>();
if (opts.fetchModel) {
const uniquePaths = Array.from(
new Set(
scene.placements
.map((p) => p.model_path)
.filter((path): path is string => !!path)
)
);
await Promise.all(
uniquePaths.map(async (path) => {
const geom = await resolveModelGeometry(
path,
opts.fetchModel!,
geomCache,
resources
);
geomCache.set(path, geom);
})
);
}
const defaultModelMaterial = new MeshStandardMaterial({
color: new Color(DEFAULT_MODEL_COLOR),
metalness: 0.4,
roughness: 0.5,
});
resources.materials.push(defaultModelMaterial);
for (const placement of scene.placements) {
let mesh: Mesh | null = null;
if (opts.fetchModel && placement.model_path) {
const geom = geomCache.get(placement.model_path) || null;
if (geom) {
mesh = buildPlacementModel(placement, geom, defaultModelMaterial, centroid, scene.board_thickness_mm);
}
}
Comment on lines
+334
to
+347
| function buildPlacementModel( | ||
| placement: ScenePlacement, | ||
| geometry: BufferGeometry, | ||
| resources: { geometries: BufferGeometry[]; materials: Material[] }, | ||
| centroid: { x: number; y: number }, | ||
| boardThicknessMm: number, | ||
| ): Mesh { | ||
| const mat = new MeshStandardMaterial({ | ||
| color: new Color(DEFAULT_MODEL_COLOR), | ||
| metalness: 0.4, | ||
| roughness: 0.5, | ||
| }); | ||
| resources.materials.push(mat); | ||
| const mesh = new Mesh(geometry, mat); |
Contributor
There was a problem hiding this comment.
Update buildPlacementModel to accept a shared Material instance instead of the resources object. This avoids creating duplicate MeshStandardMaterial instances for every placement, reducing WebGL state changes and shader compilation overhead.
function buildPlacementModel(
placement: ScenePlacement,
geometry: BufferGeometry,
material: Material,
centroid: { x: number; y: number },
boardThicknessMm: number,
): Mesh {
const mesh = new Mesh(geometry, material);
Comment on lines
+28
to
+38
| let _occt: Promise<OcctModule> | null = null; | ||
|
|
||
| /** Lazily import + instantiate the OCCT wasm module (once per process). The | ||
| * dynamic import keeps the ~7 MB wasm out of the initial bundle until a board | ||
| * actually needs a 3D model. */ | ||
| async function occt(): Promise<OcctModule> { | ||
| if (_occt === null) { | ||
| _occt = occtimportjs(); | ||
| } | ||
| return _occt; | ||
| } |
Contributor
There was a problem hiding this comment.
The comment states that a dynamic import is used to keep the ~7 MB WASM out of the initial bundle. However, occtimportjs is statically imported at the top of the file (import occtimportjs from "occt-import-js"), which means bundlers will eagerly include it in the main bundle anyway.
To achieve true lazy loading and keep the large WASM module out of the initial bundle, we should dynamically import occt-import-js inside the occt() function.
let _occt: Promise<OcctModule> | null = null;
/** Lazily import + instantiate the OCCT wasm module (once per process). The
* dynamic import keeps the ~7 MB wasm out of the initial bundle until a board
* actually needs a 3D model. */
async function occt(): Promise<OcctModule> {
if (_occt === null) {
_occt = (async () => {
const { default: occtimportjs } = await import("occt-import-js");
return occtimportjs();
})();
}
return _occt;
}![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- In
loadThreeSceneWithModels, each placement’s model fetch/decoding is awaited sequentially inside theforloop; consider batching withPromise.all(while still reusing thegeomCache) so scenes with many placements don’t incur unnecessary per-placement latency. resolveModelGeometryswallows all exceptions silently, which makes debugging STEP decoding failures harder; consider at least emitting a scoped log/warning (with themodelPath) before returningnullso it’s clear when/why the model fallback path is being used.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `loadThreeSceneWithModels`, each placement’s model fetch/decoding is awaited sequentially inside the `for` loop; consider batching with `Promise.all` (while still reusing the `geomCache`) so scenes with many placements don’t incur unnecessary per-placement latency.
- `resolveModelGeometry` swallows all exceptions silently, which makes debugging STEP decoding failures harder; consider at least emitting a scoped log/warning (with the `modelPath`) before returning `null` so it’s clear when/why the model fallback path is being used.
## Individual Comments
### Comment 1
<location path="packages/kithree/src/step.ts" line_range="101-105" />
<code_context>
+ }
+ let vertexFloats = 0;
+ let indexCount = 0;
+ for (const m of meshes) {
+ vertexFloats += m.positions.length;
+ indexCount += m.indices.length;
+ }
+ const positions = new Float32Array(vertexFloats);
+ const normals = new Float32Array(vertexFloats);
+ const indices = new Uint32Array(indexCount);
+ let floatOffset = 0;
+ let indexOffset = 0;
+ let baseVertex = 0;
+ let anyMissingNormals = false;
+ for (const m of meshes) {
+ positions.set(m.positions, floatOffset);
+ if (m.normals) {
+ normals.set(m.normals, floatOffset);
+ } else {
+ anyMissingNormals = true;
+ }
+ // Shift this solid's indices past the vertices already written.
+ indices.set(
+ m.indices.map((i) => i + baseVertex),
+ indexOffset,
</code_context>
<issue_to_address>
**suggestion (performance):** Using `Array.map` inside the merge loop allocates per-solid temporary arrays and can be avoided.
In `mergeStepMeshes`, `indices.set(m.indices.map((i) => i + baseVertex), indexOffset)` creates a new array for each mesh. Given you already know the sizes, this extra allocation is avoidable and adds GC pressure for large or many meshes. Instead, iterate over `m.indices` and write directly into `indices`, applying `+ baseVertex` in place to keep this path allocation-free.
```suggestion
// Shift this solid's indices past the vertices already written, without
// allocating a temporary array.
for (let i = 0, len = m.indices.length; i < len; i++) {
indices[indexOffset + i] = m.indices[i] + baseVertex;
}
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
Co-authored-by: sourcery-ai[bot] <58596630+sourcery-ai[bot]@users.noreply.github.com>
…ort (T10) loadThreeSceneWithModels awaited each placement's fetch+OCCT decode inside the per-placement loop, serialising network and tessellation across the whole board. Pre-fetch every unique model_path up front via Promise.all so the loop is a pure CPU walk over cached geometry, and share one MeshStandardMaterial across all real-model placements instead of allocating one per component (buildPlacementModel now takes the material from its caller). step.ts statically imported occt-import-js, so bundlers pulled the ~7 MB OCCT wasm into the initial chunk despite the lazy-load comment. Make the top-level import type-only and dynamically import the module inside occt(), so the wasm lands in a chunk fetched only when a board first decodes a model. T10 contract preserved: distinct model_path fetched/tessellated once, geometry shared across instances. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…epMeshes typecheck
Resolved the step.ts import conflict in favour of the complete lazy-load fix:
type-only OcctModule import + dynamic import("occt-import-js") inside occt().
The incoming branch had dropped the static value import without adding the
dynamic one, leaving occtimportjs undefined (would fail typecheck/runtime).
Kept the incoming allocation-free mergeStepMeshes loop, but iterate the typed
array directly so each element types as number — an indexed read is
number | undefined under noUncheckedIndexedAccess (TS2532), which the incoming
loop tripped. Typecheck clean, 24/24 kithree tests pass.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…T9 follow-up) The blinky fixture was corrected to real KiCad part libraries in T9 (5f9fe73): the ESP32-S3-WROOM-1 module footprint lives in RF_Module.pretty (RF_Module:ESP32-S3-WROOM-1), while MCU_Espressif is the schematic symbol lib. The on-disk-fixture assertion still expected the old MCU_Espressif id and failed. Track the fixture. 483/483 client vitest tests pass. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
`uv sync` installed pytest (declared as service extras) but never ruff or mypy (declared nowhere), so `uv run ruff check` failed to spawn and the python workflow went red. Declare both in a root [dependency-groups] dev (PEP 735) so `uv sync` installs them, and relock. ruff passes clean across services/ (hard gate). mypy --strict has a large pre-existing backlog (~452 errors — mostly missing PEP 561 py.typed markers and the Any leakage that cascades, plus a multi-src-root module-resolution issue); its step is continue-on-error here pending a dedicated strict-cleanup PR. pytest stays a hard gate (527 passed locally). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
… B gate)
The Tier B golden test every_shipped_kicad_reference_board_parses walks
.kicad_pcb files under development/resources/kicad/{kicad-library,freerouting}.
Those are gitignored external checkouts absent from a fresh CI checkout, so the
gate asserted walked>=1 over an empty tree and panicked (exit 101) — broken in
CI since the initial commit. Fetch both repos (pinned to the dev-tree commits,
shallow single-commit) before any cargo test. 18 reference boards available.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ites Two fixes. (1) pnpm install resolves client's file:../crates/*/pkg deps, which are gitignored wasm-pack outputs — build ki+cad wasm before install or it fails ERR_PNPM_LINKED_PKG_DIR_NOT_FOUND. (2) `pnpm -r test` also ran the Playwright packages (tests/e2e,perf,a11y) that need browsers + a dev server this job never sets up; narrow to `--filter '!./tests/*'` so node runs the 4 vitest suites only (Playwright runs in the e2e workflow). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Same ERR_PNPM_LINKED_PKG_DIR_NOT_FOUND as the node workflow: cargo-deny ran fine, then pnpm install failed on the missing crates/*/pkg wasm dirs. Add the wasm32 target + wasm-pack and build ki+cad before install. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
pnpm install ran before the wasm-pack builds (ordering bug) → install failed. Reorder so wasm is built first. Also run the a11y suite next to e2e: it is deterministic and auto-starts its own client dev server (:5318). perf is left out of CI on purpose — its >=60 FPS assertion is GPU/HW-dependent and would flake on headless shared runners; it stays a local/manual benchmark. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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.
mainAfter PR #2 merged,
services/kiserver/src/kiserver/main.pycarried ahalf-applied threading edit to
_append_lib_table_row— awith self._lock:plus a duplicate docstring and an unindented body — which is a hard
IndentationError. That breaks the entire kiserver module on import, soevery kiserver test currently fails on
main. (This is the third botchedweb edit this session, after
mouser.pyandBomView.tsx.)The fix completes the evident thread-safety intent with a module-level
_LIB_TABLE_LOCK(mirroringlibrary._CACHE_LOCK) rather than thefunction-attribute lock pattern — i.e. it restructures that edit to make it
work. It rides in this PR because it's the same file as the T10 route (can't
hunk-split).
T10 — real STEP geometry parsing (FR-029 / SPEC §6.6 / D6)
Pulls the M4 "swap the placement box for the real STEP mesh" forward.
packages/kithree/src/step.ts—decodeStep()tessellates a.stepintorenderable buffers via occt-import-js (OpenCASCADE wasm, the kernel KiCad
itself uses, so it handles the trimmed-NURBS B-rep real KiCad models carry).
Lazy one-time wasm init;
mergeStepMeshes()folds multi-solid parts into onegeometry.
loadThreeSceneWithModels()— fetches + tessellates each placement's model(cached by
model_path, one parse shared across instances) and renders realgeometry, falling back to the colour-coded placement box when a model is
missing/undecodable (the common offline case — graceful degradation, not a stub).
GET /project/{id}/model3d— resolves a footprint(model …)ref(
KICAD7/8/9_3DMODEL_DIR+KICLAUDE_BUNDLED_LIBSprefixes,.wrl→.stepsibling swap, path-traversal-safe) and streams STEP bytes from the mirror.
populate_libs.pyseeds 4 real.stepmodels intolibs/packages3D/(ESP32-S3-WROOM-1, BGA-16, C_0402, C_0603; ESP32-C6-MINI-1 + U.FL ship only
.wrlupstream → box-fallback).Verification
C_0402.steptessellates to non-trivialgeometry through the decoder; the route serves its exact bytes; the scene
loader mounts it. 6 kithree (node) + 9 kiserver tests.
manifest pins verified.
Scope notes (read before assuming "open viewer → see models")
ThreePage/Viewerstillcall the sync box loader; the real path is reachable from kithree code but not
yet mounted. Remaining M4 UI wiring: a gateway
model3dproxy + vitewasm-asset serving + swapping the viewer to
loadThreeSceneWithModels.end-to-end (the scene test feeds bytes from disk, not through the route) —
that composition is exercised when the viewer mount lands.
🤖 Generated with Claude Code
Summary by Sourcery
Introduce real STEP-based 3D model support for kithree and a safe kiserver model-serving route, while fixing a lib-table concurrency bug.
New Features:
Bug Fixes:
Enhancements:
Tests: