From c6c5064da1f9e6a18c4cc49be073a198bcfb3be8 Mon Sep 17 00:00:00 2001 From: Sarah Third Date: Wed, 19 Jun 2024 11:55:23 -0700 Subject: [PATCH] Interactive Graph Bug Fix: Ensure that we're bounding and snapping BEFORE checking if the new destination results in a valid graph. (#1356) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Summary: This is a fix for an issue in our Sinusoid graphs, where moving the points to the edge of the graph would cause the entire browser to crash. I believe that this was due the fact that we were bounding and snapping our points to the graph settings AFTER determining if the newDestination is a valid location, which means that points placed at the edge of the graph could end up in an invalid location. This made it possible to get the graph into a bit of an infinite state. By moving the boundAndSnap logic prior to checking whether it is a valid location, we can make sure that this loop is impossible. Repeating this order of operations for the Quadratic Graph actually manages to solve a separate issue that was considered a "Won't Do" earlier! :) Issue: LEMS-2064 & LEMS-2066 ## Test plan: - Manual testing in webapp (local dev) to confirm bug is reproducible - Manual testing with PR snapshot in webapp to confirm bug is solved. Author: SonicScrewdriver Reviewers: nishasy, benchristel, #perseus Required Reviewers: Approved By: nishasy Checks: ✅ codecov/project, ✅ codecov/patch, ✅ Upload Coverage (ubuntu-latest, 20.x), ✅ Publish npm snapshot (ubuntu-latest, 20.x), ✅ Lint, Typecheck, Format, and Test (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), ✅ Publish Storybook to Chromatic (ubuntu-latest, 20.x), ✅ Check for .changeset entries for all changed files (ubuntu-latest, 20.x), ✅ gerald Pull Request URL: https://github.com/Khan/perseus/pull/1356 --- .changeset/smooth-worms-wave.md | 5 ++++ .../graphs/sinusoid.test.ts | 17 +++++++++-- .../interactive-graphs/graphs/sinusoid.tsx | 30 ++++++++++++++++--- .../reducer/interactive-graph-reducer.test.ts | 19 ++++++++++++ .../reducer/interactive-graph-reducer.ts | 22 ++++++++++---- 5 files changed, 81 insertions(+), 12 deletions(-) create mode 100644 .changeset/smooth-worms-wave.md diff --git a/.changeset/smooth-worms-wave.md b/.changeset/smooth-worms-wave.md new file mode 100644 index 0000000000..b00f4ed35c --- /dev/null +++ b/.changeset/smooth-worms-wave.md @@ -0,0 +1,5 @@ +--- +"@khanacademy/perseus": patch +--- + +A fix for performance issues related to Sinusoid and Quadratic graphs diff --git a/packages/perseus/src/widgets/interactive-graphs/graphs/sinusoid.test.ts b/packages/perseus/src/widgets/interactive-graphs/graphs/sinusoid.test.ts index 2f1266ea6f..7881be21a8 100644 --- a/packages/perseus/src/widgets/interactive-graphs/graphs/sinusoid.test.ts +++ b/packages/perseus/src/widgets/interactive-graphs/graphs/sinusoid.test.ts @@ -18,16 +18,29 @@ describe("SinusoidGraph", () => { expect(getSinusoidCoefficients(coords)).toEqual(expected); }); - it("should accurately calculate the sine wave", () => { + it("should accurately calculate the sine wave for a given x-coordinate", () => { const coords: SinusoidGraphState["coords"] = [ [0, 0], [2, 2], ]; + + // Ensure that the coefficients are defined const coefficients = getSinusoidCoefficients(coords); + expect(coefficients).toBeDefined(); + // Grab a point where the sine wave should be 0 const pointToTest = coords[0][0] + 4; // The sine wave should be roughly 0 at this point when accounting for floating point errors - expect(Math.round(computeSine(pointToTest, coefficients))).toEqual(0); + // We already know that the coefficients are defined from the previous test + expect(Math.round(computeSine(pointToTest, coefficients!))).toEqual(0); + }); + + it("should return undefined when the coefficients are invalid", () => { + const coords: SinusoidGraphState["coords"] = [ + [0, 0], + [0, 0], + ]; + expect(getSinusoidCoefficients(coords)).toBe(undefined); }); }); diff --git a/packages/perseus/src/widgets/interactive-graphs/graphs/sinusoid.tsx b/packages/perseus/src/widgets/interactive-graphs/graphs/sinusoid.tsx index dc332d0e05..0419d1f53f 100644 --- a/packages/perseus/src/widgets/interactive-graphs/graphs/sinusoid.tsx +++ b/packages/perseus/src/widgets/interactive-graphs/graphs/sinusoid.tsx @@ -26,12 +26,29 @@ export function SinusoidGraph(props: SinusoidGraphProps) { // The coords[0] is the root and the coords[1] is the first peak const {coords} = graphState; - // Get the coefficients for calculating the quadratic equation - const coeffs: SineCoefficient = getSinusoidCoefficients(coords); + // The coefficients are used to calculate the sinusoid equation, plot the graph, and to indicate + // to content creators the currently selected "correct answer" in the Content Editor. + // While we should technically never have invalid coordinates, we want to ensure that + // we have a fallback so that the graph can still be plotted without crashing. + const coeffRef = React.useRef({ + amplitude: 1, + angularFrequency: 1, + phase: 1, + verticalOffset: 0, + }); + const coeffs = getSinusoidCoefficients(coords); + + // If the coefficients are valid, update the reference + if (coeffs !== undefined) { + coeffRef.current = coeffs; + } return ( <> - computeSine(x, coeffs)} color={color.blue} /> + computeSine(x, coeffRef.current)} + color={color.blue} + /> {coords.map((coord, i) => ( , -): SineCoefficient => { +): SineCoefficient | undefined => { // It's assumed that p1 is the root and p2 is the first peak const p1 = coords[0]; const p2 = coords[1]; + // If the x-coordinates are the same, we are unable to calculate the coefficients + if (p2[0] === p1[0]) { + return; + } + // Resulting coefficients are canonical for this sine curve const amplitude = p2[1] - p1[1]; const angularFrequency = Math.PI / (2 * (p2[0] - p1[0])); diff --git a/packages/perseus/src/widgets/interactive-graphs/reducer/interactive-graph-reducer.test.ts b/packages/perseus/src/widgets/interactive-graphs/reducer/interactive-graph-reducer.test.ts index fa52496549..6c54ee9a14 100644 --- a/packages/perseus/src/widgets/interactive-graphs/reducer/interactive-graph-reducer.test.ts +++ b/packages/perseus/src/widgets/interactive-graphs/reducer/interactive-graph-reducer.test.ts @@ -182,6 +182,25 @@ describe("moveControlPoint", () => { ]); }); + it("does not allow moving an endpoint of a sinusoid if the bounding logic would result in an invalid graph", () => { + const state: InteractiveGraphState = { + ...baseSinusoidGraphState, + coords: [ + [9, 1], + [10, 2], + ], + }; + + const updated = interactiveGraphReducer(state, movePoint(0, [15, 1])); + + invariant(updated.type === "sinusoid"); + // Assert: the move was canceled + expect(updated.coords).toEqual([ + [9, 1], + [10, 2], + ]); + }); + it("snaps points to the snap grid", () => { const state: InteractiveGraphState = { ...baseSegmentGraphState, diff --git a/packages/perseus/src/widgets/interactive-graphs/reducer/interactive-graph-reducer.ts b/packages/perseus/src/widgets/interactive-graphs/reducer/interactive-graph-reducer.ts index 37a988e703..6f5dee3205 100644 --- a/packages/perseus/src/widgets/interactive-graphs/reducer/interactive-graph-reducer.ts +++ b/packages/perseus/src/widgets/interactive-graphs/reducer/interactive-graph-reducer.ts @@ -286,28 +286,38 @@ function doMovePoint( }; } case "sinusoid": { - // First, we need to verify that the new coordinates are not on the same vertical line - // If they are, we don't want to move the point + // First, we need to make sure to bound the new coordinates to the graph range const destination = action.destination; + const boundDestination = boundAndSnapToGrid(destination, state); + + // Then, we need to verify that the new coordinates are not on the same + // vertical line. If they are, then we don't want to move the point const newCoords: vec.Vector2[] = [...state.coords]; - newCoords[action.index] = action.destination; + newCoords[action.index] = boundDestination; if (newCoords[0][0] === newCoords[1][0]) { return state; } + return { ...state, hasBeenInteractedWith: true, coords: setAtIndex({ array: state.coords, index: action.index, - newValue: boundAndSnapToGrid(destination, state), + newValue: boundDestination, }), }; } case "quadratic": { // Set up the new coords and check if the quadratic coefficients are valid const newCoords: QuadraticCoords = [...state.coords]; - newCoords[action.index] = action.destination; + + // Bind the new destination to the graph range/snapStep and then get the quadratic coefficients + const boundDestination = boundAndSnapToGrid( + action.destination, + state, + ); + newCoords[action.index] = boundDestination; const QuadraticCoefficients = getQuadraticCoefficients(newCoords); // If the new destination results in an invalid quadratic equation, we don't want to move the point @@ -321,7 +331,7 @@ function doMovePoint( coords: setAtIndex({ array: state.coords, index: action.index, - newValue: boundAndSnapToGrid(action.destination, state), + newValue: boundDestination, }), }; }