From 5d665bd47409170a2dccf083652ea330818b99d3 Mon Sep 17 00:00:00 2001 From: Joel Winarske Date: Tue, 6 Oct 2026 07:59:22 -0700 Subject: [PATCH] Evaluate feature-state in a paint property MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `"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 --- .../tessella-style/src/expression/evaluate.rs | 21 ++ crates/tessella-style/src/expression/mod.rs | 61 +++- crates/tessella-style/src/expression/parse.rs | 6 + crates/tessella-style/src/property.rs | 50 +++- crates/tessella-style/tests/feature_state.rs | 266 ++++++++++++++++++ 5 files changed, 396 insertions(+), 8 deletions(-) create mode 100644 crates/tessella-style/tests/feature_state.rs diff --git a/crates/tessella-style/src/expression/evaluate.rs b/crates/tessella-style/src/expression/evaluate.rs index e407ea07..002c9156 100644 --- a/crates/tessella-style/src/expression/evaluate.rs +++ b/crates/tessella-style/src/expression/evaluate.rs @@ -77,6 +77,20 @@ pub trait Feature { fn properties(&self) -> Value { Value::Null } + + /// Per-feature state a host set, by key, or `None` for a feature with none. + /// + /// Not a property: it is not in the tile and not in the style, and it changes while the tile + /// it belongs to stays as it is -- a hover, a selection, a route segment marked. The default + /// answers `None` for every key, which is what a feature with no state means and what every + /// implementation that predates the operator keeps answering. + /// + /// Keyed by the caller however it likes. The specification keys state by source, source layer + /// and feature id, and a feature here already knows which of those it is. + fn state(&self, key: &str) -> Option { + let _ = key; + None + } } /// A feature's coordinates, as the geometry operators read them. @@ -783,6 +797,13 @@ pub(super) fn evaluate(expr: &Expr, context: &Context<'_>) -> Result Ok(context.feature()?.property(&key).unwrap_or(Value::Null)), } } + Expr::FeatureState { key } => { + let key = expect_str(key, context)?; + // Absent state is null, as an absent property is, and for the same reason: a style + // reads it through `case` or `coalesce` against a default. Erroring instead would + // make every unhighlighted feature a failed evaluation. + Ok(context.feature()?.state(&key).unwrap_or(Value::Null)) + } Expr::Has { key, object } => { let key = expect_str(key, context)?; match object { diff --git a/crates/tessella-style/src/expression/mod.rs b/crates/tessella-style/src/expression/mod.rs index 3481aa25..66731383 100644 --- a/crates/tessella-style/src/expression/mod.rs +++ b/crates/tessella-style/src/expression/mod.rs @@ -74,6 +74,16 @@ impl Dependency { /// Re-evaluated per frame. Never cached across a zoom interval, because it changes inside /// one. pub const CAMERA: Self = Self(1 << 2); + /// Per-feature state a host set, which is not in the tile and not in the style. + /// + /// Always joined with [`Self::FEATURE`], because a state lookup has to know *which* feature, + /// and never on its own. The bit is separate anyway, because the two change on different + /// occasions and at different costs: a feature's properties arrive with the tile and are + /// fixed for its life, while its state is set by a host mid-frame -- a hover, a selection, a + /// route segment marked -- and what has to be redone for a change is less than a relayout. + /// + /// mbgl keeps the same distinction for the same reason, as `Dependency::FeatureState`. + pub const STATE: Self = Self(1 << 3); /// The least dependency covering both. #[must_use] @@ -110,6 +120,18 @@ impl Dependency { pub const fn needs_camera(self) -> bool { self.0 & Self::CAMERA.0 != 0 } + + /// True when per-feature state is involved. + /// + /// What a caller does with this is refuse or re-evaluate. The specification allows + /// `["feature-state", …]` in a *paint* property only -- a filter decides which features exist + /// at all and a layout property decides their geometry, and neither can depend on something + /// a host changes without the tile being rebuilt -- so a filter or a layout property reading + /// this is refused where it is compiled. + #[must_use] + pub const fn needs_state(self) -> bool { + self.0 & Self::STATE.0 != 0 + } } /// A `["collator", …]`, whose members are themselves expressions. @@ -841,6 +863,18 @@ pub enum Expr { /// The object tested, or `None` to test the feature. object: Option>, }, + /// Per-feature state a host set: `["feature-state", key]`. + /// + /// Not a property and not in the style. A host sets it to mark a feature -- hovered, + /// selected, part of a route -- and a paint property reads it, which is what lets the + /// highlight change without the tile being rebuilt from its bytes. + /// + /// Absent state is `null`, as an absent property is: a style reads it through `case` or + /// `coalesce` against a default, which is the idiom the specification's own examples use. + FeatureState { + /// State key. + key: Box, + }, /// The feature's geometry type: `Point`, `LineString` or `Polygon`. GeometryType, /// The feature's id. @@ -1242,14 +1276,29 @@ impl Expression { /// /// As [`Expression::parse`]. pub fn parse_filter(value: &Value) -> Result { - Self::parse_rooted( + let parsed = Self::parse_rooted( value, &PropertySpec { default: None, expected: None, }, false, - ) + )?; + // A filter decides which features exist at all, and what the index built from it holds. + // State is set by a host between frames, so a filter reading it would say that a hover + // changes which features are in the tile -- which is not a thing the tile can answer, and + // not a thing this would do: the filter runs once at build and the answer would simply be + // stale. The specification allows the operator in a paint property only, and mbgl refuses + // it here too. + if parsed.dependency().needs_state() { + return Err(ParseError::Malformed { + operator: "feature-state".into(), + detail: "a filter cannot read feature state; the specification allows it in a \ + paint property only" + .into(), + }); + } + Ok(parsed) } fn parse_rooted( @@ -2102,6 +2151,7 @@ fn children(expr: &Expr) -> Vec<&Expr> { out.extend(object.as_deref()); out } + Expr::FeatureState { key } => alloc::vec![&**key], Expr::Compare { lhs, rhs, .. } => alloc::vec![&**lhs, &**rhs], #[cfg(feature = "collator")] Expr::CompareWith { @@ -2291,6 +2341,13 @@ fn classify(expr: &Expr) -> Dependency { Some(object) => classify(key).join(classify(object)), None => Dependency::FEATURE.join(classify(key)), }, + // Both bits. The feature, because a state lookup has to know whose state; and the state, + // because what has to be redone when it changes is not what has to be redone when a + // tile's features change. A caller that only looked at `needs_feature` still binds this + // as a per-vertex attribute, which is correct and is the behavior that existed before. + Expr::FeatureState { key } => Dependency::FEATURE + .join(Dependency::STATE) + .join(classify(key)), // The join over whatever the options read, which is the feature in the specification's // own case: it takes the locale and the digit bounds off the feature's tags. Expr::NumberFormat { .. } => children(expr) diff --git a/crates/tessella-style/src/expression/parse.rs b/crates/tessella-style/src/expression/parse.rs index cc0a568f..0ee400cf 100644 --- a/crates/tessella-style/src/expression/parse.rs +++ b/crates/tessella-style/src/expression/parse.rs @@ -603,6 +603,12 @@ fn parse_rooted( expect_arity(operator, args, 0, 0)?; Ok(Expr::Properties) } + "feature-state" => { + expect_arity(operator, args, 1, 1)?; + Ok(Expr::FeatureState { + key: Box::new(parse_in(&args[0], scope)?), + }) + } "get" => { // With a second argument the lookup is in *that* object rather than in the feature, // which also means the expression stops depending on the feature at all — the diff --git a/crates/tessella-style/src/property.rs b/crates/tessella-style/src/property.rs index 1e328f6b..0af60fd1 100644 --- a/crates/tessella-style/src/property.rs +++ b/crates/tessella-style/src/property.rs @@ -217,6 +217,18 @@ pub enum PropertyError { /// What the style wrote. got: &'static str, }, + /// A layout property reads per-feature state, which the specification allows in a paint + /// property only. + /// + /// Worth its own error for [`Self::NotDataDriven`]'s reason, and a sharper one: layout decides + /// a feature's geometry and is cut once when the tile is built, so a value a host changes + /// between frames has no way to reach it. Tolerating it would draw the state the tile happened + /// to be built with and never change again -- a highlight that works until the first pan. + #[error("`{property}` is a layout property and cannot read feature state")] + FeatureStateInLayout { + /// Property name. + property: String, + }, /// A property varies per feature that the spec does not allow to. /// /// Worth its own error rather than being tolerated: a data-driven expression on a property @@ -1127,10 +1139,18 @@ pub fn layout_value( ) -> Option { match layer.layout.get(key)? { PropertyValue::Literal(literal) => Some(literal.clone()), - PropertyValue::Expression(expression) => Expression::parse(expression.value()) - .ok()? - .evaluate(Some(zoom), feature) - .ok(), + PropertyValue::Expression(expression) => { + let parsed = Expression::parse(expression.value()).ok()?; + // Layout cannot read per-feature state, for the reason `Half::Layout` gives: layout is + // cut once when the tile is built and state is set between frames. [`resolve_layout`] + // refuses it as an error; this answers `None`, which is the caller's own default -- + // there is no error channel here, and the layer kinds that reach this function are + // exactly the ones that table has no specs for. + if parsed.dependency().needs_state() { + return None; + } + parsed.evaluate(Some(zoom), feature).ok() + } } } @@ -1148,7 +1168,7 @@ pub fn resolve_paint( layer: &Layer, ) -> Result, PropertyError> { let specs = paint_specs(&layer.kind).unwrap_or(&[]); - let mut resolved = resolve(specs, &layer.paint)?; + let mut resolved = resolve(specs, &layer.paint, Half::Paint)?; apply_layer_rules(layer, &mut resolved); Ok(resolved) } @@ -1198,12 +1218,25 @@ pub fn resolve_layout( layer: &Layer, ) -> Result, PropertyError> { let specs = layout_specs(&layer.kind).unwrap_or(&[]); - resolve(specs, &layer.layout) + resolve(specs, &layer.layout, Half::Layout) +} + +/// Which of a layer's two property maps is being resolved. +/// +/// The only thing it decides is whether `["feature-state", …]` is allowed, which is the one rule +/// that differs between them: a paint property may read state, and a layout property may not -- +/// layout decides a feature's geometry, which is cut once at build, so a value a host changes +/// between frames cannot reach it. The specification says so and mbgl refuses it the same way. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +enum Half { + Paint, + Layout, } fn resolve( specs: &'static [PropertySpec], written: &BTreeMap, + half: Half, ) -> Result, PropertyError> { let mut resolved = BTreeMap::new(); for spec in specs { @@ -1242,6 +1275,11 @@ fn resolve( }; let dependency = expression.dependency(); + if half == Half::Layout && dependency.needs_state() { + return Err(PropertyError::FeatureStateInLayout { + property: spec.name.to_string(), + }); + } if dependency.needs_feature() && !spec.data_driven { return Err(PropertyError::NotDataDriven { property: spec.name.to_string(), diff --git a/crates/tessella-style/tests/feature_state.rs b/crates/tessella-style/tests/feature_state.rs new file mode 100644 index 00000000..b2c7e16d --- /dev/null +++ b/crates/tessella-style/tests/feature_state.rs @@ -0,0 +1,266 @@ +// SPDX-License-Identifier: BSD-2-Clause +//! `["feature-state", key]`: per-feature state a host set, read by a paint property. +//! +//! It was in the generated operator registry and nothing evaluated it (tessella#337), so a style +//! highlighting a feature through it drew every feature in its default state -- silently, because +//! an unknown operator would have been refused and this one parsed and returned nothing. +//! +//! # What state is, and is not +//! +//! Not a property: it is not in the tile and not in the style. A host sets it to mark a feature -- +//! hovered, selected, part of a route -- and it changes while the tile it belongs to stays as it is. +//! That is the whole reason the specification allows it in a *paint* property only, and both of the +//! refusals below are that rule: a filter decides which features exist and a layout property decides +//! their geometry, and each is answered once when the tile is cut. + +use std::collections::BTreeMap; + +use tessella_style::expression::{Expression, Feature}; +use tessella_style::{Value, filter::Filter}; + +/// A feature with properties and state, which is what the two lookups are told apart by. +struct Marked { + properties: BTreeMap, + state: BTreeMap, +} + +impl Feature for Marked { + fn property(&self, key: &str) -> Option { + self.properties.get(key).cloned() + } + + fn geometry_type(&self) -> &str { + "Point" + } + + fn state(&self, key: &str) -> Option { + self.state.get(key).cloned() + } +} + +/// A feature that answers nothing, which is every implementation that predates the operator. +struct Plain; + +impl Feature for Plain { + fn property(&self, _key: &str) -> Option { + None + } + + fn geometry_type(&self) -> &str { + "Point" + } +} + +fn parse(json: &str) -> Expression { + Expression::parse(&serde_json::from_str::(json).expect("the json parses")) + .expect("the expression parses") +} + +fn marked(state: &[(&str, Value)]) -> Marked { + Marked { + properties: [("kind".to_string(), Value::String("road".into()))] + .into_iter() + .collect(), + state: state + .iter() + .map(|(key, value)| ((*key).to_string(), value.clone())) + .collect(), + } +} + +/// The state of the feature being evaluated, by key. +#[test] +fn it_reads_the_state_of_the_feature() { + let expression = parse(r#"["feature-state", "hover"]"#); + let hovered = marked(&[("hover", Value::Bool(true))]); + assert_eq!( + expression + .evaluate(None, Some(&hovered)) + .expect("evaluates"), + Value::Bool(true) + ); +} + +/// A key the feature has no state for is null, as an absent property is. +/// +/// Erroring instead would make every unhighlighted feature a failed evaluation, where what a style +/// writes is `["case", ["feature-state", "hover"], …, …]` and expects the default arm. +#[test] +fn absent_state_is_null_rather_than_an_error() { + let expression = parse(r#"["feature-state", "hover"]"#); + assert_eq!( + expression + .evaluate(None, Some(&marked(&[]))) + .expect("evaluates"), + Value::Null + ); + // And a feature that does not implement the method at all answers the same, which is what the + // trait's default is for: every `Feature` written before this operator keeps working. + assert_eq!( + expression.evaluate(None, Some(&Plain)).expect("evaluates"), + Value::Null + ); +} + +/// State and properties are separate namespaces, which is the point of the operator existing. +/// +/// A key in both answers differently through each, and that is not a curiosity: a host marking +/// `selected` on a feature whose tags also carry `selected` must not have the tag win. +#[test] +fn state_is_not_the_properties() { + let feature = Marked { + properties: [("tone".to_string(), Value::String("tag".into()))] + .into_iter() + .collect(), + state: [("tone".to_string(), Value::String("state".into()))] + .into_iter() + .collect(), + }; + assert_eq!( + parse(r#"["get", "tone"]"#) + .evaluate(None, Some(&feature)) + .expect("evaluates"), + Value::String("tag".into()) + ); + assert_eq!( + parse(r#"["feature-state", "tone"]"#) + .evaluate(None, Some(&feature)) + .expect("evaluates"), + Value::String("state".into()) + ); +} + +/// The idiom a style actually writes: a highlight through `case`, with a default arm. +#[test] +fn a_highlight_picks_the_state_arm_and_falls_back() { + // Two hashes: a color is a `#` straight after a quote, which ends an `r#"..."#` string there. + let expression = + parse(r##"["case", ["==", ["feature-state", "hover"], true], "#ff0000", "#204060"]"##); + let hovered = expression + .evaluate(None, Some(&marked(&[("hover", Value::Bool(true))]))) + .expect("evaluates"); + let plain = expression + .evaluate(None, Some(&marked(&[]))) + .expect("evaluates"); + assert_ne!(hovered, plain, "the two arms drew the same color"); +} + +/// It depends on the feature *and* on the state, which are different occasions. +/// +/// Both bits, because the two are needed for different decisions: 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 would read. +#[test] +fn it_depends_on_the_feature_and_the_state() { + let dependency = parse(r#"["feature-state", "hover"]"#).dependency(); + assert!(dependency.needs_feature(), "{dependency:?}"); + assert!(dependency.needs_state(), "{dependency:?}"); + assert!(!dependency.needs_zoom(), "{dependency:?}"); + assert!(!dependency.is_constant(), "{dependency:?}"); + + // And an expression that does not read it says so, which is what makes the bit worth having. + let ordinary = parse(r#"["get", "kind"]"#).dependency(); + assert!(ordinary.needs_feature()); + assert!(!ordinary.needs_state()); +} + +/// Nested, which is the shape a real style has: the state is read inside an interpolation. +#[test] +fn the_bit_survives_a_nesting() { + let dependency = + parse(r#"["interpolate", ["linear"], ["zoom"], 0, ["feature-state", "width"], 16, 4]"#) + .dependency(); + assert!(dependency.needs_state(), "{dependency:?}"); + assert!(dependency.needs_zoom(), "{dependency:?}"); + assert!(dependency.needs_feature(), "{dependency:?}"); +} + +/// A filter cannot read it. +/// +/// A filter decides which features exist at all and runs once when the tile is built, so a filter +/// over state would answer for whatever the state was then and never change -- which reads as a +/// highlight that works until the first pan. +#[test] +fn a_filter_cannot_read_feature_state() { + let value: Value = + serde_json::from_str(r#"["==", ["feature-state", "hover"], true]"#).expect("json"); + let refused = Filter::parse(&value).expect_err("a filter over state is refused"); + let said = format!("{refused}"); + assert!(said.contains("feature state"), "{said}"); + assert!(said.contains("paint property"), "{said}"); + + // The same filter over a property is fine, which is what says the refusal is about the + // operator rather than about the shape. + let ordinary: Value = serde_json::from_str(r#"["==", ["get", "kind"], "road"]"#).expect("json"); + assert!(Filter::parse(&ordinary).is_ok()); +} + +/// Nor a layout property, for the same reason one level along: layout is cut once. +/// +/// A line layer, because that is a kind `layout_specs` has a table for. The kinds it does not -- +/// symbol above all -- are read through `layout_value`, which has no error channel at all; the test +/// below covers what happens there. +#[test] +fn a_layout_property_cannot_read_feature_state() { + let layer = r#"{ + "id": "l", "type": "line", "source": "s", "source-layer": "roads", + "layout": {"line-sort-key": ["feature-state", "rank"]} + }"#; + let layer: tessella_style::Layer = serde_json::from_str(layer).expect("the layer parses"); + let refused = + tessella_style::property::resolve_layout(&layer).expect_err("layout over state is refused"); + let said = format!("{refused}"); + assert!(said.contains("feature state"), "{said}"); + + // And a paint property on the same layer takes it, which is the allowed half of the rule. + let painted = r##"{ + "id": "l", "type": "fill", "source": "s", "source-layer": "roads", + "paint": {"fill-color": ["case", ["feature-state", "hover"], "#ff0000", "#204060"]} + }"##; + let painted: tessella_style::Layer = serde_json::from_str(painted).expect("the layer parses"); + tessella_style::property::resolve_paint(&painted).expect("a paint property may read state"); +} + +/// A symbol layout property reading state answers nothing, which is the caller's default. +/// +/// `layout_specs` has no table for a symbol layer, so `resolve_layout` answers an empty map for one +/// and the refusal above never sees it. `layout_value` is what reads those properties, one at a +/// time, and it has no error channel: its contract is already "no value, so the caller's own +/// default applies". So this is where the rule lands for the layer kind a host is most likely to +/// try it on -- a label whose text came from state would be baked in when the tile was cut. +#[test] +fn a_symbol_layout_property_answers_nothing_for_state() { + let layer = r#"{ + "id": "l", "type": "symbol", "source": "s", "source-layer": "roads", + "layout": {"text-field": ["feature-state", "label"], "text-size": 18} + }"#; + let layer: tessella_style::Layer = serde_json::from_str(layer).expect("the layer parses"); + // Empty rather than refused, which is the hazard `layout_value` documents. + assert!( + tessella_style::property::resolve_layout(&layer) + .expect("a symbol layer resolves to nothing") + .is_empty() + ); + + let marked = marked(&[("label", Value::String("Main Street".into()))]); + assert_eq!( + tessella_style::property::layout_value(&layer, "text-field", 14.0, Some(&marked)), + None, + "a label read its text from state, which is cut into the tile and cannot change" + ); + // A property that does not read state is unaffected, which is what says the check is about the + // operator rather than about this function. + assert_eq!( + tessella_style::property::layout_value(&layer, "text-size", 14.0, Some(&marked)), + Some(Value::Number(18.0)) + ); +} + +/// One argument, and it must be there. +#[test] +fn the_arity_is_one() { + for json in [r#"["feature-state"]"#, r#"["feature-state", "a", "b"]"#] { + let value: Value = serde_json::from_str(json).expect("json"); + assert!(Expression::parse(&value).is_err(), "{json}"); + } +}