Repository navigation
Record what each feature left in a bucket - #355
Merged
Merged
Conversation
The prerequisite for #338's rendered-feature query, and the part that has to happen during layout: the build job takes a `&mvt::Tile` and returns `Vec<LayerBucket>`, so the decoded tile and the response body are both gone by the time anyone could ask. mbgl answers the same query by keeping its `GeometryTileData` alive and reading the feature back out; there is nothing here to read back. A bucket's vertices carry no feature boundary at all -- a `FillBucket` is one flat `Vec<Position>` -- so `LayerBucket` gains `features`: per feature, its id, its geometry type, its properties, and the range of vertices it filled. The range is the point. A bucket's vertices are already extruded -- a line's quads carry its width, a circle's corners its radius -- so a range into them is closer to what was drawn than mbgl's query, which re-grows the source geometry by the paint at query time. `PaintBinder`'s index from #353 is not this record. It is gated on the paint reading `feature-state`, and a layer whose paint is wholly uniform has `stride == 0`, so there is no per-vertex buffer to hang a range off in the first place. Measured on one fill layer over two polygons, varying only the paint: uniform records 0 of 2, data-driven records 0 of 2 with the buffer present, state-reading records 2. Most layers in a real style are the first row. The properties keep whichever shape the source had. An MVT feature's keys are `Arc<str>` and its string values `mvt::Value::String(Arc<str>)`, pointing into one per-layer table precisely so a value repeated across ten thousand features is stored once; a GeoJSON feature's are a `BTreeMap<String, Value>` it owns, and they nest, which an `mvt::Value` cannot hold. Flattening both into the richer type copies every MVT key per feature and throws the table away; flattening into the cheaper one drops GeoJSON's nested properties. So `Tags` is an enum of the two and the accessors are what they share. Weighed on `protomaps-berlin-14-8802-5373.mvt` with every source layer named: 905 records over 2,971 property pairs, about 80 KiB of record and 119 KiB of tag against 89,859 bytes of tile. Writing it costs about 2%: on `expression_cost`'s z10 streets tile, 377.32 us against 370.48 us of data-driven build and 333.95 us against 326.92 us of constant, measured against the same binary with `Recording::add` returning immediately, for 127 allocations and 38 KiB. The tag half can still go -- a record could hold an `Arc<mvt::Layer>` and its own `Range<u32>` into that layer's tables, which is what the decoder does internally -- but that is a signature change through `boot` rather than an addition, so it is not done here. Tests, and what each one is for: - a uniform-paint layer records, and the binder's index is still empty, which says the two are different mechanisms rather than one measuring the other; - a data-driven layer records with the buffer present, the control that tells #353's state gate from an absent buffer; - each record's range holds *that* feature's vertices, checked against the feature's own tile-space bounds rather than another record's -- a range attributed to its neighbor is a host told it tapped the park when it tapped the wood, and nothing about the map looks wrong; - the ranges tile the buffer with no gap or overlap, which fails where the bounds check passes: a tracker that never advanced leaves every range starting at zero, and the first record still looks right; - a feature clipped away is not recorded. Narrower than it reads: a filtered feature never reaches the recording, and the circle arm skips a non-point before it. What does arrive is a line whose clip returned nothing, and without the guard the fixture records it twice, once per world copy, each `4..4`. An empty range is worse than a missing one -- it has no vertices to test a box against; - MVT records share their layer's tables, by `Arc::ptr_eq` across two features. Equal strings pass a `==` either way, so the pointer is the only thing that tells a share from a copy. Mutations, all caught: the empty-range guard removed; the tracker never advancing; the id narrowed through `numeric_id` as state narrows it; the tags widened into owned strings; the geometry type always `Unknown`; the fill arms recording nothing. The first of those passed on the first attempt -- the test used the circle arm, which `continue`s before the recording, so the guard was never reached. The line arm and a far-away feature are what reach it. Signed-off-by: Joel Winarske <joel.winarske@linux.com>
This was referenced Oct 6, 2026
jwinarske
added a commit
that referenced
this pull request
Oct 6, 2026
The second of #338's slices. #355 and #356 wrote down what each feature left in a bucket; this says which of those records a tap or a box lands on, in one tile's own units. No map-level entry point yet -- that is the next slice, which has the view transform and the cover. ## The paint has to be put back #355 said a bucket's vertices are already extruded, so a range into them was closer to what was drawn than mbgl's query is. That was wrong for every family but fill, and it is corrected on the issue. `line.rs` writes `pos_normal: [p[0] * 2 | bit(round), p[1] * 2 | bit(up)]` -- the centerline, doubled, with cap and side flags in the low bits -- and the extrusion arrives in the vertex shader from `line-width`. `circle.rs` writes the center doubled plus a 0-or-1 corner bit, so all four corners of a circle's quad sit within one tile unit of each other whatever `circle-radius` says. So this does what `queryIntersectsFeature` does: grows the geometry by the paint. Evaluated against the record, not read out of the binder -- the record carries the feature's properties and id, which is what a data-driven expression reads, and it gives the same number without this having to know each slot's byte encoding. Per family: a fill and an extrusion are triangles against the region; a line is its centerline within `line-width / 2`, or `line-gap-width / 2 + line-width` where a gap is set, which is mbgl's `getLineWidth` and not half the width; a circle is its center within `circle-radius` plus `circle-stroke-width`; a heatmap the same by `heatmap-radius`. A paint property is in screen pixels and a region is in tile units, so the caller passes the scale between them -- `pixelsToTileUnits`. ## A line's shape comes out of the index buffer Its vertices do not say which centerline points are joined. Two per point in emission order, and a feature clipped into pieces leaves runs of them with nothing between, so connecting consecutive points invents a segment from the end of one piece to the start of the next. In the fixture here that phantom segment is four thousand units long, and a tap in the middle of it reports a road in open water. `segments` cannot break it either: `LineBucket::add_geometry` starts a new one only at the 64k vertex cap, so one segment spans many features and many pieces. The triangles can. Two centerline points are joined if and only if some triangle mentions a vertex of each, because that triangle is the quad between them, and no triangle spans a gap. The triangles are degenerate in position -- every vertex is on the centerline -- but their topology is the line's shape, and that is what is read. ## Two bugs these tests caught `real` shifted a fill's vertices as well. A fill is the one family that writes the tile coordinate itself, so the shift halved every polygon and a tap inside a square found nothing. Split into `real` and `plain`, with the reason written down: at a glance it reads as a projection bug. And the first version of the two-pieces test asserted the wrong record. The fixture produces three, one per world copy, and only the middle one carries both pieces -- the eastern copy holds the western piece off-tile at x 10012 and the western copy the eastern piece at x -2048. The middle one is the record that holds the gap, so it is the one the test needs, and it now says so. ## Mutations | mutation | caught by | | --- | --- | | the width ignored, every line one unit wide | three tests | | the gap ignored, always half the width | the gap test | | the circle stroke ignored | the radius test | | a fill vertex shifted like a packed one | two tests | | `units_per_pixel` dropped from the line arm | the line scale test | | line adjacency taken from consecutive vertices | the two-pieces test | Two of those passed first time round. The scale one: the only assertion about it was over a circle, so the line arm's conversion was unguarded, and there is now one per arm. And my first adjacency mutation did not implement the thing it was meant to -- it allowed a degenerate pair rather than replacing the index walk -- so it changed nothing and proved nothing. Rewritten to actually connect consecutive vertices, it fails the two-pieces test and nothing else. ## And a flake in two FFI tests, which this PR exposed CI's stable canary failed on `tessella-ffi`'s `feature_state.rs`, both tests reporting that nothing was drawn. Those tests are #354's and this branch does not touch them; they passed on #354, #355 and #356. What changed is the load -- a new test file more in the workspace run. The cause is mine from #354. `settle` stopped after twelve consecutive quiet ticks whatever had arrived, and twelve ticks is a hundred and twenty milliseconds. A cold map is quiet for as long as its first tiles take, so on a loaded runner the loop returned `adds: 0` and the assertion read "the first frames drew nothing" -- which is the message a real regression produces. Reproduced here, with CI's two messages exactly, by setting the window to one tick. `restyle.rs` from #349 has the same loop, on eight call sites. The first fix was wrong and measuring it is what said so. Counting quiet only after any geometry or view record still fails, because a restyle emits the old revision's removes and releases *before* the new document's adds: a window closing between the two returns `adds: 0, removes: 8` and fails the same way. So the condition belongs to the caller. `settle_until(seconds, wanted)` waits for what the phase is about to assert -- `adds > 0`, or `adds > 0 && removes > 0`, or `removes > 0 || releases > 0` -- and a phase whose claim is silence passes one that is never true and waits out a short deadline, which is the honest answer for it. Proven both ways. With the window cut to one tick, the shape that reproduced CI, all six tests pass. With the caller's condition thrown away, they fail with CI's messages. The verdict no longer depends on how long the window is. ## The sweep's one moving row `terrain_flat_p` z14 p60 read 29, then 28, then 29 across three sweeps while the code only grew. That row is documented in `sweep.sh`'s own header as having read 26 through 29, and run alone it reads 29 three times out of three -- the other value appears only under a full sweep, where the reconciliation can land after the frame the probe settled on. A number that moves both ways under monotonic code is the scene, not the change. It also cannot be this change. The only edit to a tracked file is one line of `lib.rs` adding the module, nothing in the render path calls into it, and the rest is two new files. Signed-off-by: Joel Winarske <joel.winarske@linux.com>
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.
The prerequisite for #338's rendered-feature query, and the first of four slices. It adds a record
and reads nothing back yet.
Why the record has to be written during layout
mbgl answers
queryRenderedFeaturesby keeping the tile'sGeometryTileDataalive and reading thefeature back out of it on demand. Nothing here can: the build job takes a
&mvt::Tile, returnsVec<LayerBucket>, and drops both the decoded tile andresponse.bodyon the way out.And a bucket's vertices carry no feature boundary -- a
FillBucketis one flatVec<Position>. SoLayerBucketgainsfeatures: per feature, its id, geometry type, properties, and the range ofvertices it filled.
The range is the interesting half. A bucket's vertices are already extruded -- a line's quads
carry its width, a circle's corners its radius -- so a range into them points at geometry closer to
what was actually drawn than mbgl's own query is, which re-grows the source geometry by the paint at
query time.
#353's index is not this record
I said on #337 that it was. It is not, and the premise correction is on the issue with the numbers.
PaintBinder::indexesrecords a feature only when a paint slot readsfeature-stateand nostate-reading slot reads geometry. One fill layer over two polygons, varying only the paint:
fill-color"#ff0000"["match", ["get","kind"], …]["case", ["boolean",["feature-state","hover"],false], …]The middle row is the control that matters: the buffer is there and the index is still empty, so the
emptiness is the state gate rather than an absent buffer. The top row cannot be reached at all --
uniform paint means
stride == 0, sopushreturns before counting and there is no buffer to hang arange off. Most layers in a real style are that row.
Two property shapes, and no copy in either
An MVT feature's keys are
Arc<str>and its string valuesmvt::Value::String(Arc<str>), pointinginto one per-layer table precisely so a value repeated across ten thousand features is stored once --
mvt::Tile::decode's own doc says it was measured copying them and does not. A GeoJSON feature'sproperties are a
BTreeMap<String, Value>it owns, and they nest, which anmvt::Valuecannot hold.Flattening both into the richer type copies every MVT key and string per feature and throws the table
away; flattening into the cheaper one silently drops GeoJSON's nested properties. So
Tagsis an enumof the two, and
get/iter/lenare what they share.And the MVT arm copies nothing at all. A layer's
propertiesis already one flat table of everyproperty of every feature, which its
Featureindexes with aRange<u32>-- so a record that copiedits own slice out would be copying a table that exists. It holds the table and the range.
mvt::Layer::propertiesbecomes anArc, appended throughArc::make_mut, which is a move while itis uniquely owned, and it is for the whole of decoding.
Not
Arc<mvt::Layer>, which is where this started. A layer owns its geometry too, and on thistile that is 164 KiB of
points, 36 KiB offeaturesand 13 KiB ofendsbeside the 122 KiB oftable -- 336 KiB in all. Holding the layer to save the table would cost 214 KiB more than it saves.
The table alone is the part worth sharing, so that is what
Tags::Mvtholds.What it costs
Weighed on
protomaps-berlin-14-8802-5373.mvt, 89,859 bytes of tile, two ways -- because theanswer depends on the style, and my first number used the shape that flatters a copy:
linelayers overroadsThe second row is the shape a real style has: a source layer is named by many style layers -- a dozen
road layers over
roadsis ordinary -- and each becomes its own bucket. A copy per feature pays forthe table once per bucket, so its cost grows with the layer count; sharing it does not. That is the
2.3x.
The first row is the price, and I want to be straight that it is real. One table per source layer
keeps the pairs of features the style filtered out as well, where a copy keeps only what was kept.
Two things would take it back, neither in this PR:
Queryableis 88 bytes, of which 16 are recoverable by narrowingidoff aValue(32 bytes forwhat is a
u64or a short string) andgeometry_typeoff a&'static str(16 bytes for afour-way tag);
style.layers, so it knows how many name each source layer -- it couldcopy for the ones named once and share for the rest.
Writing it is cheap either way.
expression_cost's z10 streets tile, against the same binary withRecording::addreturning immediately:What the tests assert
The central one is not that records exist. It is that each record's range holds that feature's
vertices, checked against the feature's own tile-space bounds rather than against another record:
different mechanisms rather than this test measuring the old one;
tapped the park when it tapped the wood, and nothing about the map looks wrong;
tracker that never advanced leaves every range starting at zero, and the first record still
looks right;
identity, and by a strong count of exactly the records plus the layer, so it is one allocation
rather than 932. Two copies of a table compare equal under
==, so the pointer is the assertion;layer across all 240 features and 1,600-odd pairs. That layer has two adjacent features carrying
the same id with different tags, which is what makes a slice read from the wrong start
detectable at all.
numeric_id, as state narrows itUnknownThree of those passed on my first attempt, and they are why the last two tests exist.
The empty-range one: the test used the circle arm, and that arm
continues on a non-point beforeit reaches the recording, so the guard was never executed. What reaches it is a line whose clip
returned nothing -- and without the guard that fixture records the far line twice, once per world
copy, each
4..4. An empty range is worse than a missing record: it has no vertices to test a querybox against.
The two range mutations walked through the entire suite. The only MVT property assertions were "some
record has a
kind" and a count compared against the range it came from -- both true of any range atall, including the whole table. A record reporting its neighbor's properties is exactly the defect
the vertex test guards against, and nothing was guarding the other half of the record.
Scope
Eight build arms in two builders (fill and extrusion, line, circle, heatmap, each for GeoJSON and
MVT). Symbols are deliberately absent: they need placement's answer so a hidden label is not
returned, and a
Candidatecarries no feature identity today, so that is slice 3.Scope note: this reaches into
tessella-sourceformvt::Layer::propertiesand the two accessorsbeside it (
property_table,property_range), which is the only way a record can point into thetable instead of copying it.
Gate: clippy pinned and stable, rustdoc on default features and all features, the wasm32 no_std
lane over all six crates, the full test suite, benches built and run, fmt last then clippy again,
and the pixel gate with probes rebuilt.
Toward #338