Repository navigation
Evaluate feature-state in a paint property - #351
Merged
Merged
Conversation
`"feature-state"` was in the generated operator registry and nothing
parsed it, and what that cost is worse than the issue says: the operator
reached the parser's unknown arm, so a paint property reading it was
refused and the layer was dropped. Measured both ways --
before: REFUSED: `fill-color`: unknown expression operator `feature-state`
after: RESOLVES
-- so a style highlighting a feature drew no layer at all rather than
every feature in its default state.
The operator is one argument, reads the state of the feature being
evaluated, and answers null for a key the feature has no state for, as an
absent property does: a style writes `["case", ["feature-state", "hover"],
…, …]` and expects the default arm, and erroring would make every
unhighlighted feature a failed evaluation.
State hangs off the `Feature` trait beside `property`, with a default
answering None. That is the shape the operator has -- state belongs to
the feature being evaluated, as its tags do -- and it keeps every
implementation written before this one compiling and answering what a
feature with no state means.
`Dependency::STATE` is a bit of its own, always joined with FEATURE. The
feature bit is what makes the property a per-vertex attribute rather than
a uniform, and the state bit is what a caller deciding how much to redo
for a change reads. mbgl keeps the same distinction for the same reason.
The specification allows the operator in a paint property only, and both
halves of that are now enforced where they are compiled: a filter is
refused by `parse_filter`, and a layout property by `resolve_layout`. The
second has a hole worth naming -- `layout_specs` has no table for a
symbol layer, so `resolve_layout` answers an empty map for one and the
refusal never sees it. `layout_value` is what reads those properties and
has no error channel, so it answers None there, which is the caller's own
default. Both are tested, including the symbol case.
Still open: the store and the C setter. Nothing supplies state yet, so
the operator answers null for every feature -- which is the default arm,
and is what the layer now draws instead of not drawing.
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 operator half of #337. The store and the C setter are the other half and are not here; see the
end.
A correction to the issue, measured rather than reasoned
The issue says a style using
["feature-state", …]"draws every feature in its default state". Itdoes not.
"feature-state"was in the generated registry -- which is what makeslooks_like_expressiontreat the array as a call rather than as data -- and the parser had no armfor it, so it reached the unknown-operator path. A paint property reading it failed to resolve, and a
layer whose paint will not resolve is rejected, so the layer drew nothing at all.
Both halves of the same fill layer, through
resolve_paint:So the first thing this fixes is a style that was being dropped.
The operator
One argument, reads the state of the feature being evaluated, and answers
nullfor a key thefeature has no state for -- as an absent property does, and for the same reason: a style writes
["case", ["feature-state", "hover"], …, …]and expects the default arm, so erroring would makeevery unhighlighted feature a failed evaluation.
State hangs off the
Featuretrait besideproperty, with a default answeringNone. That isthe shape the operator has -- state belongs to the feature being evaluated, as its tags do -- and the
default keeps every implementation written before this one compiling while answering exactly what a
feature with no state means. The alternative was a sixth parameter on an
evaluatechain that isalready five deep.
Dependency::STATEis a bit of its own, always joined withFEATUREand never alone. Thefeature bit is what makes the property a per-vertex attribute rather than a uniform, so that
behavior is unchanged; the state bit is what a caller deciding how much to redo for a change reads.
mbgl keeps the same distinction for the same reason, as
Dependency::FeatureState.Paint only, enforced where each half is compiled
The specification allows the operator in a paint property and nowhere else, because a filter decides
which features exist and a layout property decides their geometry -- each answered once, when the
tile is cut. A filter over state would report whatever the state was then and never change, which
reads as a highlight that works until the first pan.
parse_filterrefuses it, naming the rule.resolve_layoutrefuses it.That second one has a hole, which is named rather than papered over:
layout_specshas no tablefor a symbol layer, so
resolve_layoutanswers an empty map for one and the refusal never seestext-field. The function that reads those properties islayout_value, one property at a time,and it has no error channel -- its contract is already "no value, so the caller's own default
applies". So that is what it does for state, and the test says so for
text-fieldwhile checkingthat
text-sizebeside it is unaffected.Mutations
property()instead ofstate()layout_value's check removedWhat is left of #337
The store and the C setter: per-feature state keyed by source, source layer and feature id, and
tessella_set_feature_state. Nothing supplies state yet, so the operator answers null for everyfeature -- which is the default arm, and is what the layer now draws instead of not drawing.
The issue's "re-evaluated on change without re-laying-out the tile" is a third piece and deserves to
be scoped on its own: a bucket is keyed by the style revision precisely because a changed filter
admits different features, and deciding per bucket whether a state change could have affected it
needs a per-layer input hash. The
STATEbit added here is what such a decision would read.Gate: clippy pinned and stable, rustdoc on default features and all features, the wasm32 check, the
no_std lane, 2268 tests, fmt last, and the pixel gate with probes rebuilt -- every sweep row at its
documented value (the two unstable ones settled back to 0 and 29 this run), zero holes, quad
22 / 22 / 22 / 32withimage_stable 1.