feat(dsl): seeded domain randomization for background, light and distractors - #97
Closed
Nitjsefnie wants to merge 8 commits into
Closed
feat(dsl): seeded domain randomization for background, light and distractors#97Nitjsefnie wants to merge 8 commits into
Nitjsefnie wants to merge 8 commits into
Conversation
Typed pydantic spec module: background, light, and distractor sub-specs, each knob a closed (min, max) interval validated against inversion. Sampling is pure and seeded via numpy.random.default_rng with a fixed draw order, so a spec + seed deterministically yields a concrete RandomizationSample. Co-Authored-By: Kimi K3 <[email protected]>
…add#41) SceneBuilder.randomize(spec, seed=...) samples the spec and applies it: sampled background/light land on new optional Scene fields, distractors go through the existing .distractor entry point, and a RandomizationRecord sidecar (spec + seed) rides on the scene. All fields default to None, so an un-randomized scene is byte-identical; the pyrender backend consumes the sampled background/light when present. Co-Authored-By: Kimi K3 <[email protected]>
Covers byte-identical sampling per seed, divergence across seeds, inverted- interval rejection, un-randomized scenes staying byte-identical (manifest and scene JSON), the distractor count knob flowing through the existing distractor path, and the provenance sidecar round-tripping and reproducing the sample. Co-Authored-By: Kimi K3 <[email protected]>
New DESIGN.md section under DSL assembly: what each knob means, its units, its default interval, and the same-spec-plus-same-seed reproducibility contract. Co-Authored-By: Kimi K3 <[email protected]>
A second .randomize(...) call restarted distractor numbering at 0, producing duplicate entity ids in the scene and manifest and silently overwriting the first batch in build()'s frames_by_id. Ids now come from a per-builder counter, so a single call keeps the stable rand_distractor_0..N-1 names and repeat calls cannot collide. Co-Authored-By: Kimi K3 <[email protected]>
NaN bounds slip past every min > max comparison and then crashed inside sample() with numpy's OverflowError. The shared interval check now rejects non-finite bounds, so LightSpec/DistractorSpec fail fast with a ValidationError naming the field (BackgroundSpec's channel range already rejected them). Co-Authored-By: Kimi K3 <[email protected]>
…d#41) PyrenderBackend(bg=...) already made the background configurable per renderer; the new knob is scene-level, randomizable control that takes precedence over that constructor default. Say so in DESIGN.md and the BackgroundSpec docstring, and document the precedence at the override point in render.py. Co-Authored-By: Kimi K3 <[email protected]>
bamdadd#41) test_golden_sample_pins_the_shipped_draw_order asserts the sampled values for one fixed spec and seed against pasted-in numeric literals, so a change to draw order or seed handling goes red even though sampling stays deterministic (verified: both the reordered-draws and default_rng(seed+1) mutations fail it). Also covers unique ids across repeated randomize calls and NaN/inf rejection for every spec. Co-Authored-By: Kimi K3 <[email protected]>
Nitjsefnie
force-pushed
the
feat/41-domain-randomization
branch
from
July 31, 2026 10:03
cfefe27 to
90fd878
Compare
Owner
|
Merged as f1e97ad. Thank you @Nitjsefnie — this is a lovely, disciplined feature: frozen typed specs mirroring noise.py, a pure seeded sample() with a fixed draw order, and the additive off-by-default contract verified by byte-identical golden manifests. The pinned golden-sample test and the NaN/inverted-bound rejection are exactly the right rigor. Appreciated the honest write-up of what was fixed in review and the pre-existing #96 you surfaced rather than smuggled a guard for. 329 passed locally, ruff/format/mypy clean. |
Owner
|
Landed via local squash-merge (author preserved as @Nitjsefnie) in f1e97ad on main; closing this fork PR. |
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.
Summary
Typed, seeded domain-randomization knobs for the Scene builder — background, light and N distractors — sampled deterministically from a pydantic spec, applied through a single
SceneBuilder.randomize(spec, seed=...)entry point. Everything is additive and off by default: a scene that never callsrandomizebuilds a byte-identical manifest.Related Issues and Pull Requests
Fixes #41
Changes
src/multicam_sim/randomization.py(new) —RandomizationSpeccomposed ofBackgroundSpec,LightSpecandDistractorSpec, all frozen pydantic models in the style ofnoise.py. Every knob is a closed(min, max)interval; amodel_validatorrejects inverted and non-finite bounds.sample(seed)returns a concrete typedRandomizationSamplevianumpy.random.default_rng(seed), with a fixed draw order.src/multicam_sim/dsl/builder.py—randomize(spec, *, seed=0)samples, stashes background and light forbuild(), and adds each sampled position through the existingdistractor(...)method so randomized distractors are ordinary entities. A per-builder counter keeps generated ids unique.src/multicam_sim/scene.py—Scene.background,Scene.lightandScene.randomization(aRandomizationRecordof spec + seed, so a randomized run is reproducible from its own output). All default toNone; the manifest builder never reads them.src/multicam_sim/dsl/render.py— the pyrender backend consumesscene.background/scene.lightwhen set, with a_light_posehelper. Absent them the render is unchanged.DESIGN.md— the knobs, their units, their default intervals, and the reproducibility contract.tests/test_randomization.py(new) — 35 tests.Only three of the issue's four knobs are implemented. Textures are not, deliberately: the codebase has no texture concept to hang one off — entities render as untextured spheres coloured by a stable id-derived RGB — so a texture knob would first need a material field on the render specs. That is renderer-side schema work and your call, not something to guess at here.
Testing
uv run ruff check .,uv run ruff format --check .,uv run mypy srcall clean.uv run pytest: 284 passed, 4 skipped onupstream/main→ 319 passed, 4 skipped on this branch, same plainuv syncboth sides.Manifests for all example scenes are byte-identical to
upstream/mainwhenrandomizeis not called — checked by sha256 and byte length, not by inspection.This PR went through an adversarial review before it was opened, and four things came back that are worth stating plainly, because three of them were wrong in the first version:
sample()being byte-identical for the same seed holds for any deterministic implementation, so a mutation that reordered the draws — or usedseed + 1— changed every sampled value and still left all tests green. There is now a golden test asserting the shipped sample for one fixed spec and seed against pasted-in literals that share no constant with production code; both mutations turn it red.randomizetwice produced duplicate entity ids. The ids restarted atrand_distractor_0, so the manifest carried duplicates andframes_by_idsilently kept only the second. Fixed with a per-builder counter, plus a test asserting all ids are unique after two calls.PyrenderBackend(bg=...)exists onmain(render.py:110); what is new here is scene-level and randomizable control. The docs now say that, and state the precedence —scene.backgroundwins over the constructorbgwhen set. Say if you would rather the explicit constructor argument win.min > maxcheck never fired) and then failed insidesample()with numpy'sOverflowError. Non-finite bounds are now rejected at construction, with 12 parametrized cases.Bugs Discovered
That one is pre-existing and independent of this PR — two plain
.distractor("dup", ...)calls reproduce it onmainwith no randomization involved. I deliberately did not add a guard toentity()ordistractor()here, since that changes existing public behaviour and belongs in its own change rather than buried in a feature PR.Follow-ups / Known Limitations
KubricSceneSpecwith a light field. The values are on theSceneand in the provenance sidecar, so any backend can pick them up later.SceneJSON now carries"background": null, "light": null, "randomization": nullon un-randomized scenes, the same shape thepossessionsidecar introduced. The byte-golden contract is the manifest (Manifest.to_json(exclude_none=True)), which is untouched, and no production code serializes whole scenes to disk.