Merge pull request #1131 from jaytaph/cleanup-render-pipeline

Cleanup render pipeline

Author: jaytaph

Cargo.lock

@@ -201,7 +201,7 @@ getrandom = { workspace = true, features = ["wasm_js"] }
 # The renamed aliases force Cargo to merge features on those resolved versions.
 getrandom_v02 = { package = "getrandom", version = "0.2", features = ["js"] }
 getrandom_v03 = { package = "getrandom", version = "0.3", features = ["wasm_js"] }
-# No default features on WASM: excludes sqlite_cookie_store (needs C SQLite) and parley_layout (needs freetype).
+# No default features on WASM: excludes sqlite_cookie_store (needs C SQLite).
 gosub_engine = { path = "./crates/gosub_engine", default-features = false }
 gosub_render_pipeline = { path = "./crates/gosub_render_pipeline" }
 gosub_renderer_vello = { path = "./crates/gosub_renderer_vello" }

Cargo.toml

@@ -80,11 +80,10 @@ gdk4-wayland = { workspace = true, features = [
 gdk4-x11 = { workspace = true, optional = true }
 
 [features]
-default = ["sqlite_cookie_store", "parley_layout"]
+default = ["sqlite_cookie_store"]
 ui_eframe = ["dep:eframe", "dep:egui"]
 winit = ["dep:winit", "dep:wgpu"]
 sqlite_cookie_store = ["r2d2", "r2d2_sqlite"]
-parley_layout = []
 metrics = []
 
 wayland = ["gdk4-wayland"]

crates/gosub_engine/Cargo.toml

@@ -976,7 +976,7 @@ fn pipeline_hover_repaint(
                 continue;
             }
 
-            tile.state = TileState::Clean;
+            tile.state = TileState::Ready;
             let key = (tile_rect.x.to_bits(), tile_rect.y.to_bits(), tile.layer_id.as_u64());
             if let Some(baked) = prev_by_pos.remove(&key) {
                 clean_baked.push(baked);

crates/gosub_engine/src/engine/context.rs

@@ -26,7 +26,6 @@ base64 = { workspace = true }
 anyhow = { workspace = true }
 cow-utils = { workspace = true }
 parking_lot = { workspace = true }
-parley = { workspace = true, default-features = true }
 gosub_shared = { version = "0.1.1", path = "../gosub_shared", registry = "gosub" }
 rayon = "1"
 gosub_interface = { version = "0.1.2", path = "../gosub_interface", registry = "gosub" }
@@ -43,7 +42,6 @@ gosub_css3 = { path = "../gosub_css3", registry = "gosub" }
 url = { workspace = true }
 
 [features]
-parley_layout = []
 wayland = ["gdk4-wayland"]
 x11 = ["gdk4-x11"]
 

crates/gosub_render_pipeline/Cargo.toml

@@ -13,23 +13,19 @@ pub enum WireframeState {
 
 /// Per-tab render settings passed through the pipeline instead of living in a global.
 pub struct BrowserState {
-    /// List of layers that will be visible are set to true
+    /// Indexed by layer id; `true` means the layer is drawn.
     pub visible_layer_list: Vec<bool>,
-    /// Defines if we need to draw wireframes, or the actual content, or both
+    /// Whether to draw wireframes, the actual content, or both.
     pub wireframed: WireframeState,
-    /// Just show the hovered debug node in wireframe
+    /// Restrict wireframing to the hovered node only.
     pub debug_hover: bool,
-    /// Show the tile grid
     pub show_tilegrid: bool,
     /// Draw a 1px red border around every table-cell element (set via GOSUB_DEBUG_TABLE_CELLS=1)
     pub debug_table_cells: bool,
-    /// When set, this is the element that is currently hovered upon
     pub current_hovered_element: Option<LayoutElementId>,
     /// Current viewport offset + size
     pub viewport: Rect,
-    /// LayerList that is currently being rendered
     pub tile_list: Option<RwLock<TileList>>,
-    /// Scale factor for DPI
     pub dpi_scale_factor: f32,
 }
 

crates/gosub_render_pipeline/src/common/browser_state.rs

@@ -4,8 +4,7 @@ use crate::common::document::style::{
 use cow_utils::CowUtils;
 use std::collections::HashMap;
 
-/// Parses a CSS inline `style` attribute value (e.g. `"color: red; width: 100px"`)
-/// into a `NodeStyle`.
+/// Parses an inline `style` attribute value (e.g. `"color: red; width: 100px"`).
 pub fn parse_inline_style_attr(style_attr: &str) -> NodeStyle {
     let mut style = NodeStyle::new();
     for declaration in style_attr.split(';') {
@@ -20,8 +19,7 @@ pub fn parse_inline_style_attr(style_attr: &str) -> NodeStyle {
     style
 }
 
-/// Extract the target of a `url(...)` token from a CSS value string, stripping surrounding
-/// quotes. Returns `None` when there is no `url()` (e.g. a gradient or plain color).
+/// Target of a `url(...)` token, dequoted. `None` when there is no `url()` (gradient, color, ...).
 fn parse_css_url(value: &str) -> Option<String> {
     let start = value.find("url(")? + "url(".len();
     let rest = &value[start..];
@@ -204,12 +202,9 @@ fn apply_border_shorthand(style: &mut NodeStyle, value: &str) {
 }
 
 fn apply_style_kv(style: &mut NodeStyle, key: &str, value: &str) {
-    // Inline `style="…"` parsing has no access to custom properties (`--*`), which live in the
-    // stylesheet cascade. A `var(...)` value therefore can't be resolved here; storing its raw
-    // text would produce an invalid keyword that still *overrides* the (correctly resolved)
-    // cascade declaration - e.g. `style="color:var(--accent-glow)"` would paint black instead of
-    // deferring to `.arrow-link { color: var(--accent-light) }`. Per CSS, a declaration we can't
-    // compute is ignored, so skip it and let the cascade win.
+    // Custom properties (`--*`) live in the cascade, so `var(...)` can't be resolved here. Storing
+    // the raw text would yield an invalid keyword that still overrides the correctly-resolved
+    // cascade declaration. Per CSS an uncomputable declaration is ignored, so let the cascade win.
     if value.cow_to_ascii_lowercase().contains("var(") {
         return;
     }
@@ -481,9 +476,8 @@ fn parse_font_weight(value: &str) -> Value {
     }
 }
 
-/// Map HTML presentation attributes (e.g. `bgcolor`, `width`) to CSS `Value`s.
-/// These have lower specificity than any real CSS rule and are only consulted
-/// when neither the `style` attribute nor the stylesheet provides a value.
+/// Maps HTML presentation attributes (`bgcolor`, `width`, ...) to CSS values. They lose to any
+/// real CSS rule, so consult this only when neither `style` nor the stylesheet supplies a value.
 pub fn html_presentation_attr(attrs: &HashMap<String, String>, prop: &StyleProperty) -> Option<Value> {
     match prop {
         StyleProperty::BackgroundColor => {
@@ -523,13 +517,12 @@ mod tests {
 
     #[test]
     fn var_inline_declaration_is_ignored() {
-        // `var()` can't be resolved during inline parsing, so the declaration is dropped
-        // (leaving the cascade to supply the color) rather than stored as an invalid keyword
-        // that would paint black.
+        // Dropped rather than stored as an invalid keyword that would paint black, so the
+        // cascade still supplies the color.
         let style = parse_inline_style_attr("color: var(--accent-glow)");
         assert!(style.get_own(&StyleProperty::Color).is_none());
 
-        // A concrete color alongside the var() one still applies; only the var() part is skipped.
+        // Only var() is skipped - a concrete color still applies.
         let style = parse_inline_style_attr("color: #123456");
         assert!(matches!(
             style.get_own(&StyleProperty::Color),

crates/gosub_render_pipeline/src/common/document/inline_style.rs

@@ -4,7 +4,6 @@ use std::collections::HashMap;
 
 pub use gosub_shared::node::NodeId;
 
-/// Map of attributes for a html element (a href, src, data-*, etc)
 #[derive(Debug, Clone)]
 pub struct AttrMap {
     attributes: HashMap<String, String>,
@@ -30,11 +29,6 @@ impl AttrMap {
     pub fn set(&mut self, key: &str, value: &str) {
         self.attributes.insert(key.to_string(), value.to_string());
     }
-
-    #[allow(unused)]
-    pub fn all(&self) -> &HashMap<String, String> {
-        &self.attributes
-    }
 }
 
 impl std::fmt::Display for AttrMap {
@@ -49,28 +43,19 @@ impl std::fmt::Display for AttrMap {
     }
 }
 
-/// Data for a html element (tag name, attributes, styles etc)
 #[derive(Clone, Debug)]
 pub struct ElementData {
     pub tag_name: String,
     pub attributes: AttrMap,
-    #[allow(unused)]
-    pub self_closing: bool,
     /// Own CSS properties for this element (only explicitly-set values; no inheritance).
     pub styles: NodeStyle,
 }
 
 impl ElementData {
-    pub fn new(
-        tag_name: String,
-        attributes: Option<AttrMap>,
-        is_self_closing: bool,
-        styles: Option<NodeStyle>,
-    ) -> ElementData {
+    pub fn new(tag_name: String, attributes: Option<AttrMap>, styles: Option<NodeStyle>) -> ElementData {
         ElementData {
             tag_name,
             attributes: attributes.unwrap_or_default(),
-            self_closing: is_self_closing,
             styles: styles.unwrap_or_default(),
         }
     }
@@ -79,21 +64,10 @@ impl ElementData {
         self.styles.get_own(key)
     }
 
-    #[allow(unused)]
     pub fn get_attribute(&self, key: &str) -> Option<&String> {
         self.attributes.get(key)
     }
 
-    #[allow(unused)]
-    pub fn set_attribute(&mut self, key: &str, value: &str) {
-        self.attributes.set(key, value);
-    }
-
-    #[allow(unused)]
-    pub fn is_self_closing(&self) -> bool {
-        self.self_closing
-    }
-
     pub fn is_inline_element(&self) -> bool {
         matches!(
             self.get_style(&StyleProperty::Display),
@@ -154,10 +128,9 @@ impl Node {
         match &self.node_type {
             NodeType::Element(data) => match data.get_style(&StyleProperty::Display) {
                 Some(Value::Display(Display::Inline)) => true,
-                // When no display is explicitly set, use the HTML element's intrinsic type.
-                // CSS initial value is `inline`, but UA stylesheets set `block` for most
-                // HTML structural elements.  Defaulting unknown elements to inline here
-                // would incorrectly group <li>, <h2>, <div> etc. into inline flows.
+                // The CSS initial value is `inline`, but UA stylesheets make most structural
+                // elements `block` - defaulting to inline would group <li>, <h2>, <div> etc.
+                // into inline flows, so fall back to the tag's intrinsic type.
                 None => is_intrinsically_inline(&data.tag_name),
                 _ => false,
             },
@@ -181,10 +154,8 @@ impl Node {
     }
 }
 
-/// Returns true if `tag` is an HTML element that is inline by spec when no CSS is applied.
-/// Block-level elements (`div`, `p`, `h1`–`h6`, `li`, `section`, etc.) return false so that
-/// missing `display` in NodeStyle (e.g. from a UA-stylesheet gap) does not accidentally put
-/// them into an inline formatting context.
+/// Inline-by-spec HTML elements. Block-level tags return false so a missing `display` (e.g. a
+/// UA-stylesheet gap) never drops them into an inline formatting context.
 fn is_intrinsically_inline(tag: &str) -> bool {
     matches!(
         tag.cow_to_ascii_lowercase().as_ref(),