Reduce technical debt: shared logic, narrower APIs, file splits - #210
Merged
Merged
Conversation
- attrs.rs: one value_at/prim_value plus decode_* helpers replace ~45 hand-written get_at::<sdf::Value> matches. Every f32 reader now accepts Half, the OpenPBR shader inputs accept Vec3d and int-authored bools. - prune_reason(): the abstract/inactive/purpose/invisible rule written once, shared by the traversal, the placement count and the prototype walk. - compose_with_parent(): the reset-xform-stack rule at one site. - assets.rs: one asset-path rule (authoring-layer anchoring everywhere, lights included) and one memoized, timed cached_asset() for every host decode. - camera.rs: CameraFrame, the one read build_camera and screen_projection both derive from. - existing_tiles(): the one 10x10 UDIM sweep (preload, streaming, .tx conversion, maketx); StreamingTexture::open loses its expand callback. - texture_cache.rs: the FIFO microcache set and MiB conversion shared by the .tx tile cache and Ptex streaming; drop the unused legacy env wrappers (ptex_*_from_env, ptex_stream_enabled, budget_from_env). All sample scenes bit-identical at 16 spp. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019nwge6NCTPhuk1VPuZucRF
- russian_roulette(): the survival test the surface bounce, the region phase scatter and the carried-medium scatter each pasted. - utils::exp3 replaces six per-component exp copies. - TRACE_T_MIN (crust_core::ray) names the 0.001 every trace asks for. - surface_visibility(): the one shadow-ray visibility (cutouts included) NEE and the learned light cache's training share, replacing the cache's own copy of the occluded/cutout logic. - LightList::pick_index_at forced inline: LLVM outlined it once trace_path grew, +0.6% instructions on cornellbox; inlined, the render runs 0.01% fewer than before this change (callgrind, 2 spp). - crust-rt: fixtures/mod.rs holds the uv_sphere, the three scenes and the ray batch ray_throughput, traversal_probe and the criterion bench shared by copy; their LCGs become openqmc::pcg::Rng. ray_throughput now reads --layout before building the default scenes. - mtlx_bench and jit_bench share bench_common (texture, points, A/B timer, material walk). All sample scenes bit-identical at 16 spp. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019nwge6NCTPhuk1VPuZucRF
- LightList: drop find_by_geom, pick_at, find_by_geom_at, iter_at and infinite_seen_by, which only tests used; tests read the index forms. - Retire CRUST_BVH_PACKET_SAH (its A/B is settled in the design record): packet-sized leaves are the only rule for all-triangle ranges, the per-primitive cost stays for ranges holding anything else. Removed from Config, CommitOptions and the builder; docs, specs and site updated. - crust-core: subsurface and commit_options are crate-private. - crust-mtlx: modules are private, every item has one path at the root (the nodedef tables and parse_literal/arity_of the tests use included). - crust-assets: tiled is private; make_tx/make_tx_atomic/TxFormat/MadeTx re-exported at the root, and the dead accessors that exposed removed. - RenderSettings::new(seven positional numbers) becomes Default plus with_resolution / with_max_depth / with_adaptive_sampling builders. - HitRecord carries uv: Option<(f32, f32)> instead of uv plus has_uv. - RayStats::merge destructures, so a new counter cannot be forgotten. - Guiding's quadtree descent draws from the K_GUIDE domain's rng() instead of a hand-rolled PCG32. cornellbox_guided changes by noise only: relmse 3.1e-2 / 2.1e-2 / 7.6e-3 at 16/64/256 spp against the old stream, equally far from an unguided reference. Every other sample is bit-identical. - Doc comments moved to their items (sample_bounce_direction in path.rs, triangulate in mesh.rs); tex_probe's gamma-2.2 lines say so. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019nwge6NCTPhuk1VPuZucRF
- scene/subdiv.rs (2,406 lines) -> subdiv/{mod,topology,uniform,adaptive,
normals,tests}.rs; tessellate_adaptive (588 lines) becomes a driver over
Cage, EdgeRating and Emitted, with tessellate_selected and
tessellate_unselected for its two phases.
- stats.rs: the 687-line Display::fmt dispatches to write_scene /
write_rays / write_textures / write_ptex / write_phases.
- closure::prepare: one prepare_* builder per BSDF kind over LeafInputs.
- usd_import/mesh.rs: mesh_source takes MeshNeeds (5 parameters, not 8)
and delegates to effective_scheme, vtx_boundary, SharpnessArrays,
refinable_chart and MeshSource::{tessellated,subdivided}.
- usd_import load_scene: ValidOptions, traverse_stage, resolve_camera and
record_geometry_counters taken out; ImageCounters reuses its From impl.
- trace_path takes a PathContext (6 parameters, not 12). Kept to the
inlined function only: handing &PathContext to the out-of-line helpers
cost cornellbox 0.08% (by value, 0.23%); as committed it runs 0.03%
fewer instructions than before (callgrind, 2 spp).
- crust-mtlx: eval.rs -> eval/{mod,apply,compile,tests}.rs; surface.rs ->
surface/{mod,open_pbr,standard_surface,gltf_pbr}.rs.
- crust-rt scene.rs -> scene/{mod,tests}.rs; commit_with sizes the arrays
in sized_primitives and expands each geometry through Expansion::add.
Fixes a latent bug found on the way: a skipped geometry (invalid disk or
cylinder, instance of an empty scene) pushed no GeomTable, but the
tables are indexed by geom_id, so every later triangle mesh read its
neighbour's bases and the last one indexed past the end (panic);
pinned by a_skipped_geometry_keeps_its_table_slot.
- crust-render: logging setup -> logging.rs, RenderProducts ->
products.rs, the traversal-stats table -> crust_core::traversal_report;
main() loses load_scene / apply_overrides / select_products (212 lines).
Every sample bit-identical at 16 spp; workspace tests pass. MaterialX
shading costs +0.055% instructions on materialx_surfaces (prepare_*).
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_019nwge6NCTPhuk1VPuZucRF
doubleailes
force-pushed
the
claude/tech-debt-cleanup
branch
from
October 5, 2026 08:59
95a6234 to
8328036
Compare
Contributor
PR Summary by QodoModularize renderer internals and consolidate shared rendering logic
AI Description
Diagram
High-Level Assessment
Files changed (124)
|
Contributor
Code Review by Qodo
1.
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
doubleailes
changed the title
Also the docs site's landing-page version badge. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019nwge6NCTPhuk1VPuZucRF
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
2 participants
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.
Summary
Pays off the technical-debt list in four commits: duplicated logic first, then rule violations and error-prone APIs, then the long functions and files.
Output. Every sample renders bit-identical at 16 spp except
cornellbox_guided(see Intentional behaviour changes). Workspace tests pass; fmt and clippy (-D warnings, also withtraversal-stats) are clean.1. Duplicated logic — USD import and texture caches
attrs.rs: onevalue_at/prim_valueplusdecode_*helpers replace ~45 hand-writtenget_at::<sdf::Value>matches.prune_reason(): the abstract / inactive / purpose / invisible rule, written once and shared by the traversal, the placement count and the prototype walk.compose_with_parent(): the reset-xform-stack rule in one place.assets.rs: one asset-path rule and one memoized, timedcached_asset()for every host decode.CameraFrame: the one camera read that bothbuild_cameraandscreen_projectionderive from.existing_tiles(): the one 10×10 UDIM sweep, used by preload, streaming,.txconversion andmaketx.texture_cache.rs: the FIFO microcache set and MiB conversion, shared by the.txcache and Ptex streaming. The unused legacy*_from_envwrappers are removed.2. Duplicated logic — integrator and fixtures
russian_roulette(),utils::exp3,TRACE_T_MIN, andsurface_visibility()(shared by NEE and the learned light cache's training).LightList::pick_index_atis forced inline; without it LLVM outlined it and cornellbox ran +0.6% instructions.crust-rt/fixtures/holds the scenes and ray batch the probes and the bench copied.mtlx_benchandjit_benchsharebench_common.3. Rule violations and error-prone APIs
tiledis private;make_tx,make_tx_atomic,TxFormatandMadeTxare re-exported at the root.subsurfaceandcommit_optionsare private, and the fiveLightListlookups only tests used are removed.CRUST_BVH_PACKET_SAHis retired; its A/B is settled in the intersection-kernel design record.RenderSettings::new(seven positional numbers) becomesDefaultplus named builders.HitRecord.uvis anOption(replacinguvplushas_uv), andRayStats::mergedestructures its fields.domain.rng()instead of a hand-rolled PCG32.4. Splits
subdiv.rs, crust-mtlxeval.rsandsurface.rs, and crust-rtscene.rsbecome directories, with their tests in their own files.tessellate_adaptive, the statsDisplay,closure::prepare,mesh_source,load_scene,main().trace_pathtakes aPathContext(12 → 6 parameters).logging.rsandproducts.rssplit out ofmain.rs; the traversal-stats table moves tocrust_core::traversal_report.Intentional behaviour changes
K_GUIDEdomain'srng()(CLAUDE.md: no RNG outside openqmc). Against the old stream,cornellbox_guidedrelmse is 3.1e-2 / 2.1e-2 / 7.6e-3 at 16 / 64 / 256 spp (--indirect-clamp 0), with no plateau. Old and new renders sit equally far from an unguided reference. Re-record thecornellbox_guidedgolden.ray_throughput/traversal_probe/ criterion fixtures now draw fromopenqmc::pcg::Rnginstead of an inline LCG (the same rule), so their rays and soup/instance-field scenes differ from earlier runs.CRUST_BVH_PACKET_SAHis gone. The per-triangle leaf cost is no longer selectable for all-triangle ranges; ranges holding other primitives keep it. Docs, site page and specs are updated.crust_mtlx::parse::Doc→crust_mtlx::Doc, etc.). Every crate ispublish = falseand every in-workspace caller is updated.half; OpenPBR shader inputs acceptvector3dand int-written bools.crust:samplesPerPixel = 0now renders 1 sample instead of 0.Bug fixed
crust-rtcommit: a geometry the build skips (an invalid disk or cylinder, or an instance of an empty scene) pushed noGeomTable. The tables are indexed bygeom_id, so every later triangle mesh read its neighbour's bases, and the last one indexed past the end and panicked. Pinned bya_skipped_geometry_keeps_its_table_slot.Measurements (callgrind, 2 spp)
materialx_surfaces: +0.055%, from theprepare_*split.&PathContextto out-of-line helpers cost 0.08% (by value, 0.23%), so it is kept to the inlinedtrace_path; the trap is documented on the type.Not run here
bvh8leg (no nightly toolchain installed).cargo deny(no dependency changes).🤖 Generated with Claude Code
https://claude.ai/code/session_019nwge6NCTPhuk1VPuZucRF