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, }), }; }