Conversation
Merging this PR will not alter performance
Comparing Footnotes
|
Alek99
left a comment
There was a problem hiding this comment.
Review findings:
- [P1]
js/src/50_chartview.js:2330: the simple scatter path does not excludeg.styleBuf. Constant-color/size circles with vector opacity, artist alpha, symbol, or stroke width are sent to_drawSimplePoints, which binds only x/y and scalar opacity; the new channels are ignored. - [P1]
python/xy/_raster.py:1500/python/xy/_svg.py:2129: scalarstroke=remains instyle, but the new static paths only inspecttrace["stroke"]and otherwise use the face paint.bar(color="red", stroke="blue")exports a red outline. For meshes this also forces one raster command per triangle with varying fills, matching the CodSpeed regression. - [P1]
js/src/40_gl.js:776plus the SVG/PNG gradient branches: gradient rectangles bypass vector opacity/artist alpha; static paths also discard vector stroke/width (and PNG corner radii). Preserve the resolved instance channels in all three renderers. - [P2]
js/src/40_gl.js:253,580,790: constant outlines use a scalar premultiplied uniform, so per-item opacity/artist alpha affects fills but not scalar strokes. The match-fill fallback also uses a trace color rather than each direct face color. - [P2]
python/xy/_trace.py:87: style/stroke-only scatters aggregate at 200k rather than retaining direct rendering until the documented 2M per-item ceiling. - [P2]
python/xy/marks.py:1572: histogram scalar_artist_alphais discarded; vector alpha works. - [P2]
js/src/50_chartview.js:3955: destruction omits the new rgba/style/stroke/radius buffers, leaking GPU allocations. - Required checks are currently failing: protocol was bumped to 4 while repository smoke specs remain at 3, and docs lint finds the missing
_artist_alphascatter parameter documentation.
Review fixes for the per-item style channel refactor: - Static exporters (SVG + PNG) resolved rect-family and triangle-mesh strokes from the trace stroke channel or the face paint only, dropping a scalar CSS stroke= into the face color. Restore the style['stroke'] fallback the scatter emitters already had. - SVG segments emitted a translucent constant color verbatim in stroke= while also feeding its intrinsic alpha into stroke-opacity, applying it twice. Gate the verbatim pass-through on an opaque color, matching the scatter path. - The affine-channel raster fast path ignored a direct per-point stroke channel, silently dropping RGBA edgecolors when combined with a colormap c=. Defer to the styled painter, like the second fast path. - WebGL uniform strokes (scalar CSS edge colors) bypassed the artist alpha / per-item opacity stack that direct stroke buffers and both static exporters apply. Ship u_stroke/u_ptStroke straight-alpha and premultiply in-shader through the same alpha stack for both modes. Adds SVG and PNG regression tests for all three exporter paths.
Greptile SummaryThis PR adds vectorized per-item styling across interactive and static chart rendering. The main changes are:
Confidence Score: 5/5This looks safe to merge. The gradient rectangle and bar paths now apply per-item opacity and artist alpha. The packaged browser clients include the same shader update. No blocking issue remains in the changed code.
What T-Rex did
Important Files Changed
Reviews (2): Last reviewed commit: "Preserve vector alpha for gradient recta..." | Re-trigger Greptile |
) Semantic resolutions: - widget.py: keep this branch's _configure_transport (callback-gated view transport) and adopt #98's expanded append() style-channel signature; the transport hook now runs on the expanded message. - render_smoke_nonumpy.py: the probe string keeps this branch's xonly flag and gains #98's vstyle via baseWithStyle. - Bundles and example assets regenerated from the merged js/src (contains containment + style channels + context release); the plotly comparison baseline stays at main's exact bytes.
unifies style attributes so per-item attributes can vary (mainly to allow setting alpha per-item; but also adds changes to color, line-height, etc.)