Skip to content

Commit

Permalink
Fix a bug where line-ending arrows would flip around or vanish (#1263)
Browse files Browse the repository at this point in the history
The bug happened when `-0` was passed as a coordinate (yes, `-0` is a distinct
value in floating point, because the sign bit is stored separately from the
mantissa).

This PR patches the bug by handling horizontal and vertical lines as a special
case, and then refactors to a design where the special-casing is no longer
necessary. It's probably easiest to read the PR one commit at a time.

Issue: LEMS-1955

## Test plan:

- `yarn dev`
- Play around with the linear graphs at `localhost:5173`

Author: benchristel

Reviewers: jeremywiebe, benchristel, handeyeco, Myranae

Required Reviewers:

Approved By: jeremywiebe

Checks: ✅ codecov/project, ✅ codecov/patch, ✅ Upload Coverage, ✅ Publish npm snapshot (ubuntu-latest, 20.x), ✅ Jest Coverage (ubuntu-latest, 20.x), ✅ Cypress (ubuntu-latest, 20.x), ✅ Check builds for changes in size (ubuntu-latest, 20.x), ✅ Lint, Typecheck, Format, and Test (ubuntu-latest, 20.x), ✅ Check for .changeset entries for all changed files (ubuntu-latest, 20.x), ✅ Publish Storybook to Chromatic (ubuntu-latest, 20.x), ✅ gerald

Pull Request URL: #1263
  • Loading branch information
benchristel committed May 14, 2024
1 parent 88c48a7 commit 1f03243
Show file tree
Hide file tree
Showing 6 changed files with 175 additions and 27 deletions.
5 changes: 5 additions & 0 deletions .changeset/giant-seals-camp.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"@khanacademy/perseus": patch
---

Fix a bug in Mafs linear graphs where the arrows on the ends of the lines would sometimes disappear or flip around
Original file line number Diff line number Diff line change
Expand Up @@ -167,8 +167,8 @@ export const shouldDrawArcOutside = (
) => {
// Create a ray from the midpoint (inside angle) to the edge of the range
const rangeIntersectionPoint = getRangeIntersectionVertex(
vertex,
midpoint,
vertex,
range,
);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -148,10 +148,10 @@ const Line = (props: LineProps) => {
if (extend) {
const trimmedRange = trimRange(range, graphDimensionsInPixels);
startExtend = extend.start
? getIntersectionOfRayWithBox(start, end, trimmedRange)
? getIntersectionOfRayWithBox(end, start, trimmedRange)
: undefined;
endExtend = extend.end
? getIntersectionOfRayWithBox(end, start, trimmedRange)
? getIntersectionOfRayWithBox(start, end, trimmedRange)
: undefined;
}

Expand Down
140 changes: 140 additions & 0 deletions packages/perseus/src/widgets/interactive-graphs/graphs/utils.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,140 @@
import {getIntersectionOfRayWithBox} from "./utils";

import type {Interval, vec} from "mafs";

describe("getIntersectionOfRayWithBox", () => {
it("returns [0, 0] when the given points are the same", () => {
const box: [Interval, Interval] = [
[-7, 7],
[-11, 11],
];
const initialPoint: vec.Vector2 = [3, 5];
const throughPoint: vec.Vector2 = [3, 5];
const intersection = getIntersectionOfRayWithBox(
initialPoint,
throughPoint,
box,
);
expect(intersection).toEqual([0, 0]);
});

test("given a horizontal ray passing through the origin to the left", () => {
const box: [Interval, Interval] = [
[-7, 7],
[-11, 11],
];
const initialPoint: vec.Vector2 = [1, 0];
const throughPoint: vec.Vector2 = [-1, 0];
const intersection = getIntersectionOfRayWithBox(
initialPoint,
throughPoint,
box,
);
expect(intersection).toEqual([-7, 0]);
});

test("given a horizontal ray passing through the origin to the right", () => {
const box: [Interval, Interval] = [
[-7, 7],
[-11, 11],
];
const initialPoint: vec.Vector2 = [-1, 0];
const throughPoint: vec.Vector2 = [1, 0];
const intersection = getIntersectionOfRayWithBox(
initialPoint,
throughPoint,
box,
);
expect(intersection).toEqual([7, 0]);
});

test("given a vertical ray passing through the origin upward", () => {
const box: [Interval, Interval] = [
[-7, 7],
[-11, 11],
];
const initialPoint: vec.Vector2 = [0, -1];
const throughPoint: vec.Vector2 = [0, 1];
const intersection = getIntersectionOfRayWithBox(
initialPoint,
throughPoint,
box,
);
expect(intersection).toEqual([0, 11]);
});

test("given a vertical ray passing through the origin downward", () => {
const box: [Interval, Interval] = [
[-7, 7],
[-11, 11],
];
const initialPoint: vec.Vector2 = [0, 1];
const throughPoint: vec.Vector2 = [0, -1];
const intersection = getIntersectionOfRayWithBox(
initialPoint,
throughPoint,
box,
);
expect(intersection).toEqual([0, -11]);
});

test("given a y coordinate of -0 for the initialPoint when the ray points right", () => {
const box: [Interval, Interval] = [
[-7, 7],
[-11, 11],
];
const initialPoint: vec.Vector2 = [-1, -0];
const throughPoint: vec.Vector2 = [1, 0];
const intersection = getIntersectionOfRayWithBox(
initialPoint,
throughPoint,
box,
);
expect(intersection).toEqual([7, 0]);
});

test("given a y coordinate of -0 for the throughPoint when the ray points right", () => {
const box: [Interval, Interval] = [
[-7, 7],
[-11, 11],
];
const initialPoint: vec.Vector2 = [-1, 0];
const throughPoint: vec.Vector2 = [1, -0];
const intersection = getIntersectionOfRayWithBox(
initialPoint,
throughPoint,
box,
);
expect(intersection).toEqual([7, 0]);
});

test("given a y coordinate of -0 for the initialPoint when the ray points left", () => {
const box: [Interval, Interval] = [
[-7, 7],
[-11, 11],
];
const initialPoint: vec.Vector2 = [1, -0];
const throughPoint: vec.Vector2 = [-1, 0];
const intersection = getIntersectionOfRayWithBox(
initialPoint,
throughPoint,
box,
);
expect(intersection).toEqual([-7, 0]);
});

test("given a y coordinate of -0 for the throughPoint when the ray points left", () => {
const box: [Interval, Interval] = [
[-7, 7],
[-11, 11],
];
const initialPoint: vec.Vector2 = [1, 0];
const throughPoint: vec.Vector2 = [-1, -0];
const intersection = getIntersectionOfRayWithBox(
initialPoint,
throughPoint,
box,
);
expect(intersection).toEqual([-7, 0]);
});
});
45 changes: 24 additions & 21 deletions packages/perseus/src/widgets/interactive-graphs/graphs/utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -18,35 +18,34 @@ export const getIntersectionOfRayWithBox = (
box: [x: Interval, y: Interval],
): [number, number] => {
const [[xMin, xMax], [yMin, yMax]] = box;
const [aX, aY] = throughPoint;
const [bX, bY] = initialPoint;
const [aX, aY] = initialPoint;
const [bX, bY] = throughPoint;

const yDiff = bY - aY;
const xDiff = bX - aX;
const slope = yDiff / xDiff;
const inverseSlope = 1 / slope;

const yAtXMin = slope * (xMin - aX) + aY;
const yAtXMax = slope * (xMax - aX) + aY;
const xAtYMin = (yMin - aY) / slope + aX;
const xAtYMax = (yMax - aY) / slope + aX;
const xExtreme = xDiff < 0 ? xMin : xMax;
const yExtreme = yDiff < 0 ? yMin : yMax;

const yAtXExtreme = aY + (xExtreme - aX) * slope;
const xAtYExtreme = aX + (yExtreme - aY) * inverseSlope;

// clock analogy to describe quadrants
switch (true) {
// 12 o'clock to 2:59
case yDiff > 0 && xDiff >= 0:
return xAtYMax > xMax ? [xMax, yAtXMax] : [xAtYMax, yMax];
// 3 o'clock to 5:59
case yDiff <= 0 && xDiff > 0:
// xAtYMin evaluates to -Infinity here, so we use absolute value
return Math.abs(xAtYMin) > xMax ? [xMax, yAtXMax] : [xAtYMin, yMin];
// 9 o'clock to 11:59
case yDiff >= 0 && xDiff < 0:
return xAtYMax < xMin ? [xMin, yAtXMin] : [xAtYMax, yMax];
// 6 o'clock to 8:59
case yDiff < 0 && xDiff <= 0:
return xAtYMin < xMin ? [xMin, yAtXMin] : [xAtYMin, yMin];
// does the ray exit the graph bounding box via the left or right edge?
case isBetween(yAtXExtreme, yMin, yMax):
return [xExtreme, yAtXExtreme];

// does the ray exit the graph bounding box via the top or bottom edge?
case isBetween(xAtYExtreme, xMin, xMax):
return [xAtYExtreme, yExtreme];

default:
return [xMax, yAtXMax];
// This default case is only reachable if the input is invalid
// (initialPoint is outside the graph bounds, or initialPoint and
// throughPoint are the same).
return [0, 0];
}
};

Expand All @@ -56,3 +55,7 @@ export const getLines = (points: readonly vec.Vector2[]): CollinearTuple[] => {
return [point, next];
});
};

function isBetween(x: number, low: number, high: number) {
return x >= low && x <= high;
}
Original file line number Diff line number Diff line change
Expand Up @@ -28,8 +28,8 @@ const LockedLine = (props: Props) => {
if (kind === "ray") {
// Rays extend to the end of the graph in one direction.
const extendedPoint = getIntersectionOfRayWithBox(
point2.coord,
point1.coord,
point2.coord,
range,
);
line = (
Expand All @@ -52,8 +52,8 @@ const LockedLine = (props: Props) => {
kind === "segment"
? point2.coord
: getIntersectionOfRayWithBox(
point2.coord,
point1.coord,
point2.coord,
range,
);
const direction = vec.sub(point2.coord, point1.coord);
Expand All @@ -70,8 +70,8 @@ const LockedLine = (props: Props) => {
kind === "segment"
? point1.coord
: getIntersectionOfRayWithBox(
point1.coord,
point2.coord,
point1.coord,
range,
);
angle = angle > 180 ? angle - 180 : angle + 180;
Expand Down

0 comments on commit 1f03243

Please sign in to comment.