Repository navigation
Name a feature by a string - #362
Merged
Merged
Conversation
#338's query hands a host a feature's id as its source gave it, and a GeoJSON source may give a string -- `a_string_id_is_reported_as_it_was_given` pins that. `tessella_set_feature_state` took a `uint64_t`, so the id a query produced was one nothing could consume. A host taps a POI, gets `"ribbon"` back, and has no way to mark it. ```rust pub enum FeatureKey { Number(u64), Text(String), } ``` A number and a string are different features, kept apart rather than flattened the way mbgl's string-only key does -- there, feature 7 and feature "7" collide. `FeatureKey::of` refuses a fractional or negative number instead of casting it: nothing in a tile can have one, and rounding a GeoJSON `1.5` onto feature `1` would mark the wrong one. In C the id is a pair, and a null text pointer means the number names the feature -- every MVT case, and the ordinary call stays short: ```c uint64_t feature_id, const uint8_t* feature_id_text, /* NULL: use the number */ size_t feature_id_text_len, ``` This breaks the signature. It costs nothing today: every caller was a test in this repository, with no `web/map.js` binding and neither consumer calling it. ## The half that would have made it hollow `Painted::id` was still `Option<u64>` through `numeric_id`. Marking a string-id feature would have stored the state and the re-paint would still not have found it, because the *recorded* id had already dropped the string -- a fix that passes its own entry point and does nothing to a pixel. It is a `FeatureKey` too, and `numeric_id` is gone. ## A mutation with nowhere to be caught Ignoring `feature_id_text` entirely -- always keying on the `uint64_t` -- passed every test in `tests/feature_state.rs`. The content stamp carries the host's state *revision*, so any mark re-announces whether or not it named something real, and the C surface has no way to read a feature's state back. An integration test can see `Status::Ok` and nothing more. So the guard is a unit test inside the crate, which can reach `MapState` and ask the map what it stored: a text id stores as `Text`, a null text pointer falls back to `Number`. That is the only place the distinction is visible without adding a readback to the ABI for a test's sake. ## One assertion of mine that was wrong I first wrote that marking the number 7 must *not* re-announce a feature named `"ribbon"`. It read `adds: 8`. The code is right and the assertion was not: the stamp is revision-based by design, from #354. That test now pins the revision behavior instead, so it is not misread as the feature having been found, and says where the byte-level discrimination lives -- `tessella-layout`'s `a_string_id_is_named_by_the_lookup`, which compares a buffer marked by `Number(7)` against one marked by `Text("motorway-7")` and asserts the first leaves it alone. | mutation | caught by | | --- | --- | | a string id dropped again, as `numeric_id` did | the lookup test | | the text id ignored at the C boundary | the in-crate boundary test | | a fractional id truncated onto its neighbor | the key-construction test | | a negative id wrapping the cast | the same | The first three passed before the tests above existed. ## The sweep `hill_p` z14 p0 and z11 p0 both read **0**, down from 1 and 14 on the previous run: those are the two rows `sweep.sh`'s header documents as unstable, and 0 is what each reads when the scene is run alone. `terrain_flat_p` z14 p60 held at 28, inside its documented 26-29. Nothing moved away from a documented value, and no parity scene uses feature state. Closes #361 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.
#338's query hands a host a feature's id as its source gave it, and a GeoJSON source may give a
string --
a_string_id_is_reported_as_it_was_givenpins that.tessella_set_feature_statetook auint64_t, so the id a query produced was one nothing could consume. A host taps a POI, gets"ribbon"back, and has no way to mark it.A number and a string are different features, kept apart rather than flattened the way mbgl's
string-only key does -- there, feature 7 and feature "7" collide.
FeatureKey::ofrefuses afractional or negative number instead of casting it: nothing in a tile can have one, and rounding a
GeoJSON
1.5onto feature1would mark the wrong one.In C the id is a pair, and a null text pointer means the number names the feature -- every MVT case,
and the ordinary call stays short:
This breaks the signature. It costs nothing today: every caller was a test in this repository, with
no
web/map.jsbinding and neither consumer calling it.The half that would have made it hollow
Painted::idwas stillOption<u64>throughnumeric_id. Marking a string-id feature would havestored the state and the re-paint would still not have found it, because the recorded id had
already dropped the string -- a fix that passes its own entry point and does nothing to a pixel. It
is a
FeatureKeytoo, andnumeric_idis gone.A mutation with nowhere to be caught
Ignoring
feature_id_textentirely -- always keying on theuint64_t-- passed every test intests/feature_state.rs. The content stamp carries the host's state revision, so any markre-announces whether or not it named something real, and the C surface has no way to read a feature's
state back. An integration test can see
Status::Okand nothing more.So the guard is a unit test inside the crate, which can reach
MapStateand ask the map what itstored: a text id stores as
Text, a null text pointer falls back toNumber. That is the only placethe distinction is visible without adding a readback to the ABI for a test's sake.
One assertion of mine that was wrong
I first wrote that marking the number 7 must not re-announce a feature named
"ribbon". It readadds: 8. The code is right and the assertion was not: the stamp is revision-based by design, from#354. That test now pins the revision behavior instead, so it is not misread as the feature having
been found, and says where the byte-level discrimination lives --
tessella-layout'sa_string_id_is_named_by_the_lookup, which compares a buffer marked byNumber(7)against one marked byText("motorway-7")and asserts the first leaves it alone.numeric_iddidThe first three passed before the tests above existed.
The sweep
hill_pz14 p0 and z11 p0 both read 0, down from 1 and 14 on the previous run: those are the tworows
sweep.sh's header documents as unstable, and 0 is what each reads when the scene is run alone.terrain_flat_pz14 p60 held at 28, inside its documented 26-29. Nothing moved away from a documentedvalue, and no parity scene uses feature state.
Closes #361
Gate: clippy pinned and stable, rustdoc on default features and all features, the wasm32 no_std lane
over all six crates, the wasm32 module built and its ABI checked, 2332 tests, five JS tests,
benches built and run, fmt last then clippy again, and the pixel gate with probes rebuilt -- quad
22 / 22 / 22 / 32withimage_stable 1.