diff --git a/DESCRIPTION b/DESCRIPTION index 818abd09..fe0bc866 100644 --- a/DESCRIPTION +++ b/DESCRIPTION @@ -1,7 +1,7 @@ Package: rhtmlDonut Type: Package Title: R htmlwidget package for creating a detailed donut plot -Version: 1.0.13 +Version: 1.0.14 Author: Displayr Maintainer: Displayr Description: R htmlwidget package for creating a detailed donut plot. diff --git a/babel.config.js b/babel.config.js new file mode 100644 index 00000000..7d7a9259 --- /dev/null +++ b/babel.config.js @@ -0,0 +1,8 @@ +// Used only by babel-jest when running the spec tests (`rhtml testSpecs`). The production bundle is +// built by esbuild, which does not read this file. Without it jest cannot parse the `import` +// statements that most of theSrc/scripts uses, so the labeller code was untestable in isolation. +module.exports = { + presets: [ + ['@babel/preset-env', { targets: { node: 'current' } }], + ], +} diff --git a/theSrc/scripts/lib/d3pie/labellers/segmentLabeller/mutations/performDescendingOrderCollisionResolution/DescendingOrderCollisionResolver.jest.test.js b/theSrc/scripts/lib/d3pie/labellers/segmentLabeller/mutations/performDescendingOrderCollisionResolution/DescendingOrderCollisionResolver.jest.test.js new file mode 100644 index 00000000..95c83afe --- /dev/null +++ b/theSrc/scripts/lib/d3pie/labellers/segmentLabeller/mutations/performDescendingOrderCollisionResolution/DescendingOrderCollisionResolver.jest.test.js @@ -0,0 +1,177 @@ +const DescendingOrderCollisionResolver = require('./DescendingOrderCollisionResolver') +const OuterLabel = require('../../outerLabel') +const computeCoordOnEllipse = require('../../utils/computeCoordOnEllipse') + +// The resolver only ever talks to the canvas through the small interface that SegmentLabeller +// builds in extendCanvasInterface, so we can drive it headlessly by supplying that interface +// directly. getLabelSize is the only part that really needs a DOM (it measures text in an SVG), so +// here it is approximated with a fixed per character width. +const buildStubCanvas = ({ + width = 600, + height = 600, + outerRadius = 150, + labelOffset = 15, + maxVerticalOffset = 60, + charWidth = 0.5, +} = {}) => { + const pieCenter = { x: width / 2, y: height / 2 } + return { + width, + height, + outerRadius, + labelOffset, + maxVerticalOffset, + pieCenter, + getLabelSize: ({ labelText, fontSize }) => { + const lineHeight = fontSize * 1.2 + return { + lineHeight, + height: lineHeight, + width: labelText.length * fontSize * charWidth, + labelTextLines: [labelText], + } + }, + computeCoordOnEllipse: ({ angle, radialWidth, radialHeight }) => computeCoordOnEllipse({ + angle, + radialWidth: radialWidth || outerRadius + labelOffset, + radialHeight: radialHeight || outerRadius + labelOffset, + pieCenter, + }), + labelIsInBounds: (label) => + (label.minX >= 0) && (label.maxX <= width) && (label.minY >= 0) && (label.maxY <= height), + } +} + +// Mirrors SegmentLabeller.buildLabels: segments are laid out in input order, and the resolver +// relies on that order being descending by value (id 0 is the largest label). +const buildLabelSet = ({ values, canvas, fontSize = 10 }) => { + const canvasInterface = () => canvas + const totalValue = values.reduce((total, value) => total + value, 0) + let cumulativeValue = 0 + + return values.map((value, index) => { + const angleExtent = value * 360 / totalValue + const angleStart = cumulativeValue * 360 / totalValue + cumulativeValue += value + + return new OuterLabel({ + canvasInterface, + color: '#333333', + displayDecimals: 0, + displayPercentage: true, + fontFamily: 'arial', + fontSize, + proportion: value / totalValue, + group: null, + id: index, + innerPadding: 1, + label: `Category ${index}`, + segmentAngleMidpoint: angleStart + angleExtent / 2, + value, + }) + }) +} + +// The resolver moves labels one angleIncrement at a time, so counting placements is a direct +// measure of how much work it did. A budget that throws keeps a runaway from hanging the test run +// the same way it hangs the browser. +const withLabelMovementBudget = (budget, fn) => { + const place = OuterLabel.prototype.placeLabelViaConnectorCoordOnEllipse + let moves = 0 + OuterLabel.prototype.placeLabelViaConnectorCoordOnEllipse = function (...args) { + moves++ + if (moves > budget) { + throw new Error(`exceeded the budget of ${budget} label movements`) + } + return place.apply(this, args) + } + + try { + const result = fn() + return { result, moves } + } finally { + OuterLabel.prototype.placeLabelViaConnectorCoordOnEllipse = place + } +} + +const resolve = ({ values, canvasOptions, fontSize, budget = 100000 }) => { + const canvas = buildStubCanvas(canvasOptions) + const labelSet = buildLabelSet({ values, canvas, fontSize }) + + return withLabelMovementBudget(budget, () => new DescendingOrderCollisionResolver({ + labelSet, + variant: { labelMaxLineAngle: 80, minProportion: 0.003 }, + invariant: { liftOffAngle: 30, outerPadding: 1 }, + canvas, + }).go()) +} + +describe('DescendingOrderCollisionResolver', () => { + it('places a label set that has no collisions without moving anything', () => { + const { result, moves } = resolve({ values: Array(8).fill(10) }) + + expect(result.outer).toHaveLength(8) + // one placement per label for the initial layout, then nothing to resolve + expect(moves).toEqual(8) + }) + + // RS-23152. With enough equally sized segments the largest label sits just clockwise of 0 degrees, + // and the counter clockwise sweep pushes it across the seam. Before the fix the angle it was moved + // to cycled 0 -> 360.5 -> 0 forever, so the sweep's `while` loop never terminated and the browser + // tab locked up with "Page unresponsive". + it('terminates when the counter clockwise sweep pushes the largest label past 0 degrees', () => { + const { result } = resolve({ values: Array(60).fill(10) }) + + expect(result.outer.length).toBeGreaterThan(0) + }) + + // The same freeze, reached with an uneven distribution rather than a uniform one. + it.each([40, 50, 60, 70, 80, 90, 100])('terminates for %i equally sized segments', (segmentCount) => { + const { result } = resolve({ values: Array(segmentCount).fill(10) }) + + expect(result.outer.length).toBeGreaterThan(0) + }) + + it('terminates for a long tail of small segments', () => { + const values = [100, 80, 60, 40].concat(Array(80).fill(1)) + + const { result } = resolve({ values }) + + expect(result.outer.length).toBeGreaterThan(0) + }) + + // Defence in depth: even if some future change reintroduces a placement that fails to make + // angular progress, no label may be walked more than once around the ellipse. + it('never moves a single label more than once around the ellipse', () => { + const canvas = buildStubCanvas() + const labelSet = buildLabelSet({ values: Array(60).fill(10), canvas }) + const movesPerLabel = {} + + // 720 steps of 0.5 degrees is one full revolution. go() retries the layout once per extraHeight + // variation and each attempt runs several sweeps, so allow a generous multiple of that while + // still catching a label that is stuck going round and round. The budget is enforced as it goes + // rather than asserted afterwards, so a regression fails the run instead of hanging it. + const perLabelBudget = 720 * 20 + const place = OuterLabel.prototype.placeLabelViaConnectorCoordOnEllipse + OuterLabel.prototype.placeLabelViaConnectorCoordOnEllipse = function (...args) { + movesPerLabel[this.id] = (movesPerLabel[this.id] || 0) + 1 + if (movesPerLabel[this.id] > perLabelBudget) { + throw new Error(`label ${this.id} was moved more than ${perLabelBudget} times`) + } + return place.apply(this, args) + } + + try { + new DescendingOrderCollisionResolver({ + labelSet, + variant: { labelMaxLineAngle: 80, minProportion: 0.003 }, + invariant: { liftOffAngle: 30, outerPadding: 1 }, + canvas, + }).go() + } finally { + OuterLabel.prototype.placeLabelViaConnectorCoordOnEllipse = place + } + + expect(Object.keys(movesPerLabel).length).toBeGreaterThan(0) + }) +}) diff --git a/theSrc/scripts/lib/d3pie/labellers/segmentLabeller/mutations/performDescendingOrderCollisionResolution/DescendingOrderCollisionResolver.js b/theSrc/scripts/lib/d3pie/labellers/segmentLabeller/mutations/performDescendingOrderCollisionResolution/DescendingOrderCollisionResolver.js index c20e94fd..fc5aed70 100644 --- a/theSrc/scripts/lib/d3pie/labellers/segmentLabeller/mutations/performDescendingOrderCollisionResolution/DescendingOrderCollisionResolver.js +++ b/theSrc/scripts/lib/d3pie/labellers/segmentLabeller/mutations/performDescendingOrderCollisionResolution/DescendingOrderCollisionResolver.js @@ -4,6 +4,7 @@ import { extractAndThrowIfNullFactory } from '../../mutationHelpers' import { terminateLoop } from '../../../../../loopControls' import RBush from 'rbush' import { labelLogger } from '../../../../../logger' +import { normaliseAngle } from '../../../../math' const CC = 'COUNTER_CLOCKWISE' const CW = 'CLOCKWISE' @@ -18,8 +19,6 @@ const INVARIABLE_CONFIG = [ 'outerPadding', ] -const boundedAngle = (angle) => (angle < 0) ? 360 - angle : angle % 360 - class DescendingOrderCollisionResolver { constructor ({ labelSet, variant, invariant, canvas }) { this.extractConfig({ variant, invariant }) @@ -182,8 +181,8 @@ class DescendingOrderCollisionResolver { const nearestLargerNeighbor = wrappedLabelSet.getNearestActiveLargerNeighbor(label) if (nearestLargerNeighbor && nearestLargerNeighbor.labelAngle > label.labelAngle) { labelLogger.debug(`${logPrefix} sweep${sweepState.sweepCount} CW: detected ${label.shortText} got left behind. Pushing Pushing ${CW}`) - const newLineConnectorCoord = getLabelCoordAt(boundedAngle(nearestLargerNeighbor.labelAngle + angleIncrement)) - wrappedLabelSet.moveLabel(label, newLineConnectorCoord, boundedAngle(nearestLargerNeighbor.labelAngle + angleIncrement)) + const newLineConnectorCoord = getLabelCoordAt(normaliseAngle(nearestLargerNeighbor.labelAngle + angleIncrement)) + wrappedLabelSet.moveLabel(label, newLineConnectorCoord, normaliseAngle(nearestLargerNeighbor.labelAngle + angleIncrement)) } const labelLineAngleExceededTooFarClockWise = (label) => @@ -210,8 +209,8 @@ class DescendingOrderCollisionResolver { labelLogger.debug(`${label.shortText} out of bounds`) } } - const newLineConnectorCoord = getLabelCoordAt(boundedAngle(label.labelAngle + angleIncrement)) - wrappedLabelSet.moveLabel(label, newLineConnectorCoord, boundedAngle(label.labelAngle + angleIncrement)) + const newLineConnectorCoord = getLabelCoordAt(normaliseAngle(label.labelAngle + angleIncrement)) + wrappedLabelSet.moveLabel(label, newLineConnectorCoord, normaliseAngle(label.labelAngle + angleIncrement)) } if (labelLogger.isDebugEnabled()) { @@ -268,8 +267,8 @@ class DescendingOrderCollisionResolver { const nearestSmallerNeighbor = wrappedLabelSet.getNearestActiveSmallerNeighbor(label) if (nearestSmallerNeighbor && nearestSmallerNeighbor.labelAngle < label.labelAngle) { labelLogger.debug(`${logPrefix} sweep${sweepState.sweepCount} ${CC}: detected ${label.shortText} got left behind. Pushing ${CC}`) - const newLineConnectorCoord = getLabelCoordAt(boundedAngle(nearestSmallerNeighbor.labelAngle - angleIncrement)) - wrappedLabelSet.moveLabel(label, newLineConnectorCoord, boundedAngle(nearestSmallerNeighbor.labelAngle - angleIncrement)) + const newLineConnectorCoord = getLabelCoordAt(normaliseAngle(nearestSmallerNeighbor.labelAngle - angleIncrement)) + wrappedLabelSet.moveLabel(label, newLineConnectorCoord, normaliseAngle(nearestSmallerNeighbor.labelAngle - angleIncrement)) } const labelLineAngleExceededTooFarCounterClockWise = (label) => @@ -296,8 +295,8 @@ class DescendingOrderCollisionResolver { labelLogger.debug(`${label.shortText} out of bounds`) } } - const newLineConnectorCoord = getLabelCoordAt(boundedAngle(label.labelAngle - angleIncrement)) - wrappedLabelSet.moveLabel(label, newLineConnectorCoord, boundedAngle(label.labelAngle - angleIncrement)) + const newLineConnectorCoord = getLabelCoordAt(normaliseAngle(label.labelAngle - angleIncrement)) + wrappedLabelSet.moveLabel(label, newLineConnectorCoord, normaliseAngle(label.labelAngle - angleIncrement)) } if (labelLogger.isDebugEnabled()) { diff --git a/theSrc/scripts/lib/d3pie/math.jest.test.js b/theSrc/scripts/lib/d3pie/math.jest.test.js index ad61ec7a..dfea1f15 100644 --- a/theSrc/scripts/lib/d3pie/math.jest.test.js +++ b/theSrc/scripts/lib/d3pie/math.jest.test.js @@ -1,5 +1,45 @@ const math = require('./math') +describe('math.normaliseAngle', () => { + it('leaves angles already in [0, 360) untouched', () => { + expect(math.normaliseAngle(0)).toEqual(0) + expect(math.normaliseAngle(0.5)).toEqual(0.5) + expect(math.normaliseAngle(180)).toEqual(180) + expect(math.normaliseAngle(359.5)).toEqual(359.5) + }) + + it('wraps angles at or above 360 back into range', () => { + expect(math.normaliseAngle(360)).toEqual(0) + expect(math.normaliseAngle(360.5)).toEqual(0.5) + expect(math.normaliseAngle(720)).toEqual(0) + }) + + // NB the reason this fn exists. The previous implementation returned `360 - angle` for negatives, + // which sends -0.5 to 360.5 (i.e. further counter clockwise past the seam rather than just below + // it). See RS-23152. + it('wraps negative angles back into range', () => { + expect(math.normaliseAngle(-0.5)).toEqual(359.5) + expect(math.normaliseAngle(-1)).toEqual(359) + expect(math.normaliseAngle(-90)).toEqual(270) + expect(math.normaliseAngle(-360)).toEqual(0) + expect(math.normaliseAngle(-360.5)).toEqual(359.5) + }) + + // RS-23152: stepping counter clockwise by a fixed increment must always make progress. The old + // implementation formed a closed cycle across the seam (0 -> 360.5 -> 0 -> ...), so the collision + // resolver's `while` loop could never terminate. + it('stepping counter clockwise across the seam always makes progress', () => { + const increment = 0.5 + let angle = 2 + const visited = new Set() + for (let step = 0; step < 720; step++) { + angle = math.normaliseAngle(angle - increment) + expect(visited.has(angle)).toBe(false) + visited.add(angle) + } + }) +}) + describe('math.angleAbsoluteDifference', () => { it('simple', () => { expect(math.angleAbsoluteDifference(1, 2)).toEqual(1) diff --git a/theSrc/scripts/lib/d3pie/math.js b/theSrc/scripts/lib/d3pie/math.js index 1c309eb3..bafd6fb2 100644 --- a/theSrc/scripts/lib/d3pie/math.js +++ b/theSrc/scripts/lib/d3pie/math.js @@ -112,6 +112,10 @@ let math = { return angle }, + // Brings an angle in degrees back into [0, 360), in either direction. NB the modulo is applied + // twice because javascript's % keeps the sign of the dividend, so -0.5 % 360 is -0.5, not 359.5. + normaliseAngle: (angleInDegrees) => ((angleInDegrees % 360) + 360) % 360, + inclusiveBetween: (a, b, c) => (a <= b && b <= c), exclusiveBetween: (a, b, c) => (a < b && b < c), between: (a, b, c) => (a <= b && b < c), diff --git a/theSrc/test/snapshots/ci/master/testPlans/examples_misc_600x600/examples_misc_600x600_data_misc_browser_stats_gradient-snap.png b/theSrc/test/snapshots/ci/master/testPlans/examples_misc_600x600/examples_misc_600x600_data_misc_browser_stats_gradient-snap.png index 7ab663ce..b3e6ea20 100644 Binary files a/theSrc/test/snapshots/ci/master/testPlans/examples_misc_600x600/examples_misc_600x600_data_misc_browser_stats_gradient-snap.png and b/theSrc/test/snapshots/ci/master/testPlans/examples_misc_600x600/examples_misc_600x600_data_misc_browser_stats_gradient-snap.png differ diff --git a/theSrc/test/snapshots/ci/master/testPlans/examples_misc_600x600/examples_misc_600x600_data_misc_browser_stats_ordered-snap.png b/theSrc/test/snapshots/ci/master/testPlans/examples_misc_600x600/examples_misc_600x600_data_misc_browser_stats_ordered-snap.png index e79464de..75df8ad7 100644 Binary files a/theSrc/test/snapshots/ci/master/testPlans/examples_misc_600x600/examples_misc_600x600_data_misc_browser_stats_ordered-snap.png and b/theSrc/test/snapshots/ci/master/testPlans/examples_misc_600x600/examples_misc_600x600_data_misc_browser_stats_ordered-snap.png differ diff --git a/theSrc/test/snapshots/ci/master/testPlans/issues/vis438-snap.png b/theSrc/test/snapshots/ci/master/testPlans/issues/vis438-snap.png index 67cea7aa..290ce160 100644 Binary files a/theSrc/test/snapshots/ci/master/testPlans/issues/vis438-snap.png and b/theSrc/test/snapshots/ci/master/testPlans/issues/vis438-snap.png differ diff --git a/theSrc/test/snapshots/ci/master/testPlans/label_placement/42_labels_with_lots_of_equal_values-snap.png b/theSrc/test/snapshots/ci/master/testPlans/label_placement/42_labels_with_lots_of_equal_values-snap.png index 9ff692ac..28b686b4 100644 Binary files a/theSrc/test/snapshots/ci/master/testPlans/label_placement/42_labels_with_lots_of_equal_values-snap.png and b/theSrc/test/snapshots/ci/master/testPlans/label_placement/42_labels_with_lots_of_equal_values-snap.png differ diff --git a/theSrc/test/snapshots/ci/master/testPlans/label_placement/decreasing_values_100_label_24_36_vary_size-3-snap.png b/theSrc/test/snapshots/ci/master/testPlans/label_placement/decreasing_values_100_label_24_36_vary_size-3-snap.png index 96dd4f52..f906e1f6 100644 Binary files a/theSrc/test/snapshots/ci/master/testPlans/label_placement/decreasing_values_100_label_24_36_vary_size-3-snap.png and b/theSrc/test/snapshots/ci/master/testPlans/label_placement/decreasing_values_100_label_24_36_vary_size-3-snap.png differ diff --git a/theSrc/test/snapshots/ci/master/testPlans/label_placement/decreasing_values_100_with_offsets-2-snap.png b/theSrc/test/snapshots/ci/master/testPlans/label_placement/decreasing_values_100_with_offsets-2-snap.png index 552b3f20..ed62b67c 100644 Binary files a/theSrc/test/snapshots/ci/master/testPlans/label_placement/decreasing_values_100_with_offsets-2-snap.png and b/theSrc/test/snapshots/ci/master/testPlans/label_placement/decreasing_values_100_with_offsets-2-snap.png differ diff --git a/theSrc/test/snapshots/ci/master/testPlans/label_placement/decreasing_values_200_with_offsets_label_16-1-snap.png b/theSrc/test/snapshots/ci/master/testPlans/label_placement/decreasing_values_200_with_offsets_label_16-1-snap.png index 9c39eb07..57adbaf7 100644 Binary files a/theSrc/test/snapshots/ci/master/testPlans/label_placement/decreasing_values_200_with_offsets_label_16-1-snap.png and b/theSrc/test/snapshots/ci/master/testPlans/label_placement/decreasing_values_200_with_offsets_label_16-1-snap.png differ diff --git a/theSrc/test/snapshots/ci/master/testPlans/label_placement/decreasing_values_794_company_earnings_small-snap.png b/theSrc/test/snapshots/ci/master/testPlans/label_placement/decreasing_values_794_company_earnings_small-snap.png index 3fb3319e..0aeda500 100644 Binary files a/theSrc/test/snapshots/ci/master/testPlans/label_placement/decreasing_values_794_company_earnings_small-snap.png and b/theSrc/test/snapshots/ci/master/testPlans/label_placement/decreasing_values_794_company_earnings_small-snap.png differ