Cleanup render pipeline#1131
Conversation
…e dead parley_layout feature
📝 WalkthroughWalkthroughThis PR removes Parley feature branches and compositor APIs, updates computed CSS and inline layout handling, renames tile readiness states, adds pixel-format-aware SVG caching, adjusts renderer backend contracts, and refreshes render-pipeline documentation. ChangesRender pipeline alignment
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant DOMStyles
participant PipelineDocument
participant TaffyLayouter
participant FontSystem
participant Rasterizer
DOMStyles->>PipelineDocument: resolve computed CSS
PipelineDocument->>TaffyLayouter: provide border and text-align values
TaffyLayouter->>FontSystem: measure and shape text
TaffyLayouter->>Rasterizer: emit layout and paint commands
Rasterizer->>Rasterizer: reuse format-compatible SVG or rasterize
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/gosub_render_pipeline/src/layering/layer.rs (1)
230-247: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove redundant
get_layer_mutto avoid double lookups and fix unreachable code.The current implementation of
get_layer_mutfilters out missing layers usingcontains_key. Consequently, theelseblock containing thelog::warn!insideadd_to_layeris unreachable when a layer is missing.You can simplify the logic, remove the double HashMap lookup, and restore the intended warning by directly accessing the entry.
♻️ Proposed refactor
- fn add_to_layer(&self, layer_id: LayerId, element_id: LayoutElementId) { - if let Some(mut layers) = self.get_layer_mut(layer_id) { - if let Some(layer) = layers.get_mut(&layer_id) { - layer.add_element(element_id); - } else { - log::warn!("Layer {} not found in HashMap", layer_id); - } - } - } - - fn get_layer_mut(&self, layer_id: LayerId) -> Option<parking_lot::RwLockWriteGuard<'_, HashMap<LayerId, Layer>>> { - let layers = self.layers.write(); - if layers.contains_key(&layer_id) { - Some(layers) - } else { - None - } - } + fn add_to_layer(&self, layer_id: LayerId, element_id: LayoutElementId) { + if let Some(layer) = self.layers.write().get_mut(&layer_id) { + layer.add_element(element_id); + } else { + log::warn!("Layer {} not found in HashMap", layer_id); + } + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/gosub_render_pipeline/src/layering/layer.rs` around lines 230 - 247, Remove the redundant get_layer_mut method and update add_to_layer to acquire the layers write guard directly, then use a single get_mut(&layer_id) lookup to add the element or emit the existing warning when the layer is absent.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@crates/gosub_render_pipeline/src/layering/layer.rs`:
- Around line 230-247: Remove the redundant get_layer_mut method and update
add_to_layer to acquire the layers write guard directly, then use a single
get_mut(&layer_id) lookup to add the element or emit the existing warning when
the layer is absent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: d25d3b8e-b388-4c56-9a45-99bb02fd7dda
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (79)
Cargo.tomlcrates/gosub_engine/Cargo.tomlcrates/gosub_engine/src/engine/context.rscrates/gosub_render_pipeline/Cargo.tomlcrates/gosub_render_pipeline/src/common/browser_state.rscrates/gosub_render_pipeline/src/common/document/inline_style.rscrates/gosub_render_pipeline/src/common/document/node.rscrates/gosub_render_pipeline/src/common/document/pipeline_doc.rscrates/gosub_render_pipeline/src/common/document/style.rscrates/gosub_render_pipeline/src/common/font.rscrates/gosub_render_pipeline/src/common/font/parley.rscrates/gosub_render_pipeline/src/common/geo.rscrates/gosub_render_pipeline/src/common/media/decoder.rscrates/gosub_render_pipeline/src/common/media/decoder/raster.rscrates/gosub_render_pipeline/src/common/media/decoder/svg.rscrates/gosub_render_pipeline/src/common/media/image.rscrates/gosub_render_pipeline/src/common/media/media.rscrates/gosub_render_pipeline/src/common/media/media_store.rscrates/gosub_render_pipeline/src/common/media/svg.rscrates/gosub_render_pipeline/src/common/texture.rscrates/gosub_render_pipeline/src/common/texture_store.rscrates/gosub_render_pipeline/src/compositor.rscrates/gosub_render_pipeline/src/layering/layer.rscrates/gosub_render_pipeline/src/layouter.rscrates/gosub_render_pipeline/src/layouter/box_model.rscrates/gosub_render_pipeline/src/layouter/css_taffy_converter.rscrates/gosub_render_pipeline/src/layouter/inline_run.rscrates/gosub_render_pipeline/src/layouter/taffy.rscrates/gosub_render_pipeline/src/layouter/text.rscrates/gosub_render_pipeline/src/layouter/text/parley.rscrates/gosub_render_pipeline/src/lib.rscrates/gosub_render_pipeline/src/painter.rscrates/gosub_render_pipeline/src/painter/commands.rscrates/gosub_render_pipeline/src/painter/commands/border.rscrates/gosub_render_pipeline/src/painter/commands/brush.rscrates/gosub_render_pipeline/src/painter/commands/color.rscrates/gosub_render_pipeline/src/painter/commands/gradient.rscrates/gosub_render_pipeline/src/painter/commands/rectangle.rscrates/gosub_render_pipeline/src/painter/commands/text.rscrates/gosub_render_pipeline/src/rasterizer.rscrates/gosub_render_pipeline/src/render.rscrates/gosub_render_pipeline/src/render/backend.rscrates/gosub_render_pipeline/src/render/backends.rscrates/gosub_render_pipeline/src/render/compositor.rscrates/gosub_render_pipeline/src/render/compositor_router.rscrates/gosub_render_pipeline/src/render/tile_composite.rscrates/gosub_render_pipeline/src/rendertree_builder/tree.rscrates/gosub_render_pipeline/src/tests.rscrates/gosub_render_pipeline/src/tiler.rscrates/gosub_renderer_cairo/src/backend.rscrates/gosub_renderer_cairo/src/compositor.rscrates/gosub_renderer_cairo/src/compositor/inner.rscrates/gosub_renderer_cairo/src/lib.rscrates/gosub_renderer_cairo/src/rasterizer.rscrates/gosub_renderer_cairo/src/rasterizer/brush.rscrates/gosub_renderer_cairo/src/rasterizer/svg.rscrates/gosub_renderer_cairo/src/rasterizer/text/glyphs.rscrates/gosub_renderer_dynamic/src/lib.rscrates/gosub_renderer_skia/src/backend.rscrates/gosub_renderer_skia/src/rasterizer.rscrates/gosub_renderer_skia/src/rasterizer/paint.rscrates/gosub_renderer_skia/src/rasterizer/rectangle.rscrates/gosub_renderer_skia/src/rasterizer/svg.rscrates/gosub_renderer_skia/src/rasterizer/text/glyphs.rscrates/gosub_renderer_vello/Cargo.tomlcrates/gosub_renderer_vello/src/backend.rscrates/gosub_renderer_vello/src/backend/font_cache.rscrates/gosub_renderer_vello/src/backend/font_manager.rscrates/gosub_renderer_vello/src/backend/text_renderer.rscrates/gosub_renderer_vello/src/compositor.rscrates/gosub_renderer_vello/src/compositor/inner.rscrates/gosub_renderer_vello/src/gpu_tiles.rscrates/gosub_renderer_vello/src/lib.rscrates/gosub_renderer_vello/src/rasterizer.rscrates/gosub_renderer_vello/src/rasterizer/brush.rscrates/gosub_renderer_vello/src/rasterizer/svg.rscrates/gosub_renderer_vello/src/rasterizer/text/glyphs.rsdocs/render-pipeline/README.mddocs/render-pipeline/stages.md
💤 Files with no reviewable changes (21)
- crates/gosub_render_pipeline/src/common/font/parley.rs
- crates/gosub_renderer_vello/Cargo.toml
- crates/gosub_render_pipeline/src/layouter/box_model.rs
- crates/gosub_renderer_skia/src/rasterizer/paint.rs
- crates/gosub_render_pipeline/src/render.rs
- crates/gosub_render_pipeline/src/lib.rs
- crates/gosub_renderer_cairo/src/compositor/inner.rs
- crates/gosub_renderer_vello/src/compositor/inner.rs
- crates/gosub_render_pipeline/src/compositor.rs
- crates/gosub_render_pipeline/src/layouter/text/parley.rs
- crates/gosub_render_pipeline/src/common/media/media.rs
- crates/gosub_renderer_vello/src/compositor.rs
- crates/gosub_renderer_vello/src/lib.rs
- crates/gosub_renderer_cairo/src/lib.rs
- crates/gosub_renderer_cairo/src/compositor.rs
- crates/gosub_render_pipeline/src/render/compositor_router.rs
- crates/gosub_render_pipeline/Cargo.toml
- crates/gosub_renderer_vello/src/backend/font_manager.rs
- crates/gosub_render_pipeline/src/common/font.rs
- crates/gosub_renderer_vello/src/backend/font_cache.rs
- crates/gosub_render_pipeline/src/common/geo.rs
Summary by CodeRabbit
Bug Fixes
noneorhidden.Documentation