From c66c838539afbd5eaffe2a8b21290c4853bae7df Mon Sep 17 00:00:00 2001 From: Kirk Swenson Date: Thu, 10 Sep 2026 07:58:44 -0700 Subject: [PATCH] CODAP-1526: draw residual points with the category's shape The residual plot draws its own SVG rather than going through a point renderer, so its points stayed circles whatever the upper plot was drawn with. It is the fourth and last surface that draws a point, after the two renderers and the legend key. A point is an outline now, positioned by a translate rather than a center. The radius moves to a data attribute because an outline has none to read back, and everything that decides how a point is drawn reads it from there. Growing a new point regenerates its outline at each step rather than scaling it. A scale takes the stroke with it, and the stroke is what marks a point selected, so a selected point would have faded in with an outline that thickened as it grew. Regenerating leaves the whole of the existing fade-in alone, including the guard against redirecting a transition that is already running. The shape comes from the same lookup the upper plot uses, so the two agree. Corrects the claim that a residual plot never has a legend. It is turned off for a categorical legend only when the line is an LSRL, so a movable line or a plotted function has one -- which is what makes a shape per category reach these points at all, rather than only the display's own. Seven Cypress assertions counted circles. The marquee is untouched: it indexes positions it computes itself, not the elements. Co-Authored-By: Claude Opus 5 --- v3/cypress/e2e/bivariate-adornments.spec.ts | 14 ++-- .../scatter-plot/residual-marquee-utils.ts | 2 +- .../plots/scatter-plot/use-residual-plot.ts | 80 +++++++++++-------- 3 files changed, 54 insertions(+), 42 deletions(-) diff --git a/v3/cypress/e2e/bivariate-adornments.spec.ts b/v3/cypress/e2e/bivariate-adornments.spec.ts index fedabe6fac..37b5756288 100644 --- a/v3/cypress/e2e/bivariate-adornments.spec.ts +++ b/v3/cypress/e2e/bivariate-adornments.spec.ts @@ -371,7 +371,7 @@ context("Graph adornments", () => { cy.get("[data-testid=adornment-checkbox-movable-line]").click() cy.get("[data-testid=adornment-checkbox-residual-plot]").find("input").should("not.have.attr", "disabled") cy.get("[data-testid=adornment-checkbox-residual-plot]").click() - cy.get("*[data-testid^=residual-points-]").find("circle").should("have.length.at.least", 1) + cy.get("*[data-testid^=residual-points-]").find("path").should("have.length.at.least", 1) cy.get(".axis-wrapper.leftLower").should("exist") // V2 behavior: removing the line CLEARS (unchecks) the Residual Plot rather than @@ -392,7 +392,7 @@ context("Graph adornments", () => { // Re-checking restores the residual plot. (Legend-triggered clearing follows the same // residualPlotIsApplicable path and is covered by the graph-content-model integration tests.) cy.get("[data-testid=adornment-checkbox-residual-plot]").click() - cy.get("*[data-testid^=residual-points-]").find("circle").should("have.length.at.least", 1) + cy.get("*[data-testid^=residual-points-]").find("path").should("have.length.at.least", 1) cy.get(".axis-wrapper.leftLower").should("exist") }) it("restyles residual points to reflect selection without tearing down the residual plot", () => { @@ -402,7 +402,7 @@ context("Graph adornments", () => { graph.getDisplayValuesButton().click() cy.get("[data-testid=adornment-checkbox-movable-line]").click() cy.get("[data-testid=adornment-checkbox-residual-plot]").click() - cy.get("*[data-testid^=residual-points-]").find("circle").should("have.length.at.least", 1) + cy.get("*[data-testid^=residual-points-]").find("path").should("have.length.at.least", 1) // A residual point starts unselected — its fill is not the solid selection color. cy.get("*[data-testid^=residual-point-]").first().should("not.have.attr", "fill", "#4682b4") @@ -413,7 +413,7 @@ context("Graph adornments", () => { // sibling circle — force the click on the specific first circle. cy.get("*[data-testid^=residual-point-]").first().click({ force: true }) cy.get("*[data-testid^=residual-point-]").first().should("have.attr", "fill", "#4682b4") - cy.get("*[data-testid^=residual-points-]").find("circle").should("have.length.at.least", 1) + cy.get("*[data-testid^=residual-points-]").find("path").should("have.length.at.least", 1) cy.get(".axis-wrapper.leftLower").should("exist") // Clicking the residual-plot background deselects all cases, reverting the styling. @@ -428,7 +428,7 @@ context("Graph adornments", () => { graph.getDisplayValuesButton().click() cy.get("[data-testid=adornment-checkbox-movable-line]").click() cy.get("[data-testid=adornment-checkbox-residual-plot]").click() - cy.get("*[data-testid^=residual-points-]").find("circle").should("have.length.at.least", 1) + cy.get("*[data-testid^=residual-points-]").find("path").should("have.length.at.least", 1) // No residual point carries the solid selection fill before the drag. cy.get('*[data-testid^=residual-point-][fill="#4682b4"]').should("not.exist") @@ -450,7 +450,7 @@ context("Graph adornments", () => { // The marquee covered the strip, so at least one residual point is now selected (selection fill), // and the residual plot is not torn down. cy.get('*[data-testid^=residual-point-][fill="#4682b4"]').should("have.length.at.least", 1) - cy.get("*[data-testid^=residual-points-]").find("circle").should("have.length.at.least", 1) + cy.get("*[data-testid^=residual-points-]").find("path").should("have.length.at.least", 1) }) it("adds to the existing selection when the residual marquee is shift-dragged", () => { @@ -460,7 +460,7 @@ context("Graph adornments", () => { graph.getDisplayValuesButton().click() cy.get("[data-testid=adornment-checkbox-movable-line]").click() cy.get("[data-testid=adornment-checkbox-residual-plot]").click() - cy.get("*[data-testid^=residual-points-]").find("circle").should("have.length.at.least", 1) + cy.get("*[data-testid^=residual-points-]").find("path").should("have.length.at.least", 1) // Drags a marquee over a horizontal slice of the residual strip, expressed as fractions of its // width. pointermove/pointerup go to the document so they reach the marquee's window listeners. diff --git a/v3/src/components/graph/plots/scatter-plot/residual-marquee-utils.ts b/v3/src/components/graph/plots/scatter-plot/residual-marquee-utils.ts index dfea2efdcf..5ebfbf2aeb 100644 --- a/v3/src/components/graph/plots/scatter-plot/residual-marquee-utils.ts +++ b/v3/src/components/graph/plots/scatter-plot/residual-marquee-utils.ts @@ -9,7 +9,7 @@ export interface IResidualPosition { // Screen position of each residual point, in the graph-SVG coordinate frame: x from the shared // getXCoord, y on the lower axis (plotHeight is the top of the residual region). Mirrors the cx/cy -// used to draw the residual circles, so the hit-test index and the drawn points agree by construction. +// used to draw the residual points, so the hit-test index and the drawn points agree by construction. export function buildResidualPositions( residuals: IResidualPoint[], getXCoord: (caseID: string) => number, plotHeight: number, lowerScale: (residual: number) => number diff --git a/v3/src/components/graph/plots/scatter-plot/use-residual-plot.ts b/v3/src/components/graph/plots/scatter-plot/use-residual-plot.ts index a79296cbd3..729699f68b 100644 --- a/v3/src/components/graph/plots/scatter-plot/use-residual-plot.ts +++ b/v3/src/components/graph/plots/scatter-plot/use-residual-plot.ts @@ -1,4 +1,4 @@ -import { active, format, ScaleLinear, select } from "d3" +import { active, format, interpolateNumber, ScaleLinear, select } from "d3" import { tip as d3tip } from "d3-v6-tip" import { untracked } from "mobx" import { useCallback, useEffect, useRef } from "react" @@ -7,6 +7,7 @@ import { mstAutorun } from "../../../../utilities/mst-autorun" import { t } from "../../../../utilities/translation/translate" import { transitionDuration } from "../../../data-display/data-display-types" import { handleClickOnCase } from "../../../data-display/data-display-utils" +import { pointShapePathData } from "../../../data-display/renderer/point-shapes" import { isNumericAxisModel, NumericAxisModel } from "../../../axis/models/numeric-axis-models" import { IGraphContentModel } from "../../models/graph-content-model" import { IGraphDataConfigurationModel } from "../../models/graph-data-configuration-model" @@ -78,25 +79,29 @@ export function useResidualPlot(props: IUseResidualPlot) { // subscribing to selection. Delegates the pure decision to residualPointStyle (unit-tested). const styleFor = useCallback((caseID: string) => { const isSelected = !!dataset?.isCaseSelected(caseID) - // The legend handling here is intentionally retained even though residualPlotIsApplicable - // currently excludes legends (so legendAttrID is always undefined in this path today). Coloring - // each residual point by its legend category is structurally/mathematically well-defined — each - // point's residual is taken against its own category's line, and the adornments already store a - // line per cell — so this is kept ready for a future legend-supporting version rather than - // removed as dead code. (Enabling it also means dropping the legend exclusion and making the + // residualPlotIsApplicable turns the plot off for a categorical legend only when the line is an + // LSRL, so this runs whenever the line is a movable line or a plotted function. Each point's + // residual is taken against its own category's line and the adornments store a line per cell, + // so coloring by category is well defined. (Allowing it for an LSRL as well means making the // predictor cell-aware; see getPredictor/computeResiduals in residual-plot-utils.) const legendColor = legendAttrID ? dataConfiguration?.getLegendColorForCase(caseID) : undefined - const { pointColor, pointStrokeColor } = graphModel.pointDescription - return residualPointStyle({ - isSelected, hasLegend: !!legendAttrID, legendColor, pointColor, pointStrokeColor, - pointRadius: graphModel.getPointRadius(), selectedRadius: graphModel.getPointRadius('select') - }) + const { pointColor, pointStrokeColor, pointShape } = graphModel.pointDescription + // Resolved here rather than inside residualPointStyle, which decides how a style is derived; the + // shape is carried through unchanged, so it would only be passing itself along. + const shape = dataConfiguration?.getLegendShapeForCase(caseID, pointShape) ?? pointShape + return { + ...residualPointStyle({ + isSelected, hasLegend: !!legendAttrID, legendColor, pointColor, pointStrokeColor, + pointRadius: graphModel.getPointRadius(), selectedRadius: graphModel.getPointRadius('select') + }), + shape + } }, [dataset, legendAttrID, dataConfiguration, graphModel]) - // Selection-only restyle: re-set the selection-dependent attrs on the existing circles. No data + // Selection-only restyle: re-set the selection-dependent attrs on the existing points. No data // join, no residual/predictor recompute — a selection change (via refreshPointSelection) updates // styling without re-running the residual pipeline. - // Compute the style once per circle (styleFor does selection/legend lookups) rather than per attr. + // Compute the style once per point (styleFor does selection/legend lookups) rather than per attr. // // Radius handling: circles that come in at r=0 (enter selection in renderResidualPoints, marking // "newly-appeared point") animate up to their assigned radius via a named "radius" transition @@ -110,16 +115,21 @@ export function useResidualPlot(props: IUseResidualPlot) { // rare enough that finishing at the old target is acceptable — the next applyResidualStyles // after the fade-in completes will snap to the current radius. const applyResidualStyles = useCallback((g: SVGGElement) => { - select(g).selectAll("circle") + select(g).selectAll("path") .each(function (d) { const style = styleFor(d.caseID) const sel = select(this) if (!active(this, "radius")) { - const currentR = +sel.attr("r") + const currentR = +sel.attr("data-r") if (currentR === 0 && style.radius > 0) { - sel.transition("radius").duration(transitionDuration).attr("r", style.radius) + // The outline is regenerated at each step rather than the point being scaled up: a + // scale would take the stroke with it, and the stroke is what marks a point selected. + const grow = interpolateNumber(currentR, style.radius) + sel.transition("radius").duration(transitionDuration) + .attrTween("d", () => (tt: number) => pointShapePathData(style.shape, grow(tt))) + .attr("data-r", style.radius) } else { - sel.attr("r", style.radius) + sel.attr("d", pointShapePathData(style.shape, style.radius)).attr("data-r", style.radius) } } sel @@ -162,7 +172,7 @@ export function useResidualPlot(props: IUseResidualPlot) { const { getXCoord } = scatterPlotFuncs(layout, dataConfiguration) const lowerScale = layout.getAxisScale("leftLower") as ScaleLinear | undefined if (!lowerScale) { - select(g).selectAll("circle").remove() + select(g).selectAll("path").remove() lastRenderedPositionsRef.current = null return } @@ -188,17 +198,21 @@ export function useResidualPlot(props: IUseResidualPlot) { })) const positionsChanged = !samePositions(lastRenderedPositionsRef.current, positioned) lastRenderedPositionsRef.current = new Map(positioned.map(d => [d.caseID, d])) - const selection = select(g).selectAll("circle") + const selection = select(g).selectAll("path") .data(positioned, d => d.caseID) selection.exit().remove() - // Enter: new circles at their final cx/cy with r=0. applyResidualStyles below transitions r up - // to the assigned radius (see currentR===0 branch there), so newly-appeared residual points - // fade in from a dot the way newly-appeared main-plot points do. - const enterSelection = selection.enter().append("circle") + /* + * Enter: new points at their final position with a radius of 0. applyResidualStyles below grows + * them to the assigned radius (see the currentR===0 branch there), so newly-appeared residual + * points fade in from nothing the way newly-appeared main-plot points do. + * + * The radius lives on data-r because a shape is drawn as an outline, which has no radius to read + * back the way a circle does. Everything downstream reads it from there. + */ + const enterSelection = selection.enter().append("path") .attr("data-testid", d => `residual-point-${d.caseID}`) - .attr("cx", d => d.cx) - .attr("cy", d => d.cy) - .attr("r", 0) + .attr("transform", d => `translate(${d.cx}, ${d.cy})`) + .attr("data-r", 0) // Cursor and handlers are bound on enter+update every paint. tipTextFor closes over the x // attribute's ID and name as of this paint, and the join is keyed by case ID, so circles survive // an attribute change — handlers left in place from an earlier paint would report the previous x @@ -222,16 +236,14 @@ export function useResidualPlot(props: IUseResidualPlot) { if (positionsChanged) { if (animateCxCy) { selection.transition("cxcy").duration(transitionDuration) - .attr("cx", d => d.cx) - .attr("cy", d => d.cy) + .attr("transform", d => `translate(${d.cx}, ${d.cy})`) } else { selection.interrupt("cxcy") - .attr("cx", d => d.cx) - .attr("cy", d => d.cy) + .attr("transform", d => `translate(${d.cx}, ${d.cy})`) } } - // Apply current selection styling to enter+update circles without subscribing to selection. - // Enter circles (r=0) get their r transitioned up here. + // Apply current selection styling to enter+update points without subscribing to selection. + // Entering points (radius 0) are grown to their radius here. untracked(() => applyResidualStyles(g)) }, [layout, dataConfiguration, dataset, applyResidualStyles]) @@ -267,7 +279,7 @@ export function useResidualPlot(props: IUseResidualPlot) { const teardown = () => { if (layout.showLowerPlot) layout.setShowLowerPlot(false) if (graphModel.getAxis("leftLower")) graphModel.removeAxis("leftLower") - if (residualPointsRef.current) select(residualPointsRef.current).selectAll("circle").remove() + if (residualPointsRef.current) select(residualPointsRef.current).selectAll("path").remove() // Forget the last-rendered positions so the first paint after re-activation isn't mistaken // for a no-op and skipped. lastRenderedPositionsRef.current = null