Skip to content

feat(ir): add PartLoader convenience API for one-shot part loads#72

Open
bkfunk wants to merge 2 commits into
segfault87:mainfrom
bkfunk:feature/oneshot-partloader
Open

feat(ir): add PartLoader convenience API for one-shot part loads#72
bkfunk wants to merge 2 commits into
segfault87:mainfrom
bkfunk:feature/oneshot-partloader

Conversation

@bkfunk
Copy link
Copy Markdown

@bkfunk bkfunk commented May 13, 2026

Summary

The minimal recipe to load and bake one part by ID currently takes ~7 lines of fairly opaque ceremony:

let loader = LocalLoader::new(Some(library_root), None);
let colors = loader.load_colors().await?;
let alias = PartAlias::from(format!("{part_id}.dat"));         // must remember .dat
let (_, document) = loader.load_ref(alias, false, &colors).await?;
let cache = Arc::new(RwLock::new(PartCache::default()));       // pure overhead for one-shot
let resolutions = resolve_dependencies_multipart(
    &document, cache, &colors, &loader, &|_, _| {}             // mandatory no-op
).await;
let part = bake_part_from_multipart_document(&document, &resolutions, false);

Friction points:

  1. Manual .dat suffix — easy to forget; produces a "not found" error that doesn't explain why.
  2. Arc<RwLock<PartCache>> is required — for a single-shot load there's nothing to share, so the lock is pure overhead.
  3. load_colors() repeats per call — color catalog is invariant per library; for a UI loading 50 parts, that's 50 redundant LDConfig.ldr parses.
  4. No-op on_update callback — most callers don't care; the API requires passing something.

Change

Adds ldraw_ir::loader::PartLoader<L>, a pre-warmed wrapper that owns the loader, the color catalog, and an internal PartCache. The recipe collapses to:

let loader = PartLoader::from_library_root(library_root).await?;
let part = loader.load_part("3001").await?;

The .dat/.ldr/.mpd suffix is added automatically when missing. The internal cache is reused across calls so successive loads amortize primitive parsing.

Public API

Constructor Use case
PartLoader::new(loader, colors) BYO loader + already-loaded catalog
PartLoader::with_loaded_colors(loader) BYO loader; load colors once up front
PartLoader::<LocalLoader>::from_library_root(path) Filesystem library (most common case)
Method Returns
load_part(part_id: &str) Result<Part, ResolutionError>
colors() -> &ColorCatalog Borrowed catalog
inner() -> &L Underlying loader (escape hatch)

Test plan

  • cargo check --workspace passes
  • cargo test -p ldraw-ir --lib loader passes (unit tests for .dat/.ldr/.mpd suffix handling, including case-insensitivity and whitespace trimming)
  • cargo doc -p ldraw-ir --no-deps produces no broken intra-doc links

The minimal recipe to load and bake a single part by ID currently
takes about 7 lines of ceremony:

    let loader = LocalLoader::new(Some(library_root), None);
    let colors = loader.load_colors().await?;
    let alias = PartAlias::from(format!("{part_id}.dat"));
    let (_, document) = loader.load_ref(alias, false, &colors).await?;
    let cache = Arc::new(RwLock::new(PartCache::default()));
    let resolutions = resolve_dependencies_multipart(
        &document, cache, &colors, &loader, &|_, _| {}).await;
    let part = bake_part_from_multipart_document(
        &document, &resolutions, false);

Friction points:

  1. Manual `.dat` suffix — easy to forget; failure mode is a confusing
     "file not found".
  2. Required `Arc<RwLock<PartCache>>` even when there's nothing to
     share — pure overhead for one-shot loads.
  3. `load_colors()` repeats on every call — for a UI loading 50 parts,
     that's 50 redundant `LDConfig.ldr` parses.
  4. Mandatory `on_update` callback — most callers don't care but the
     API requires passing something.

New `ldraw_ir::loader::PartLoader` collapses all four:

    let loader = PartLoader::from_library_root(library_root).await?;
    let part = loader.load_part("3001").await?;

It owns the loader, the color catalog, and the part cache. The
`.dat`/`.ldr`/`.mpd` suffix is added automatically. The cache is
reused across calls so successive loads amortize primitive parsing.

Public API:
  - `PartLoader::new(loader, colors)` — BYO catalog
  - `PartLoader::with_loaded_colors(loader)` — load colors once up front
  - `PartLoader::<LocalLoader>::from_library_root(path)` — most common case
  - `load_part(part_id)` — returns a baked `Part`
  - `colors()`, `inner()` — accessors

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings May 13, 2026 18:49
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR introduces a new convenience API in ldraw-ir to load and bake a single LDraw part by ID with significantly less boilerplate, by wrapping an existing LibraryLoader together with a cached ColorCatalog and an internal PartCache.

Changes:

  • Added ldraw_ir::loader::PartLoader<L> with constructors for “BYO loader + colors”, “BYO loader + load colors once”, and a filesystem convenience constructor for LocalLoader.
  • Added load_part(&str) which auto-appends a default .dat suffix when no LDraw extension is provided, plus unit tests for the suffix behavior.
  • Exported the new loader module from ldraw-ir.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File Description
ir/src/loader.rs Adds PartLoader, suffix normalization helper, and unit tests to simplify one-shot part loading and reuse cached colors/cache across calls.
ir/src/lib.rs Exposes the new loader module as part of the public ldraw-ir API.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread ir/src/loader.rs Outdated
Comment on lines +18 to +29
use std::{
path::PathBuf,
sync::{Arc, RwLock},
};

use ldraw::{
color::ColorCatalog,
error::ResolutionError,
library::{LibraryLoader, PartCache, resolve_dependencies_multipart},
resolvers::local::LocalLoader,
PartAlias,
};
Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch — addressed in 8cb0746. PathBuf/LocalLoader imports and the impl PartLoader<LocalLoader> block are now gated behind #[cfg(not(target_arch = "wasm32"))]. The module-level doctest is cfg-gated similarly. Verified cargo check -p viewer_web --target wasm32-unknown-unknown still passes.

Comment thread ir/src/loader.rs Outdated
Comment on lines +85 to +98
pub async fn load_part(&self, part_id: &str) -> Result<Part, ResolutionError> {
let alias = alias_with_default_suffix(part_id);
let (_, document) = self.loader.load_ref(alias, false, &self.colors).await?;
let resolutions = resolve_dependencies_multipart(
&document,
Arc::clone(&self.cache),
&self.colors,
&self.loader,
&|_, _| {},
)
.await;
Ok(bake_part_from_multipart_document(
&document, &resolutions, false,
))
Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair point — addressed in 8cb0746. Two changes:

  1. load_part now calls PartCache::collect(CacheCollectionStrategy::Parts) after each bake. Primitives stay cached (matching the documented "successive loads amortize primitive parsing"), but full part documents are released once the baked Part has been returned, so a long-lived PartLoader no longer grows unboundedly.

  2. Added a public clear_cache(strategy) method so callers who want a harder reset can pass CacheCollectionStrategy::PartsAndPrimitives and drop primitives too.

Updated the struct-level and load_part doc comments to reflect the new lifecycle.

…cache growth

Two follow-ups from review on segfault87#72:

- `LocalLoader` is `cfg(not(target_arch = "wasm32"))`. The original PR
  imported it unconditionally and provided `impl PartLoader<LocalLoader>`
  unconditionally, which would have broken `ldraw-ir` consumers building
  for wasm (e.g. `viewer-common` via `viewer_web`). Move the import,
  the `from_library_root` constructor, and the `PathBuf` import behind
  the same cfg. Verified `cargo check -p viewer_web --target
  wasm32-unknown-unknown` still passes. The module-level doctest is
  cfg-gated similarly.

- The internal `PartCache` was never collected, so a long-lived
  `PartLoader` would accumulate every full part document it ever
  loaded. `load_part` now calls
  `PartCache::collect(CacheCollectionStrategy::Parts)` after each bake
  — primitives remain cached (matching the documented "successive loads
  amortize primitive parsing"), but full part documents are released
  once the baked `Part` has been returned. Also expose a public
  `clear_cache(strategy)` method so callers can drop primitives too
  (`PartsAndPrimitives`) when they want a hard reset.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants