Skip to content

CODAP-1509: draw and hit test point shapes in the PIXI renderer - #2697

Open
kswenson wants to merge 3 commits into
leads-point-shapesfrom
CODAP-1509-point-shapes-pixi
Open

CODAP-1509: draw and hit test point shapes in the PIXI renderer#2697
kswenson wants to merge 3 commits into
leads-point-shapesfrom
CODAP-1509-point-shapes-pixi

Conversation

@kswenson

Copy link
Copy Markdown
Member

Draws the seven point shapes in the PIXI/WebGL renderer and hit tests against them. Fifth in the LEADS Point Shapes stack, on top of #2696.

Fixes CODAP-1509.

What was actually needed

Very little plumbing. Nothing in the style path branches on renderer type, so the work done for the canvas renderer already carries shapes to PIXI through the same setPointStyle and matchPointsToData calls — and point-shapes.ts is consumed unchanged, which is what it was built for. The texture cache needed no change either: its key is the whole style, which now carries the shape, so two shapes cannot collide on one texture.

Two things did need care.

Centering

A sprite draws its texture around its anchor, which for points is the middle of the texture. Left to size itself, a texture takes the bounds of the ink — and a triangle's ink is not centered on the point, because the shape sits on its center of area. Anchoring the middle of that box would draw a triangle low by about a third of its radius, reinstating in WebGL exactly the positional bias the canvas renderer was fixed for. At r=8 the offset is 2.9px for a triangle and 1.1px for a star; every other shape is symmetric and unaffected.

The texture is given an explicit frame centered on the point instead, sized by a new pointShapeSymmetricExtent. Circles keep the path they have always had, including sizing their own texture.

Hit testing, and a behavior change worth noticing

Sprites now carry a hitArea that tests the drawn shape, deferring to the same containment the canvas renderer uses so the two renderers agree on what counts as a hit. That matters because graphics acceleration is a user setting — the same document should not pick differently depending on it.

This also tightens picking for circles. Sprites previously had no hitArea at all, so they hit tested against the rectangle of their texture: a click up to about 1.4 radii from a point's center would select it. That stops. It is a fix rather than a regression, but it reaches every existing document, so it is worth a reviewer's attention rather than being buried.

Bars keep the rectangular test, which is what a bar is.

Scope

Behind the pointShapes feature flag, which gates the controls. Verified manually with graphics acceleration on, and with it toggled off and on to confirm the two renderers agree.

Not measured: texture churn with many shapes in play. The count stays bounded — shape and color are both functions of the same legend category — but cleanupUnusedTextures() still has its single call site in the ticker's idle branch.

🤖 Generated with Claude Code

@codecov

codecov Bot commented Sep 10, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 97.91667% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 87.91%. Comparing base (7c1af50) to head (e8f1e90).

Files with missing lines Patch % Lines
...nents/data-display/renderer/pixi-point-renderer.ts 97.43% 1 Missing ⚠️
Additional details and impacted files
@@                        Coverage Diff                         @@
##           CODAP-1504-point-shapes-canvas    #2697      +/-   ##
==================================================================
+ Coverage                           87.89%   87.91%   +0.01%     
==================================================================
  Files                                 821      821              
  Lines                               47486    47527      +41     
  Branches                            12135    12039      -96     
==================================================================
+ Hits                                41738    41782      +44     
+ Misses                               5732     5729       -3     
  Partials                               16       16              
Flag Coverage Δ
cypress 69.57% <63.63%> (+0.01%) ⬆️
jest 63.24% <95.83%> (+0.05%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@cypress

cypress Bot commented Sep 10, 2026

Copy link
Copy Markdown

codap-v3    Run #12244

Run Properties:  status check passed Passed #12244  •  git commit e8f1e90736: null
Project codap-v3
Branch Review CODAP-1509-point-shapes-pixi
Run status status check passed Passed #12244
Run duration 09m 10s
Commit git commit e8f1e90736: null
Committer null
View all properties for this run ↗︎

Test results
Tests that failed  Failures 0
Tests that were flaky  Flaky 1
Tests that did not run due to a developer annotating a test with .skip  Pending 82
Tests that did not run due to a failure in a mocha hook  Skipped 0
Tests that passed  Passing 384
View all changes introduced in this branch ↗︎

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.


💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

@kswenson

Copy link
Copy Markdown
Member Author

Reviewing this stack — five PRs, each based on the one below it, merging bottom-up into leads-point-shapes:

PR Story What it is
#2691 CODAP-1506 (part 1) Format palette restructure — structural, no shape behavior
#2692 CODAP-1505 Per-category shape storage — model only
#2693 CODAP-1506 (part 2) The shape controls — UI
#2696 CODAP-1504 Canvas rendering and hit testing
#2697 CODAP-1509 PIXI/WebGL rendering and hit testing

CODAP-1506 is split across #2691 and #2693, so approving #2691 does not complete that story — it should not leave code review until #2693 is approved too. The other three map one-to-one.

Nothing here is superseded — this is the top of the stack.

Worth your attention:

  • The centering fix. This is the one that would have shipped as a subtle wrong-looking plot rather than an obvious break: a triangle drawn ~2.9px low at r=8, which reads as "the points are slightly off" rather than as a bug in shape rendering.
  • The hit-testing change tightens picking for circles, in every existing document — sprites previously had no hitArea at all and tested against their texture's rectangle. Called out in the description; I'd rather it be argued about than slip through.
  • point-shapes.ts is consumed unchanged from CODAP-1504: draw and hit test point shapes in the canvas renderer #2696. If something about the geometry looks wrong here, it belongs on that PR.

Out of scope by design, each with a ticket: legend keys carrying the shape (CODAP-1507), the residual plot (CODAP-1526).

kswenson and others added 3 commits September 10, 2026 20:12
The smallest box centered on the point that contains the drawn shape, which is
not the box that hugs the ink: a triangle sits on its center of area, so its
outline reaches further above the point than below.

A renderer that positions a shape by the middle of a box needs this rather than
the drawn extent. Drawing into a texture and anchoring it at its center is
exactly that, which is what the PIXI renderer does.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Textures are built from the shared geometry rather than an arc, so the WebGL
renderer draws what the canvas renderer draws. The texture cache needed no
change: its key is the whole style, which now carries the shape, so two shapes
can never collide on one texture.

A sprite's texture is drawn around its anchor, which for points is the middle of
the texture. Left to size itself a texture takes the bounds of the ink, and for
a triangle that box is not centered on the point -- the ink sits above the
center of area, so the triangle would be drawn low by about a third of its
radius, reinstating in WebGL the bias the canvas renderer was fixed for. The
texture is given an explicit frame centered on the point instead. Circles keep
the path they have always had, including sizing their own texture.

Sprites get a hit area testing the drawn shape. PIXI hands it the pointer in the
sprite's own coordinates, where the origin is the point's position, so it can
defer to the same containment the canvas renderer uses and the two agree on what
counts as a hit. Note this also tightens picking for circles, which until now
were hit tested against the rectangle of their texture -- a click well outside a
circular point could select it. Bars keep the rectangular test, which is what a
bar is.

The hit area holds the shape and radius rather than looking them up, since it
runs per point per pointer event. It is kept in step with the texture at all
three places a sprite's texture is assigned: creation, a style change, and the
end of a points/bars transition.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Why a texture has to be centered on the point rather than on the ink was
explained at the helper that computes the frame and again where it is called.
It lives at the helper, which exists for no other reason; the call site says
only that circles keep sizing their own.

The symmetric extent restated why a triangle's box is off center, which the
triangle's own geometry already explains. It now says what it is and who needs
it.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@kswenson
kswenson force-pushed the CODAP-1509-point-shapes-pixi branch from cf96a45 to e8f1e90 Compare September 11, 2026 03:13
Base automatically changed from CODAP-1504-point-shapes-canvas to leads-point-shapes September 11, 2026 21:28
@kswenson
kswenson requested review from emcelroy and removed request for tealefristoe September 12, 2026 07:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants