1/7 Widen the mark CSS subset: stroke-linecap and marker-shape - #302
1/7 Widen the mark CSS subset: stroke-linecap and marker-shape#302Alek99 wants to merge 1 commit into
Conversation
`styles.py` accepted nine mark properties and no way to add a tenth without touching three renderers by hand. This adds the two the renderers can honestly agree on today and, in doing so, settles a cap disagreement they had been hiding. `marker-shape` was free: all three renderers have drawn 17 marker symbols for some time and `symbol=` already reached every one of them. Only the CSS spelling was missing, and both spellings compile to the same trace-style value, so the two build byte-identical specs. `stroke-linecap` was not free, and the reason is worth recording. The three renderers disagreed and none of them said so: the native rasterizer capped round (its clamped segment distance field is a capsule), the WebGL client capped butt with a half-pixel bleed, and the SVG writer hardcoded `round` on line paths while the area outline inherited SVG's `butt` — which `_pdf` then read back as `butt` as well. XY's default is now `round` everywhere, chosen because the rasterizer is the reference for static export, and every stroked SVG path names both cap and join instead of letting the format decide. The rasterizer keeps its existing fast path untouched for round/round and routes anything else through `stroke_shaped`, which decomposes a polyline into oriented-box segments plus join primitives (disc, bevel triangle, miter wedge with SVG's limit of 4) max-combined into one scratch buffer, so overlapping join geometry cannot double-darken a translucent stroke. Join geometry is not optional there: butt-capped segments leave a notch at every interior vertex that something has to fill. `stroke-linejoin` is deliberately NOT shipped. SVG emits it and the rasterizer implements all three joins, but the WebGL client draws a polyline as one instanced quad per segment with no join geometry at all — an attempt at a second instanced pass over interior vertices did not survive verification, so the property would have been honored by two renderers out of three. That is precisely the failure the mark style subset exists to prevent, so it waits for the client. The default-join divergence it leaves behind (rasterizer fills the vertex, WebGL leaves the notch) predates this change and is now written down rather than left to be discovered. Caps are verified end to end, not just at the seam: a Rust unit test on coverage past the endpoint, a Python test on rasterized ink, and three Chromium screenshots that hash differently per cap. Absent keys stay off the wire, so specs built before this change are unchanged byte for byte. ABI_VERSION 41 -> 42: OP_STROKE and OP_SMOOTH_STROKE carry two more bytes.
📝 WalkthroughWalkthroughAdds ChangesStyling and rendering updates
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant StyleCompiler
participant TraceRenderer
participant NativeRasterizer
participant SVGWriter
participant WebGLChart
StyleCompiler->>TraceRenderer: compile linecap or marker-shape
TraceRenderer->>NativeRasterizer: serialize cap and join modes
TraceRenderer->>SVGWriter: provide stroke style
TraceRenderer->>WebGLChart: provide linecap trace style
NativeRasterizer->>NativeRasterizer: render shaped stroke
SVGWriter->>SVGWriter: emit cap and join attributes
WebGLChart->>WebGLChart: apply cap-aware shader coverage
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Merging this PR will not alter performance
Comparing 
Footnotes
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
python/xy/_raster.py (1)
252-253: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueUnguarded dict lookups raise a bare
KeyErrorfrom the display-list writer.
_emit_linewraps the style values instr(...), which implies the writer does not fully trust its input, but_CAP_CODES[cap]then raises an opaqueKeyError: 'Round'for anything the CSS layer did not produce (e.g. a trace style mutated directly). A defaulting lookup or an explicit error keeps the failure legible.♻️ Optional hardening
- self.buf.append(_CAP_CODES[cap]) - self.buf.append(_JOIN_CODES[join]) + self.buf.append(_CAP_CODES.get(cap, _CAP_CODES["round"])) + self.buf.append(_JOIN_CODES.get(join, _JOIN_CODES["round"]))Also applies to: 529-530
🤖 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 `@python/xy/_raster.py` around lines 252 - 253, Update _emit_line and the corresponding style-writing path around the later _CAP_CODES/_JOIN_CODES lookups to avoid opaque KeyError failures for unsupported cap or join values. Use a deliberate default or explicit validation error with the invalid value, while preserving existing mappings for valid CSS-produced styles.src/raster.rs (2)
2911-2954: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider covering the dashed path.
The run-splitting loop in
stroke_shaped(Lines 660-703) is the most intricate new branch — it decides where each dash starts and ends, and SVG caps every dash. A dashed variant ofink(CAP_BUTT) < ink(CAP_SQUARE)would pin that behavior; the current tests only exercise the undashed single-run case.🤖 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 `@src/raster.rs` around lines 2911 - 2954, Extend the stroke_caps_shape_the_ends test to cover a dashed stroke through the same stroke_shaped path: configure a nonzero dash pattern and compare ink counts for CAP_BUTT and CAP_SQUARE, preserving the expected ordering that square-capped dashes cover more pixels. Keep the existing undashed assertions and probe unchanged.
2273-2275: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winOut-of-range
cap/joinbytes are silently coerced instead of rejected.Every other enum this decoder reads is range-checked (
color_mode > 2,encoding > 1,size_mode > 1allreturn None). Acapof 7 here quietly renders as butt and ajoinof 7 as bevel. Rejecting keeps the wire contract strict and self-describing.♻️ Suggested change
let cap = r.u8()?; let join = r.u8()?; + if cap > CAP_SQUARE || join > JOIN_BEVEL { + return None; + } stroke_shaped(&mut cv, &pts, width, c, closed, &dash, cap, join);Also applies to: 2720-2723
🤖 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 `@src/raster.rs` around lines 2273 - 2275, Validate the decoded cap and join bytes in both stroke decoding paths before calling stroke_shaped, returning None for values outside their supported enum ranges instead of allowing coercion. Keep valid cap/join values unchanged and apply the same strict wire validation at both occurrences.
🤖 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.
Inline comments:
In `@src/raster.rs`:
- Around line 711-728: Guard the CAP_SQUARE endpoint-extension block in the
run-processing logic so it executes only for open runs, not closed paths.
Preserve the existing square-cap extension for open runs while leaving closed
runs’ endpoints unchanged, including when the wire operation supplies
CAP_SQUARE.
- Around line 786-797: In the pixel accumulation logic, update the touched-index
tracking so an index is pushed only when its stored coverage actually
transitions from zero to nonzero. Use the computed coverage from to_u8(c) before
deciding whether to push, while preserving the maximum-coverage update and
preventing duplicate entries in touched.
---
Nitpick comments:
In `@python/xy/_raster.py`:
- Around line 252-253: Update _emit_line and the corresponding style-writing
path around the later _CAP_CODES/_JOIN_CODES lookups to avoid opaque KeyError
failures for unsupported cap or join values. Use a deliberate default or
explicit validation error with the invalid value, while preserving existing
mappings for valid CSS-produced styles.
In `@src/raster.rs`:
- Around line 2911-2954: Extend the stroke_caps_shape_the_ends test to cover a
dashed stroke through the same stroke_shaped path: configure a nonzero dash
pattern and compare ink counts for CAP_BUTT and CAP_SQUARE, preserving the
expected ordering that square-capped dashes cover more pixels. Keep the existing
undashed assertions and probe unchanged.
- Around line 2273-2275: Validate the decoded cap and join bytes in both stroke
decoding paths before calling stroke_shaped, returning None for values outside
their supported enum ranges instead of allowing coercion. Keep valid cap/join
values unchanged and apply the same strict wire validation at both occurrences.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: c0f7a0b0-e0cc-4814-b85e-156c6c90523d
📒 Files selected for processing (15)
CHANGELOG.mddocs/styling/customize.mddocs/styling/mark-styles.mdjs/src/40_gl.tsjs/src/50_chartview.tspython/xy/_native.pypython/xy/_raster.pypython/xy/_svg.pypython/xy/marks.pypython/xy/styles.pyspec/api/styling.mdsrc/lib.rssrc/raster.rstests/test_css_mark_styles.pytests/test_static_client_security.py
| let mut ends = run.clone(); | ||
| if cap == CAP_SQUARE { | ||
| // A square cap is a butt cap on a segment pushed out by hw, which | ||
| // is the whole difference between the two. | ||
| let extend = |from: (f32, f32), to: (f32, f32)| -> (f32, f32) { | ||
| let (dx, dy) = (to.0 - from.0, to.1 - from.1); | ||
| let len = (dx * dx + dy * dy).sqrt(); | ||
| if len <= 1e-6 { | ||
| return to; | ||
| } | ||
| (to.0 + dx / len * hw, to.1 + dy / len * hw) | ||
| }; | ||
| let head = extend(run[1], run[0]); | ||
| let tail = extend(run[run.len() - 2], run[run.len() - 1]); | ||
| ends[0] = head; | ||
| let end = ends.len() - 1; | ||
| ends[end] = tail; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Square caps are applied to closed paths, which have no ends.
The extension runs for every run, including the closed case, so a closed stroke with CAP_SQUARE grows two spurs at the seam where run[0] and run[last] coincide. Only the round/round fast path is exercised by today's closed callers (the triangle outlines at Line 2699), but the wire op accepts any cap for a closed path.
🐛 Proposed fix
- if cap == CAP_SQUARE {
+ if cap == CAP_SQUARE && !closed {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let mut ends = run.clone(); | |
| if cap == CAP_SQUARE { | |
| // A square cap is a butt cap on a segment pushed out by hw, which | |
| // is the whole difference between the two. | |
| let extend = |from: (f32, f32), to: (f32, f32)| -> (f32, f32) { | |
| let (dx, dy) = (to.0 - from.0, to.1 - from.1); | |
| let len = (dx * dx + dy * dy).sqrt(); | |
| if len <= 1e-6 { | |
| return to; | |
| } | |
| (to.0 + dx / len * hw, to.1 + dy / len * hw) | |
| }; | |
| let head = extend(run[1], run[0]); | |
| let tail = extend(run[run.len() - 2], run[run.len() - 1]); | |
| ends[0] = head; | |
| let end = ends.len() - 1; | |
| ends[end] = tail; | |
| } | |
| let mut ends = run.clone(); | |
| if cap == CAP_SQUARE && !closed { | |
| // A square cap is a butt cap on a segment pushed out by hw, which | |
| // is the whole difference between the two. | |
| let extend = |from: (f32, f32), to: (f32, f32)| -> (f32, f32) { | |
| let (dx, dy) = (to.0 - from.0, to.1 - from.1); | |
| let len = (dx * dx + dy * dy).sqrt(); | |
| if len <= 1e-6 { | |
| return to; | |
| } | |
| (to.0 + dx / len * hw, to.1 + dy / len * hw) | |
| }; | |
| let head = extend(run[1], run[0]); | |
| let tail = extend(run[run.len() - 2], run[run.len() - 1]); | |
| ends[0] = head; | |
| let end = ends.len() - 1; | |
| ends[end] = tail; | |
| } |
🤖 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 `@src/raster.rs` around lines 711 - 728, Guard the CAP_SQUARE
endpoint-extension block in the run-processing logic so it executes only for
open runs, not closed paths. Preserve the existing square-cap extension for open
runs while leaving closed runs’ endpoints unchanged, including when the wire
operation supplies CAP_SQUARE.
| if c <= 0.0 { | ||
| continue; | ||
| } | ||
| let index = (y - by0) * sw + (x - bx0); | ||
| let slot = &mut scratch[index]; | ||
| if *slot == 0 { | ||
| touched.push(index); | ||
| } | ||
| let coverage = to_u8(c); | ||
| if coverage > *slot { | ||
| *slot = coverage; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
touched can record the same pixel twice, double-blending it.
The index is pushed while *slot == 0, but the slot is only updated when to_u8(c) > *slot. A pixel whose first visit yields a sub-quantum coverage (0 < c, to_u8(c) == 0) leaves the slot at 0 and is pushed again by the next piece that touches it. The final loop then blends that pixel twice — precisely the double-darkening on translucent strokes the doc comment above says this function prevents. Gate the push on the slot actually transitioning away from 0.
🐛 Proposed fix
let index = (y - by0) * sw + (x - bx0);
- let slot = &mut scratch[index];
- if *slot == 0 {
- touched.push(index);
- }
let coverage = to_u8(c);
+ let slot = &mut scratch[index];
if coverage > *slot {
+ if *slot == 0 {
+ touched.push(index);
+ }
*slot = coverage;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if c <= 0.0 { | |
| continue; | |
| } | |
| let index = (y - by0) * sw + (x - bx0); | |
| let slot = &mut scratch[index]; | |
| if *slot == 0 { | |
| touched.push(index); | |
| } | |
| let coverage = to_u8(c); | |
| if coverage > *slot { | |
| *slot = coverage; | |
| } | |
| if c <= 0.0 { | |
| continue; | |
| } | |
| let index = (y - by0) * sw + (x - bx0); | |
| let coverage = to_u8(c); | |
| let slot = &mut scratch[index]; | |
| if coverage > *slot { | |
| if *slot == 0 { | |
| touched.push(index); | |
| } | |
| *slot = coverage; | |
| } |
🤖 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 `@src/raster.rs` around lines 786 - 797, In the pixel accumulation logic,
update the touched-index tracking so an index is pushed only when its stored
coverage actually transitions from zero to nonzero. Use the computed coverage
from to_u8(c) before deciding whether to push, while preserving the
maximum-coverage update and preventing duplicate entries in touched.
|
Closing in favour of #310, which carries this same work as a single PR with before/after renders of the cap fix. Generated by Claude Code |
1 participant
First of a seven-PR stack that makes claims about xy's customizability defensible instead of assertable. This one ships capability; the later ones make it measurable and findable.
What changes
marker-shapewas nearly free. All three renderers have drawn 17 marker symbols for some time andsymbol=already reached every one — only the CSS spelling was missing. Both spellings compile to the same trace-style value, so they build byte-identical specs.stroke-linecapwas not free, and the reason is the interesting part: the three renderers disagreed and none of them said so.roundhardcoded on line paths; area outline inherited SVG'sbuttbuttback out of the SVGroundis now the contract everywhere, chosen because the rasterizer is the reference for static export.The rasterizer keeps its existing fast path untouched for round and routes anything else through
stroke_shaped, which decomposes a polyline into oriented-box segments plus join primitives (disc, bevel triangle, miter wedge with SVG's limit of 4), max-combined into one scratch buffer so overlapping join geometry cannot double-darken a translucent stroke. Join geometry is not optional there — butt-capped segments leave a notch at every interior vertex.What is deliberately not here
stroke-linejoindoes not ship. SVG emits it and the rasterizer implements all three joins, but the WebGL client draws a polyline as one instanced quad per segment with no join geometry at all. An attempt at a second instanced pass over interior vertices did not survive verification (its vertex attributes read as NaN; bisected to the attribute fetch, not the shader math), so shipping the property would mean two renderers honoring it and one ignoring it — precisely the failurestyles.pyexists to prevent.The default-join divergence that leaves behind — rasterizer fills the vertex, WebGL leaves the notch — predates this change and is now written down instead of left to be discovered. PR 4 records it as a registry row.
Verification
Caps are checked end to end, not just at the seam:
roundis unchangedsquarepaints more thanbutt)Absent keys stay off the wire, so specs built before this change are byte-identical.
cargo test119 passed ·uv run pytest2326 passed ·node js/build.mjsclean ·render_smoke_nonumpy.pyOKNote for reviewers
ABI_VERSION41 → 42:OP_STROKEandOP_SMOOTH_STROKEcarry two more bytes.Generated by Claude Code
Summary by CodeRabbit
New Features
stroke-linecapstyling for line-based marks, supporting butt, round, and square caps.marker-shapestyling for scatter marks as an alternative to symbol selection.Bug Fixes