Repository navigation
Conversation
![github-actions[bot]](https://github.githubassets.com/images/gravatars/gravatar-user-420.png){kind=avatar}
Pure code motion off upstream main, no behaviour change, as the base for the typed rewrite.
BREAKING: Geometry is now an internally-tagged enum whose variants carry depth-correct boundaries. A document whose boundaries do not match its declared type no longer deserializes.
…viour The two most intricate rewrites in the depth-typing change had no unit tests. Expected values for the two new appearance tests were produced by running the old implementation (a97bc2d) on the same inputs, so they prove equivalence rather than restate the new code. Includes the asymmetric texture-vertex numbering quirk, which is upstream behaviour and now pinned. Also extend the depth test to reject boundaries one level too shallow and to round-trip every variant's boundaries, and pin first-encounter renumbering at the deepest (5-level) traversal.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
geomprimitives.schema.json types material.values and semantics.values as ["array","null"] at *every* nesting level, not just at the leaf, so a whole null shell or null solid is valid CityJSON. Both ladders carried Option only at the leaf and hard-failed such documents with an untagged-enum error that names neither the path nor the position. Make the intermediate levels Option in MaterialValues and SemanticsValues, and make Semantics.values itself an Option (required member, nullable value) so it is written back as null rather than dropped. Every None serializes as null, never as [] -- finding cityjson#7 -- pinned by test at each level. texture.values is deliberately left strict: the schema types it and each of its intermediate items as a plain "array", with only the innermost items ["integer","null"], so a null surface or shell there is not valid CityJSON. TextureReference.values does become Option, because the per-theme texture object carries no "required" keyword at all. Also drop the deep copy on the happy path of Geometry::from_json_value by deserializing from &Value instead of cloning into from_value. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
{"theme":{"values":null}} is schema-valid, but with a plain Option field it
deserialized to the same value as {"theme":{}} and re-emitted as {}, which
satisfies neither branch of material's oneOf(value|values) and is rejected by
a validator. Same bug class as the Semantics.values fix in e4f88c6.
values becomes Option<Option<MaterialValues>>: the outer level is
present-vs-absent, the inner null-vs-array. serde_json maps a JSON null onto
the *outer* None of a plain double Option, so the field needs a three-line
deserialize_with that defers to the derived Deserialize of the inner type and
wraps it in Some. No visitor is written and serialization stays fully derived.
Also retitle the TextureReference {"values": null} assertion: that shape is
invalid CityJSON, and the test now documents accepting it as a deliberate
leniency that normalizes to {} rather than claiming schema support.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Add byte-for-byte round-trip tests for an Extension-typed CityObject inside a CityJSONFeature and an Extension-typed SemanticsSurface inside a real Geometry -- the shape the noise_extension fixture actually has, which the bare-CityObjectType/SemanticSurfaceType tests never exercised since they don't go through the #[serde(flatten)] path. Also document the match-on-value-not-reference caveat on both const-alias blocks, since matching a const against a &CityObjectType is E0308 and rustc's fix-it (rename the arm to a binding) compiles into a silent catch-all. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ests The two document-level byte-for-byte tests only had "type" in their fixtures, so an empty other/flatten field would pass identically to a correctly-populated one. Add exactly one extra member to each: a genuinely unrecognized "+noiseLevel" (nested object) on the CityObject -- not "attributes", which is CityObject's own named field and would never reach other -- and the spec's own "azimuth" example on the SemanticsSurface. One key each, since other's underlying map has no stable iteration order past a single entry. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Task 4 introduced CjseqError with Json and GeometryDepth variants; this finishes porting develop's 5a8102c by adding the two remaining cases the crate actually needs -- Io (file/stdio operations fail) and Validation (structural rules serde alone can't express, eg an unsupported CityJSON version) -- and wiring their Display/source/From impls. CityJSON::from_str, CityJSONFeature::from_str, and ReferenceSystem::from_url now return the crate's Result instead of serde_json::Error or &'static str directly, and main.rs's hand-rolled MyError enum (a near duplicate of CjseqError) is gone in favour of the library's own type. No thiserror dependency added; the impls stay hand-written to match error.rs's existing style.
Ports develop's 828c7c7. cityobjects.schema.json types CityObjectGroup's children_roles as one string-or-null entry per id in children (§ 2.5), so this is Option<Vec<Option<String>>>, not Option<Vec<String>> as the original fork had it -- the schema explicitly permits an unspecified role to be null rather than an omitted array slot.
Ports develop's 8f7fbaf onto the current base, reusing metadata's GeographicalExtent alias instead of a bespoke array type. cityobjects .schema.json's _AbstractCityObject fixes geographicalExtent's minItems and maxItems to 6 -- a 5- or 7-element extent is not schema-valid CityJSON -- and the field stays optional (it is not in the schema's required list).
Ports develop's 6a844e3. CityJSON, CityJSONFeature, CityObject, Transform, Address, PointOfContact, ReferenceSystem, and Metadata were the public types still missing PartialEq (every public type in this crate is meant to derive Debug, Clone, PartialEq). None of them contain anything that would block PartialEq, so this is a plain derive add, not a restructure. 6917631's Eq on SemanticsSurface/Semantics has no equivalent here: Task 3 already gave SemanticsSurface a HashMap<String, Value> `other` (Eq-capable in principle) but Semantics.values is untagged over Vec<Option<f64-adjacent usize>> types with no floats, so nothing here regresses -- but the fork's Eq was for those exact types and they already carry PartialEq only by design elsewhere in this branch; not touched.
develop's 5eaf8f5 and aff25a1 made SemanticsSurface's fields public and its `other` catch-all Option<serde_json::Value> so a surface with no extra members has nothing to flatten. Task 3 already made the fields public and gave `other` a HashMap<String, Value> instead of Value -- flattening an empty map contributes zero keys, so the same "no spurious members" outcome already holds without an Option wrapper. This pins that behaviour; no production code changed.
sorted_ids and transform_correction are #[serde(skip)] internal state, not part of the document, but the derived PartialEq added in 3f5cb12 compared them anyway. sorted_ids is filled by from_str iterating city_objects (a HashMap, whose iteration order is not guaranteed stable across independently-constructed maps) and can be further reordered by sort_cjfeatures(Random), so two CityJSONs parsed from the identical string could compare unequal. Confirmed empirically before fixing: a 5-top-level-CityObject fixture parsed twice via CityJSON::from_str and compared with the then-derived PartialEq failed on the first run (sorted_ids differed: ["id-a","id-c","id-d","id-b","id-e"] vs ["id-d","id-e","id-b","id-a","id-c"] for the identical input). Replaced the derive with a hand-written impl that compares only the document fields; reran the same fixture 20x under the fix with no failures. A hand-written PartialEq does not touch the "never hand-write" constraint, which is about Serialize/Deserialize. Checked every other type for the same #[serde(skip)]-reaches-PartialEq shape (only skip_serializing_if elsewhere, which does not affect equality); CityJSON was the only offender. Also fills in the missing too-long case in city_object_geographical_extent_rejects_the_wrong_length, whose doc comment already claimed to cover it.
Completes develop's 6917631, previously declined in 3f5cb12 on the belief that serde_json::Value blocks Eq through SemanticsSurface.other: HashMap<String, Value>. Checked rather than assumed: this serde_json version (1.0.151) implements Eq for both Number (src/number.rs:50, sound because a JSON number can never be NaN) and Value (src/value/mod.rs:115), so nothing here actually blocks it. Verified by removing the derives, confirming E0277 on all three types, then restoring them.
Every existing test in this crate is synthetic -- hand-built JSON literals
written from the spec. These are real files, copied in (rather than
referenced by absolute path) so the suite works in a fresh clone, which
this branch needs since it is destined for an upstream pull request.
Chosen for diversity rather than bulk (104 kB total):
noise_extension Extension City Object and semantic surface types, with
their leading `+`
geom_temp geometry templates, GeometryInstance, header appearance
empty_appearance an appearance object whose arrays are empty
small 3DBAG output, with non-core `metadata` members
degenerate_extent, inferable_types, long_strings, single_feature
conformance edge cases from the C++ suite
duplicate_keys kept for reference only; see tests/roundtrip.rs for why
it is not round-tripped
flatcitybuf's delft.city.jsonl (6.6 MB) was deliberately skipped: it adds
nothing over small.city.jsonl, which comes from the same 3DBAG pipeline.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Currently FAILING on tests/data/small.city.jsonl, deliberately: it catches
a real bug, fixed in the next commit.
A reader compared against another reader's output can agree with it on the
wrong answer -- that is precisely how the two bugs motivating this rewrite
stayed invisible. Only a trip back out through the writer can catch a
member that silently vanished, so that is the shape of this test.
Comparison is on parsed Values rather than strings, because several members
are backed by HashMap and have no stable serialization order; a byte
comparison would fail for reasons that are not data loss. The one further
tolerance is the lexical spelling of a number: `metadata.schema.json` types
geographicalExtent's items as {"type": "number"}, JSON has a single number
type, and a fixture's `0` coming back as `0.0` loses nothing. That
tolerance stops short of `a.as_f64() == b.as_f64()`, which would equally
forgive 9007199254740993 -> 9007199254740992.0, and it is pinned by its
own test.
duplicate_keys.city.jsonl is excluded with a comment: a repeated member
name is not preserved by any JSON parser, so both sides of the comparison
are lossy before cjseq sees them.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Round-tripping tests/data/small.city.jsonl dropped two members:
/metadata/fullMetadataUrl: DROPPED ("https://data.3dbag.nl/...")
/metadata/version: DROPPED ("v2024.04.20")
Both are legal. metadata.schema.json's `metadata` object lists its six
properties and then stops -- it declares no `additionalProperties: false`,
in contrast to `transform` and `geometry-templates`, which both do. So
anything further is valid CityJSON, and the spec itself (§ 5) points at the
MetadataExtended Extension as the place to put more of it.
Metadata had no catch-all, so every such member vanished on the way
through. Every 3DBAG dataset carries these two, which makes this silent
data loss on what is probably the most widely distributed CityJSON there
is. Fixed with `#[serde(flatten)] other`, matching what CityJSON and
CityObject already do -- and deliberately NOT extended to CityJSONFeature,
whose schema *does* say `additionalProperties: false`, so its lack of a
catch-all is correct and stays.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Two defects in the same schema object, both found by reading
metadata.schema.json against the code rather than by a fixture -- none of
the round-trip fixtures carries a pointOfContact at all, so these were
invisible to the new test and would have stayed invisible.
1. cjseq rejected the CityJSON spec's own § 5.3 example:
invalid type: string "24", expected i64
Address declared five *required* members, with `thoroughfareNumber`
typed i64. The schema types `address` as a bare {"type": "object"} --
no properties, no required, no additionalProperties -- and the prose
says why: "any properties can be used, to accommodate the different
ways addresses are structured in different countries". Both spec
examples spell the number as a string ("134", "24") and the postcode
as `postcode`, where cjseq demanded `postalCode`. Structure the model
does not actually know is worse than none: it turns valid input into a
parse error. Address is now the free-form object the schema describes.
2. `pointOfContact.organization` was silently dropped. It is named by both
§ 5.3 and the schema's contactDetails.properties, and had no field and
no catch-all. Added, along with `other` for anything further --
contactDetails declares no additionalProperties: false either.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
From the codex review (finding 9 of its first pass): three more objects
whose schemas leave the door open but whose Rust types shut it.
geomprimitives.schema.json declares additionalProperties: false on the
*geometry* object, and then declares no such thing on three objects nested
inside it:
- the `semantics` object (names surfaces, values)
- each per-theme `material` object (names value, values)
- each per-theme `texture` object (names values)
So `{"surfaces":[...],"values":[0],"vendorData":{"x":1}}` is legal
CityJSON and cjseq was dropping `vendorData` on the floor. Same for
`{"value":0,"vendorData":true}` in a material theme.
SemanticsSurface already had an `other` for exactly this reason; its
container did not. Added `#[serde(flatten)] other` to Semantics,
MaterialReference and TextureReference.
The catch-all on MaterialReference sits alongside the `values` double
Option, so there is a test pinning that {"values": null} is still distinct
from {} and from an absent member -- flatten switches serde to its buffered
path and that distinction is exactly what would break silently.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
From the codex review: ReferenceSystem hand-wrote both Serialize and Deserialize, which violates the crate's derived-only rule outright. The rule exists because a hand-written impl is the one place a field can be dropped or reshaped with no type-level trace of it having happened -- the same failure this branch's round-trip test was built to catch. The type is a struct in Rust and a bare string in JSON, which is what the hand-written pair was for. `#[serde(try_from = "String", into = "String")]` expresses that within the derive: serde handles a plain String on the wire and calls ordinary TryFrom/From to convert, and those two conversions are just the existing from_url/to_url. No visitor, no shape logic. Pinned by tests, because a derived Serialize on a four-field struct would otherwise quietly start emitting an object where a string belongs: reference_system_is_a_string_in_json_not_an_object, reference_system_round_trips_inside_metadata, and a_non_crs_reference_system_is_rejected (TryFrom's error must still surface, not fall back to a default). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…rcised
duplicate_keys.city.jsonl was excluded on a rationale I wrote from its
filename without reading it: "a JSON object with a repeated member name; no
JSON parser preserves both". That is false. Parsing all six lines with an
object_pairs_hook that reports repeated names finds none, at any nesting
depth. The file is five structurally identical Building features whose
`grp` *attribute value* repeats -- the conformance case is duplicate
grouping, not duplicate keys. It round-trips cleanly, and now does so in
CI.
The deeper problem is that nothing would have told me. The fixture lists
are hardcoded rather than globbed, which is the right call (a glob passes
vacuously when it matches nothing) but leaves a file able to sit in
tests/data/ unexercised in silence. every_fixture_on_disk_is_accounted_for
closes that: every file on disk must be either round-tripped or listed in
EXCLUDED with a reason, and every listed path must exist. EXCLUDED is
empty and the comment says it should stay that way -- a fixture that
cannot survive a round trip is a finding, not a housekeeping detail.
Verified both directions: dropping an unlisted file into tests/data/ fails
the new test by name, and injecting an unknown top-level member into
duplicate_keys line 2 fails the round trip
("/unknownFeatureMember: dropped on round trip"), proving the fixture is
genuinely exercised rather than merely listed.
Also two comparison-harness fixes from the same review:
- canonical_numbers used 9.007e15 as its lossless boundary, which is
below 2^53. Integral values in the gap canonicalized differently
depending on whether they arrived float- or integer-spelled, reporting
a difference that isn't one. Now spelled 2^53 exactly.
- assert_equivalent passed the round-tripped value as assert_eq!'s left
argument, so failures labelled the output "left" and the input
"right", inverted from how the message reads.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Codex finding A2 had two halves and 92bf4f6 fixed only the first (the hand-written serde). This is the second, which is a rejects-legal defect of exactly the same class as the old Address -- and so the one place my own triage principle went unapplied. metadata.schema.json constrains referenceSystem by a prefix and nothing else: "referenceSystem": { "type": "string", "pattern": "^(http|https)://www.opengis.net/def/crs/" } from_url demanded exactly three `/`-separated elements after `crs/`, so every one of these legal strings was a hard parse error: https://www.opengis.net/def/crs/EPSG/0/4326/extra (codex's example) https://www.opengis.net/def/crs/EPSG/0/7415/ (trailing slash) https://www.opengis.net/def/crs/EPSG/0 https://www.opengis.net/def/crs/ The three-element {authority}/{version}/{code} form is the OGC convention and what every real file uses, but it is not what makes the member valid. ReferenceSystem now stores the path elements verbatim as `segments`, so to_url rebuilds any of the above byte for byte -- a trailing slash is just a final empty element. authority/version/code become accessors returning Option<&str> rather than String fields. Keeping them as fields alongside a verbatim path was the obvious alternative and is a trap: the two can disagree, and then to_url silently contradicts the fields. The prefix itself is still enforced, since that part the schema does constrain: a non-opengis host, a missing /crs/, and a bare `.../def/crs` without the trailing slash are all still rejected. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
`other` is the private `#[serde(flatten)]` catch-all, so a struct literal cannot be written outside this module and there was no other way to construct a `CityObject` in code -- only by deserializing one. `flatcitybuf` builds City Objects field by field while decoding a FlatBuffers feature and had no route at all. The constructor takes only `thetype`, which is the one member with no meaningful empty value; the rest are `pub` and are set by the caller. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The typed rewrite carried the fork's package identity (cjseq2 0.2.0-alpha.x, "typed fork … pending upstream merge"). Upstream publishes cjseq, so the package, version and crate docs say so again. The binary is `cjseq` again too.
Replaces the deprecated actions-rs toolchain action with dtolnay/rust-toolchain, runs clippy over all targets with warnings denied, and builds the wasm feature for wasm32. The clippy fixes this needed include implementing FromStr and Default for CityJSON and CityJSONFeature in place of inherent from_str/new.
add_cjfeature shifted a feature's texture-vertex indices by the feature's own UV count instead of the number of UVs already collected, and update_texture wrote the first use of a UV unshifted and later uses shifted. Every textured feature after the first came back pointing at the wrong UVs (8 of 9 rings on a two-building test). Collecting now maps each UV index to old + the collected count; slicing a document into features still compacts UVs in first-use order. The unit test that pinned the old numbering is replaced by tests of the correct behaviour, plus integration tests over several merged features, out-of-order UV use and a cat/collect round trip. serde_json's float_roundtrip feature is on, so coordinates such as 107072.02818522777 no longer drift by one ulp.
- vertices are [i64; 3], transform scale/translate [f64; 3], vertices-templates Vec<[f64; 3]> and a GeometryInstance's boundaries [usize; 1], so malformed arrays fail to parse instead of panicking later. - CityObjects and the material/texture theme maps are IndexMaps, and serde_json keeps object order, so output follows input order and is deterministic. - get_cjfeature returns Result<Option<_>>: it takes every descendant of a top-level object (not only its children), survives parent/child cycles, and reports a dangling child (DanglingReference) or an out-of-range vertex or palette index (IndexOutOfRange) as an error. - update_transform, update_geographicalextent and centroid (now Option<[f64; 3]>) handle a document with no vertices. - SortingStrategy is AsIs | Random | Lexicographical. AsIs is the input order and the CLI default; Random really shuffles. The unimplemented Morton/Hilbert variants are gone. - remove_duplicate_vertices keys on the vertex itself, not a String, and the appearance-slicing code shared by get_metadata and get_cjfeature is one function.
Objects the CityJSON 2.0 schemas close (additionalProperties: false) reject members they do not name: every geometry object (including a GeometryInstance, which takes no lod), transform, appearance and geometry-templates. Every other object keeps unnamed members verbatim in an `other` field, which is now a serde_json::Map<String, Value> everywhere (CityJSON, CityJSONFeature, CityObject, Metadata, PointOfContact, Address, Semantics, semantic surfaces and theme references). - CityJSONFeature keeps extra root members; they used to be dropped. - CityJSON::new() and CityJSONFeature::new() equal their parsed twins. - Geometry variants carry their own semantics/material/texture fields instead of a flattened GeometryCommon; MultiPoint and MultiLineString have no material or texture, as in the schema. GeometryCommon, common() and common_mut() are replaced by semantics(), material() and texture().
- Lod: one of the twenty values the schema enumerates ("0"-"3",
"0.0"-"3.3"), with level() and refinement(). Every geometry
primitive requires one, as the schema does.
- CityJSON.version is a CityJSONVersion (1.1 or 2.0); any other value
fails to parse, so the CLI's own version check is gone.
- CityObject.attributes is a JSON object map.
- CityJSON.extensions maps a name to an Extension { url, version }.
- A CityObjectType or SemanticSurfaceType that is neither a known name
nor '+' followed by a name is an error, so "Buildng" no longer
passes as an Extension.
- pointOfContact contactType and role are the spec's enums
(ContactType, ContactRole).
- A texture ring is TexturedRing::Untextured ([null]) or
Textured { texture, uvs }; a null texture-vertex index is rejected.
Texture type and image stay optional: appearance.schema.json requires
neither.
VertexIndex, UvIndex (texture vertex), MaterialIndex, TextureIndex, SurfaceIndex and TemplateIndex are serde-transparent newtypes over usize, all implementing an Index trait that knows which array it indexes. Boundaries, GeometryInstance template and boundary, semantic surface parent/children and values, material value/values and texture rings use them, and the renumbering maps in get_cjfeature, add_cjfeature and remove_duplicate_vertices are typed per kind, so mixing up two kinds of index no longer compiles. IndexOutOfRange reports the kind from the type.
Semantics, MaterialReference and TextureReference are generic over the shape of their values, and each Geometry variant names its own: PrimitiveValues for MultiPoint/MultiLineString/MultiSurface/ CompositeSurface, SolidValues for Solid, MultiSolidValues for MultiSolid/CompositeSolid (of SurfaceIndex or MaterialIndex), and texture rings nested as deeply as the boundaries. Values nested to the wrong depth for their geometry no longer parse, an empty values array is no longer ambiguous, and the untagged SemanticsValues, MaterialValues and TextureValues ladders are gone. Indices are walked through one Indices trait over the index types and the containers that nest them. Geometry::semantics/material/texture are replaced by surfaces(); the depth-specific values are on each variant. On a 65 MB, 15,000-feature CityJSONSeq (release build, median of 3) collect goes from 0.78 s to 0.68 s and cat from 0.47 s to 0.43 s, with byte-identical output.
CityJSON::errors()/validate() and CityJSONFeature::errors()/validate() check: - every vertex index against vertices (and, in a geometry template, against vertices-templates); GeometryInstance templates against the templates; - semantic surface values, parents and children against the surfaces; - material, texture and texture-vertex indices against the appearance palettes (no appearance means empty palettes); - that each material theme has exactly one of value/values; - that semantics, material and texture values have one entry per primitive, shell or solid, and a textured ring one UV per vertex; - that every child and parent exists and names the other back; - for a feature, that its id is one of its City Objects and that it carries none of the document-only root members. Errors are structured: IndexOutOfRange, DanglingReference, HierarchyMismatch, ValueCount, MisplacedMember, wrapped in InCityObject/InTemplate to say where. errors() returns them all, validate() the first. Every fixture validates, as does everything collect and cat make from it, except two fixture problems the validator found, recorded in the test with the exact error each must produce: a material index with no materials palette in empty_appearance.city.jsonl, and a header in geom_temp.city.jsonl written by a cat with the bug fixed here. fix: get_metadata wrote the geometry templates unrenumbered after slicing the appearance down to what they use, so template material and texture indices pointed at the wrong entries or past the end. The header now carries the renumbered templates, and get_metadata returns a Result instead of silently dropping the appearance when slicing fails.
CityJSON holds the document and nothing else: the hidden sorted_ids
and transform_correction state is gone, and so is the hand-written
PartialEq. In its place:
- CityJSON::header() (was get_metadata) and
CityJSON::features(order), an iterator of Result<CityJSONFeature>;
CityJSON::feature(id) slices out one. sort_cjfeatures and
get_cjfeature(i) are gone.
- Collector: new(header), add(feature), add_header(header) for a
further sequence whose features are in another transform, and
finish(). It replaces add_cjfeature/add_transform_correction, and
refuses a feature whose id is not one of its City Objects.
cjseq_to_cj wraps it and returns a Result.
- SeqReader<R: BufRead> yields Line::Header / Line::Feature, skips blank
lines and wraps errors in AtLine { line }; SeqWriter<W: Write>
writes them back. Line::from_str parses a feature line once, and
looks at the type only when that fails.
- Transform::new is public, with Default.
The CLI uses them: every line is parsed once, output is buffered,
filter --cotype no longer panics on a feature whose id is missing,
collect refuses features before any header, and a new
'cjseq validate' prints every problem in a CityJSON or CityJSONSeq.
The wasm binding reports collect errors instead of panicking.
On the 65 MB / 15,000-feature sequence (release, median of 3):
collect 0.68 s -> 0.41 s, filter --radius 0.49 s -> 0.25 s, cat
unchanged at 0.44 s; collect output byte-identical.
Doc comments and test comments no longer carry review history (finding numbers, reviewer names, commit hashes, "used to", "pre-rewrite"); each says what the item does, and the schema rule behind it where that is the point. The long CityObjectType/SemanticSurfaceType docs keep the one thing callers trip on, matching the associated consts by value. The const blocks get their own doc comment again.
Collector keeps, for each distinct material and texture already in the document's palettes (header entries included), where it is, keyed on every member with floats compared by value (-0.0 and 0.0 alike). Adding a feature's material or texture is now a lookup instead of a scan over the palette, so collecting is no longer quadratic in the number of distinct entries: 80,000 features with one distinct material each collect in 0.27 s instead of 8.1 s (release, identical output). Appearance::add_material/add_texture are gone.
The CHANGELOG lists the typed model, the unknown-member policy, the split between the document and CityJSONSeq sequencing, validate(), the two data-corrupting bugs fixed, and timings against 0.4.0. The README documents --order, collecting several sequences, the validate subcommand, the library API with an example that runs as a doctest, and the wasm feature flag.
A document using all four cases of spec § 8 (a +census root property, +colour/+area-parcel attributes, a +ThermalSurface semantic surface with its own member, a +NoiseBuilding with a +NoiseBuildingPart child and an extra City Object member) parses, validates, round-trips byte-equal as JSON, and survives cat and collect unchanged; the root property and the extensions declaration travel in the header, and the Extension child goes with its parent's feature.
An Extension City Object type must match (\+)([A-Z])\w+, an Extension semantic surface type (\+)\w+, and an entry of "extensions" must have a version X.Y or X.Y.Z (cityjson.schema.json). A mismatch is a CjseqError::Pattern naming the value and the pattern, located in its City Object or template. Parsing stays lenient (any '+'-prefixed name), so a file with a non-conforming name can still be read and repaired.
tests/data/geom_temp.city.jsonl was cut from the CityJSON Railway example (all 18 of its City Objects, its three geometry templates and its header palette are Railway's) by a cat with the header bug fixed in 0913656: the palette was sliced to Railway's materials 1 and 2, but the templates kept pointing at 1 and 2, one past the end. Its header line is now the fixed cat's header for Railway.city.json, which differs only in those template material indices (now 0 and 1); the four feature lines are unchanged. The validate test no longer needs to treat the file as known-invalid.
- CityJSON::warnings()/CityJSONFeature::warnings() report a core City Object with an Extension City Object as a child (spec § 8.7 rule 3, ExtensionChildOfCoreObject). It is a warning rather than an error because the Noise Extension published with the spec does exactly this (tests/data/noise_extension.city.jsonl validates, with one warning per segment). CityObjectGroup members are exempt. - CityJSONFeature lines keep accepting root members the schema does not name (cityjsonfeature.schema.json 2.0.2 has additionalProperties: true). Collector drops them, as there is nowhere to keep them per feature in a document, and reports each as a DroppedMember in Collector::warnings(). - cjseq validate prints warnings and still exits 0 when there are no errors; cjseq collect prints its warnings to stderr. - CHANGELOG and README updated.
tests/cat_collect.rs slices a document into a CityJSONSeq and collects it back, then compares the two with every vertex resolved to its coordinates on the original grid and every material, texture, texture vertex and template index resolved to what it points at, so vertex and palette renumbering do not count as differences but anything lost or pointing elsewhere does. It runs over both document fixtures, every sequence fixture (collected first), and one document that uses an address with a location, default themes, materials, textures, a geometry template with its own appearance, a CityObjectGroup, an Extension City Object with a child and root, attribute and semantic extensions. That last case fails today (the address location is not renumbered) and is #[ignore]d until the fixes that follow.
A Building, BuildingPart, BuildingUnit, Bridge or BridgePart address may
carry a location, a MultiPoint into the document's vertices (§ 2.3). It
sat untyped in CityObject.other, so cat left its indices pointing into
the whole document's vertices from inside a feature, collect did not
offset them, vertex dedup did not renumber them, and validate() did not
check them.
CityObject.address is now Vec<CityObjectAddress { location, other }>,
and CityObject::geometries()/geometries_mut() yield every geometry that
indexes vertices, address locations included; slicing, collecting,
dedup and validate() all go through them. A location that is not a
MultiPoint is a validation error.
default-theme-material/-texture belong to the document, but header() only kept an appearance when there were geometry templates, so without templates the defaults rode only inside each feature's sliced appearance, and Collector::add never read them back: cat | collect lost them. The header now carries the default themes whenever the document has them; Collector adopts a feature's when the document has none, and drops one that disagrees with the document's with a DroppedMember warning.
- features() roots a feature at every City Object whose parents are only CityObjectGroups (or none), and does not follow a group's children: a group's members are not its parts (§ 2.5), so each member is a feature of its own and a member of two groups is written once instead of being copied into both groups' features. - A City Object that no feature reaches (its parent is missing, or does not list it as a child) used to vanish from cat's output with exit 0. features() now yields a single NotInAnyFeature error naming all of them, and cat checks it before writing anything. - A feature's validate() accepts its root naming groups that live in other features, and a group naming members that do. - CityObject::is_toplevel is gone; number_of_city_objects counts feature roots.
Collector::add_header kept only a later header's transform. Its geometry templates were dropped, so its features' GeometryInstances pointed at the first sequence's templates or past the end, and its extensions, metadata and root members were dropped too. Now a later header's templates are appended to the document's, with their vertices-templates offset and their appearance merged into the document's palettes, and every GeometryInstance in the features that follow is shifted to match. Its extensions are added; one with the same name but a different URL or version, or a different CityJSON version, is a HeaderMismatch error. Metadata or root members that differ from the document's are dropped with a DroppedHeaderMember warning. Appearance merging for headers and features is one function.
Collector::add renumbers a feature's material, texture and texture-vertex indices through maps built from the feature's palettes, and shifts its vertex and template indices. An index past the end of the feature's own data was not in a map, so it was handed the next free position, and a shifted vertex or template index landed in another feature's data: an invalid feature came out of collect looking valid. add() now checks every vertex, material, texture, texture-vertex and template index against the feature's own vertices and palettes and the current sequence's templates before merging, and rejects the feature with IndexOutOfRange inside InCityObject. collect time on the 65 MB sequence is unchanged (0.41-0.43 s), output identical.
Collector::add inserted every City Object of a feature by id, so a second feature carrying an id already collected silently replaced the first one's object. Ids are unique in a document; such a feature is now a DuplicateId error, raised before anything of it is added.
CjseqError, IndexKind, Geometry, GeometryType, KnownCityObjectType, KnownSemanticSurfaceType, CityJSONVersion, SortingStrategy, ContactType, ContactRole, TextureFormat, WrapMode and TextureType are #[non_exhaustive], so a new error variant, CityJSON version, geometry or codelist value is not a breaking change; a compile_fail doctest pins it. Line stays exhaustive: a sequence line is a header or a feature by definition, and every reader matches on it.
- clap, rand and (on wasm32) getrandom are optional, behind a default
`cli` feature that the cjseq binary and the CLI tests require. A
library user adds cjseq with default-features = false and gets only
serde, serde_json and indexmap; the wasm build no longer compiles
clap or needs getrandom's js backend.
- SortingStrategy::Random { seed } shuffles with a built-in SplitMix64
Fisher-Yates, so an order is reproducible from its seed; cjseq cat
--order random takes --seed, and draws one when it is absent.
- CI also lints and tests the library without default features and
builds wasm without the CLI; the npm workflow and README build wasm
the same way.
CityJSON::warnings(), CityJSONFeature::warnings() and Collector::warnings() return Warning (ExtensionChildOfCoreObject, DroppedMember, DroppedHeaderMember), a #[non_exhaustive] enum of its own with Display, so a caller can tell from the type alone that none of them is fatal. The three variants are gone from CjseqError.
1.85 is the oldest Rust the locked dependencies build on (indexmap 2.14 and its hashbrown, clap 4.6); the whole test suite passes on it. A CI job builds and tests with --locked on exactly 1.85. The CHANGELOG covers this round's fixes and API changes.
Undoes the group policy of 074ce96. A CityObjectGroup's members name the group in their parents (§ 2.5), so they are part of the group's feature, as before; a member of two groups is in both groups' features. The NotInAnyFeature check from the same commit stays: cat still never drops a City Object silently. To collect such sequences, Collector::add now takes a City Object it has already collected when the new copy is identical, and still reports DuplicateId when it differs (nothing of that feature's City Objects or vertices is added then). Identical copies compare equal because vertices and texture vertices are now merged as features are added (finish() did this for vertices already, so collect output is byte-identical on the 65 MB sequence, 0.38 s). A feature's validate() accepts a City Object with a group parent in the feature naming its other groups, which live in other features.
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.
No description provided.