Deliver the scene format and fixture scenes (1.2) - #9
Draft
leynos wants to merge 3 commits into
Draft
Conversation
Draft the execution plan for roadmap step 1.2, which fills the empty `thysalion-world` crate with the voxel type registry, the scene document model, load-time validation, and three fixture scenes. The plan resolves the open question ADR 005 forwarded to this step: the scene format stays inside `thysalion-world` rather than becoming a leaf `thysalion-scene` crate, with a recorded reversal trigger. A new ADR 006 will record that and the format's other hard-to-reverse choices. Substantive positions taken, each with its rationale in the decision log: - The document is all-integer (centi-degrees, millimetres, fixed-point), so JSON/MessagePack parity holds by construction and the world crate keeps the float discipline ADR 005 requires of it. - The voxel payload is keyed by chunk with uniform-chunk elision, rather than one global Z-major run stream. A global stream fragments on every raster row of a localized fixture; chunk-keying makes encoding, decoding, hashing, and diffing scale with populated volume instead of declared extent. - MessagePack is written with `to_vec_named`, pinned by a wire-shape assertion rather than by a round-trip test alone. - Loading is two-phase: fail-fast deserialization located by `serde_path_to_error`, then accumulate-everything validation reported through `miette`'s related-diagnostic collection. - `slope_dir` becomes a closed `SlopeDirection` enum, and palette index zero is reserved for air. Research recorded in Surprises: design section 7.3 credits lille's map format as proven prior art, but that format was never implemented in lille. The design document is corrected as part of the plan, and the three lessons actually taken from it are named. The plan signposts the documentation and agent skills a reader needs, and passes `make markdownlint`.
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![codescene-access[bot]](https://avatars.githubusercontent.com/in/3755875?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Reviewer's GuideAdds a comprehensive execution plan document for roadmap step 1.2 that defines the scene format, voxel/world data model, validation strategy, fixtures, and implementation stages for the thysalion-world crate, without introducing any production Rust code. Sequence diagram for two-phase scene loading and validationsequenceDiagram
actor Operator
participant SceneLoader
participant SceneSource
Operator->>SceneLoader: load
SceneLoader->>SceneSource: read
SceneSource-->>SceneLoader: bytes
SceneLoader->>SceneSource: has
SceneSource-->>SceneLoader: bool
SceneLoader-->>Operator: Scene
alt validation_failed
SceneLoader-->>Operator: SceneLoadError
end
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Stress-test the scene-format plan through structural, alternatives, contracts, and failure-mode reviews before any code exists, and fold the confirmed findings back into the plan. Six changes are load-bearing. `VoxelRun` becomes a named struct. `rmp_serde::serialize_tuple_struct` writes a positional array unconditionally, ignoring the struct-map configuration, so the plan's headline mitigation for positional encoding never reached its most numerous wire type. Spike A2 must now measure the named form against the 1 MiB tolerance. The document gains a compatibility policy. The draft cited `#[non_exhaustive]` as enforcing wire additivity, which it cannot do: it is a Rust-source guarantee with no bearing on the encoding, and it is inert inside the defining crate. Source and wire compatibility are now separated, the version rule is a major/minor pair with an accepted range, the serde attribute policy is a constraint, and checked-in golden bytes are the enforcing test. Content hashing is designed in. Design section 12.3 requires save archives to hash scene assets, and the naive fix breaks on this step's own two-encoding premise: a scene's JSON and MessagePack forms hash differently. The hash is defined over the canonical MessagePack encoding of the validated document, which four canonicality rules make well defined. `grid/` becomes a peer of `scene/`. The runtime voxel structure is state-plane data, and four later phases would otherwise import it through a path announcing itself as the file format. The document-to-domain split becomes real, and `SceneSource` loses its `has` method. A derived `Deserialize` on `Palette` was a public constructor reaching every invariant the plan declares enforced, and a boolean existence check cannot distinguish an absent file from an unreadable one. Validation becomes phased and bounded, with a stated trust boundary, enforced resource caps, and iterative prototype resolution. "Report every problem" is not implementable against untrusted input. Two decisions are reversed and two rationales corrected. `miette` is dropped: its spans would point at generated JSON rather than at the authoring layer file, and it would couple every snapshot to an upstream renderer version. The all-integer document is justified by `Eq` across the document tree, the NaN gap in JSON, and design section 10.5 — not by a JSON/MessagePack float divergence, which does not exist.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
A second adversarial review verified the plan's serde, Clippy, schemars, and miette claims against the pinned toolchain rather than from memory. Four defects had survived the first revision. The worked minimal JSON did not deserialize. `ChunkPayload` is an externally-tagged enum, and the example wrote a run array directly beside `at` while never declaring the entry type joining them. The payload is now a newtype-variant enum behind an explicit `payload` field, `ChunkEntryDocument` is declared, and the uniform case is shown. `clippy.toml` had never been read. It sets a cognitive-complexity threshold of 9 against a denied lint, 70 lines per function, four arguments, and four levels of nesting — and the plan mandated a fused per-chunk traversal checking four properties, which breaches the complexity ceiling by construction. The traversal becomes one pass feeding several single-purpose accumulators. The operator tool sat on an unresolvable trilemma: `make lint` lints examples, `clippy::print_stdout` is denied, and the plan forbade new suppressions. Writing through `writeln!` on a `Stdout` handle resolves it without a suppression, and the coverage half is resolved by amending the plan's own promise with one ignore entry. Two writers existed for one format and only one was tested. The staleness check compared Python output against Python output, while Rust's serializer re-emits defaults the generator would omit — different bytes for the same scene from the first commit, invisible to a canonical-bytes test. Fixtures are now committed fully explicit. Also folded in: a palette-reorder hazard guarded by a committed palette manifest; the layered-text authoring format specified with a worked example, having previously been named but not defined; the fixed-point scale for `SimProperties` pinned at Q8.8, since design section 10.5 mandates a width and no scale; a scope tolerance that would otherwise fire on Stage C3 by construction; reference data moved ahead of the rules that validate against it; `Scenario Outline` in place of a `Scenario` carrying an `Examples:` table, which would not parse; port path-safety rules; and `hyperfine` pointed at a built binary rather than `cargo run`. One severity claim is corrected. The previous revision note called the tuple-struct `VoxelRun` a defect; it is not, encoding identically under both `to_vec` and `to_vec_named`. The objection is purely about evolution, and the named struct stands on that ground alone.
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.
Summary
This branch carries the pre-implementation execution plan for roadmap step
1.2, which fills the currently empty
thysalion-worldcrate with the voxel typeregistry, the scene document model, load-time validation, and the fixture scenes
every later spike consumes. No production code lands here: the branch is one
document, and its purpose is to fix the format's hard-to-reverse decisions before
anyone writes against them.
Roadmap task: (1.2) — docs/roadmap.md
Execplan: docs/execplans/1-2-deliver-the-scene-format-and-fixture-scenes.md
The plan authorizes four commits' worth of future work — one per roadmap task
plus a documentation stage — and it resolves the open question
ADR 005 forwarded to this step:
the scene format stays inside
thysalion-world, withbevybehind an optionalfeature as the mechanism that keeps the format engine-free. A new ADR 006 will
capture that and the format's other structural choices when the work lands.
The plan went through two adversarial design reviews before any code was
written, the second of which verified its serde, Clippy, and
schemarsclaimsempirically against the pinned toolchain rather than from memory. It changed
substantially both times.
Review walkthrough
Reviewers with limited time should read the two revision notes, then the
decision log.
for what a contributor can do afterwards and how to observe it. It separates
the two compatibility promises — Rust-source and wire — that the first draft
conflated.
and second
revision notes, which are the shortest route to what review overturned and to
the two claims the plan corrected about its own earlier reasoning.
which carries the positions worth arguing about, including two reversals of
earlier decisions in the same document.
which now reaches past this step: format evolution against phase 8's content
hashes, the mutability question against step 2.1.3, semantic validity as
distinct from structural validity, and a palette reorder silently
reinterpreting every stored index.
where the canonical-encoding rules, the trust boundary, and the
clippy.tomlcomplexity thresholds live. Those are the parts that cannot be relaxed later
without invalidating saves.
for the behavioural specification and the fully worked minimal scene document,
which is the reference every later section assumes.
the contract the implementer works from, and
Signposts,
which lists the documentation and agent skills to load, in order.
Validation
Documentation only, so the Markdown gates apply.
The plan itself is not executable. The gates it commits future work to are in
its
Validation and acceptancesection.Notes
Three departures from the design document, all amended back into
thysalion-design.mdin the plan's documentation stage, and each worth areviewer's disagreement:
payload with uniform-chunk elision. A single global run stream fragments on
every raster row of a spatially localized fixture — roughly 165,000 runs and
2 MB of JSON for a populated fragment inside the wilderness extent — and a
one-building edit rewrites the whole stream.
slope_dir: IVec2becomes a closedSlopeDirectionenum, and itspassable: [bool; 6]becomes a six-field struct.never implemented in lille — it ships a Tiled pipeline instead. The plan
names the three lessons actually taken and improves on each.
The plan also pins a fixed-point scale (Q8.8) that design §10.5 leaves
unspecified, having mandated a width and no scale; §10.5 is amended to match.
The behavioural suite does not reuse the existing Bevy
rstest-bddadapter,because
thysalion-worldhas no Bevy dependency and ADR 005 stages one inlater. A structurally identical adapter over a plain loader context is added
instead, to be merged with the Bevy one at step 1.3.1 — the promotion point the
developers' guide already names.
Two open questions are deliberately left for ADR 006 rather than settled here:
whether
chunk_sizeis a real degree of freedom or should be a schema constant,and whether the decoded
VoxelGridis a transient buffer that step 2.1.1 copiesinto
bevy_voxel_world's storage and discards.Of the six review lenses, scaling and long-term viability were completed by the
author rather than by an independent reviewer after those reviewers were
interrupted. Their conclusions are folded into
Risks,Tolerances, themodule-size budgets, and the test-proportionality note, but they have had less
scrutiny than the rest.