Angle Graph bug fixes and refactoring #1666

SonicScrewdriver · 2024-09-25T00:16:13Z

Summary:

This ticket includes several fixes to the angle graph in terms of the correct calculation of the angle, particularly when dealing with reflexive angles. A short summary of the changes will be listed below:

Addition of key (missing) Angle Graph features to the Content Editor
- This was required to perform the necessary bug fixes below
- Content Editors can now properly handle whether or not to show the angles, and whether to allow reflexive angles.
Consolidation of findAngle logic into a single new function called getAngleFromVertex
- We had several copies of this function across our code, and 99% of use cases were misusing the parameters as written.
- Opted to remove the triple point option, as that should be calculated by taking in the graph reflexivity setting in mind. This resulted in a cleaner function.
Creation of a new shared function called getClockwiseAngle, which can properly handle angle reflexivity.
- This can be used to correctly calculate the angle between any three points, depending on whether the graph is allowed to be reflexive.
Rename of old, buggy findAngle function in graphie to findAngleDeprecated.
- This can be deleted when the legacy graph code is removed.
- Additional research was done across all Khan repositories to ensure that there are no external references to this function that might regress as a result of the rename.
Fixed bugs related to angle equations for both the Correct Answer and Start Coordinates in the Content Editor
- Angle equations will now correctly calculate the angle positions regardless of reflexivity
Minor update to Polygon graph
- Removed some duplicate code and updated one findAngle usage to use the new getClockwiseAngle function.

Video Example:

Screen.Recording.2024-09-26.at.3.44.49.PM.mov

Issue: LEMS-2302

Test plan:

yarn jest
Extensive manual testing in storybook and flipbook
Additional testing with @Myranae in regards to the Polygon Graph
Further testing in webapp using snapshot and live content. Ensured to test twice so that I could test each arm position.

SonicScrewdriver · 2024-09-25T00:17:10Z

packages/perseus-editor/src/widgets/interactive-graph-editor/interactive-graph-editor.tsx

@@ -405,6 +405,86 @@ class InteractiveGraphEditor extends React.Component<Props> {
                        />
                    </LabeledRow>
                )}
+                {this.props.correct?.type === "angle" && (


This adds the missing options to the Angle Graph Editor for controlling "showAngles" and "allowReflexAngles"

SonicScrewdriver · 2024-09-25T00:17:46Z

...ages/perseus-editor/src/widgets/interactive-graph-editor/start-coords/start-coords-angle.tsx

    return (
        <>
            {/* Current equation */}
            <View style={styles.equationSection}>
                <LabelMedium>Starting equation:</LabelMedium>
                <BodyMonospace style={styles.equationBody}>
-                    {getAngleEquation(startCoords)}
+                    {getAngleEquation(startCoords, allowReflexAngles)}
                </BodyMonospace>


We need the allowReflexAngles option in order to correctly calculate the angle equation for the start coordinates.

github-actions · 2024-09-25T00:18:35Z

npm Snapshot: Published

Good news!! We've packaged up the latest commit from this PR (fe0bef1) and published it to npm. You
can install it using the tag PR1666.

Example:

yarn add @khanacademy/perseus@PR1666

If you are working in Khan Academy's webapp, you can run:

./dev/tools/bump_perseus_version.sh -t PR1666

github-actions · 2024-09-25T00:18:43Z

Size Change: +785 B (+0.09%)

Total Size: 866 kB

Filename	Size	Change
`packages/perseus-editor/dist/es/index.js`	280 kB	+188 B (+0.07%)
`packages/perseus/dist/es/index.js`	419 kB	+597 B (+0.14%)

ℹ️ View Unchanged

Filename	Size
`packages/kas/dist/es/index.js`	38.3 kB
`packages/keypad-context/dist/es/index.js`	760 B
`packages/kmath/dist/es/index.js`	4.27 kB
`packages/math-input/dist/es/index.js`	78 kB
`packages/math-input/dist/es/strings.js`	1.79 kB
`packages/perseus-core/dist/es/index.js`	1.48 kB
`packages/perseus-linter/dist/es/index.js`	22.2 kB
`packages/perseus/dist/es/strings.js`	3.4 kB
`packages/pure-markdown/dist/es/index.js`	3.66 kB
`packages/simple-markdown/dist/es/index.js`	12.4 kB

_{compressed-size-action}

SonicScrewdriver · 2024-09-25T00:19:08Z

packages/perseus/src/index.ts

@@ -125,6 +125,9 @@ export {
    getQuadraticCoords,
    getAngleCoords,
 } from "./widgets/interactive-graphs/reducer/initialize-graph-state";
+// This export is to support necessary functionality in the perseus-editor package.
+// It should be removed if widgets and editors become colocated.
+export {getClockwiseAngle} from "./widgets/interactive-graphs/math";



After reviewing kmath and some other possible locations, it seemed best to simply export this. I feel like this is a relatively safe function to export and is using the same approach as the getAngleCoords.

I'm happy to take any feedback on this however!

SonicScrewdriver · 2024-09-25T00:20:20Z

packages/perseus/src/util/graphie.ts

+    // This function is deprecated as it has several issues calculating
+    // correctly when dealing with reflexive angles or the position of point2
+    // (LEMS-2202) Remove this function while removing the Legacy Interactive Graph.
+    findAngleDeprecated: function (


I can add a comment to LEMS-2202 to mention deleting this function once we strip out the Legacy Graph. First, I need to confirm if interactive.ts is used anywhere else as it's the only other consumer of this function.

SonicScrewdriver · 2024-09-25T00:22:44Z

packages/perseus/src/widgets/interactive-graph.tsx

+                        }
+                        const angle = getClockwiseAngle(
+                            coords,
+                            allowReflexAngles,
                        );


While there aren't any graphs that currently make use of the reflexive angles, this update in validation logic is necessary to make sure that we can support them properly moving forward.

I think it is safe to update the validator at this time as the Legacy Angle Graph is no longer being shown to users.

SonicScrewdriver · 2024-09-25T00:23:18Z

packages/perseus/src/widgets/interactive-graphs/graphs/components/angle-indicators.test.ts

@@ -94,10 +95,13 @@ describe("shouldDrawArcOutside", () => {
        ).toBe(true);
    });

+    // TODO: (third) Move this test to the math package
    it("should correctly calculate the angle for the given coordinates", () => {


Whoops missed a TODO, I'll get this updated tomorrow.

SonicScrewdriver · 2024-09-25T00:25:37Z

packages/perseus/src/widgets/interactive-graphs/math/angles.ts

+// This function calculates the angle between two points and an optional vertex.
+// If the vertex is not provided, the angle is measured between the two points.
+// This does not account for reflex angles or clockwise position.
+export const findAngleFromVertex = (


There was only one case I could find where we were actually making use of all 3 points for findAngle, which was the polygon graph.

In order to simplify these functions, I've opted to scope this function to only calculate the angle from a single point and the vertex. I have updated the polygon graph to use the new getClockwiseAngle method below, which is able to be shared in several other places.

SonicScrewdriver · 2024-09-25T00:27:29Z

packages/perseus/src/widgets/interactive-graphs/math/angles.ts

+// This function calculates the clockwise angle between three points,
+// and is used to generate the labels and equation strings of the
+// current angle for the interactive graph.
+export const getClockwiseAngle = (


I should note that this is also used in the validator now, in order to preemptively solve issues with calculating the angles of reflexive graphs.

SonicScrewdriver · 2024-09-25T00:29:32Z

packages/perseus/src/widgets/interactive-graphs/stateful-mafs-graph.tsx

+            ? graph.showAngles
+            : null;
+    const allowReflexAngles =
+        graph.type === "angle" ? graph.allowReflexAngles : null;


These options needed to be added in order to update the graph previews while editing these options in the Content Editor.

SonicScrewdriver · 2024-10-01T19:50:44Z

packages/perseus/src/widgets/interactive-graphs/interactive-graph-validator.ts

+                        return false;
+                    }
+                    const angle = getClockwiseAngle(coords, allowReflexAngles);
+                    return angle;


While I generally tried to avoid updating the validation logic, I think this is now necessary in order to properly score reflexive/non-reflexive graphs in the future. This will provide a much more stable and consistent experience for our users.

EXAMPLE
To provide an example of the issue being solved here: Previously, the legacy graph would incorrectly become "reflexive" if you dragged the bottom ray (point2) down, even if reflexive angles were turned off. This has been a long-standing bug with our graphs. As a result, legacy graphs could only be scored correct unidirectionally. Furthermore, there were several issues related to how reflexive angles were calculated and often resulted in negative angles.

In our modern re-implementation, these issues have already been fixed visually. Now, a graph's ability to become reflexive is wholly controlled by the allowReflexAngles property. As a result, the graph should be scored correctly regardless of the clockwise order of the points. ie. [0,1] [0,0] [1,0] and [1,0][0,0][0,1] both create a 90* angle for a graph where allowReflexAngles=false.

khan-actions-bot · 2024-10-01T20:18:24Z

Gerald

Required Reviewers

@Khan/perseus for changes to .changeset/angry-crews-care.md, packages/perseus/src/index.ts, packages/perseus/src/util/graphie.ts, packages/perseus/src/util/interactive.ts, packages/perseus/src/widgets/interactive-graph.tsx, packages/perseus/src/widgets/interactive-graphs/interactive-graph-validator.ts, packages/perseus/src/widgets/interactive-graphs/stateful-mafs-graph.tsx, packages/perseus-editor/src/widgets/interactive-graph-editor/interactive-graph-editor.tsx, packages/perseus/src/widgets/interactive-graphs/math/angles.ts, packages/perseus/src/widgets/interactive-graphs/math/index.ts, packages/perseus/src/widgets/interactive-graphs/reducer/interactive-graph-reducer.test.ts, packages/perseus/src/widgets/interactive-graphs/reducer/interactive-graph-reducer.ts, packages/perseus-editor/src/widgets/interactive-graph-editor/start-coords/start-coords-angle.tsx, packages/perseus-editor/src/widgets/interactive-graph-editor/start-coords/start-coords-settings.tsx, packages/perseus-editor/src/widgets/interactive-graph-editor/start-coords/util.test.ts, packages/perseus-editor/src/widgets/interactive-graph-editor/start-coords/util.ts, packages/perseus/src/widgets/interactive-graphs/graphs/components/angle-indicators.test.ts, packages/perseus/src/widgets/interactive-graphs/graphs/components/angle-indicators.tsx

Don't want to be involved in this pull request? Comment #removeme and we won't notify you of further changes.

anakaren-rojas

Overall lgtm - very fun to play with, you did a great job!
Since I'm not as familiar with this feature though, I will leave the approving the those that are 😜

anakaren-rojas · 2024-10-03T17:54:05Z

packages/perseus-editor/src/widgets/interactive-graph-editor/interactive-graph-editor.tsx

+                                    !!this.props.correct?.showAngles
+                                }
+                                onChange={() => {
+                                    if (this.props.graph?.type === "angle") {


nit since this seems to be the pattern in the file:
could the logic for the checkboxes be extracted into their own methods?

handleAngleChange = () => { const { graph, correct, onChange } = this.props; if (graph?.type !== "angle") return; invariant( correct.type === "angle", `Expected graph type to be angle, but got ${correct.type}`, ); onChange({ correct: { ...correct, showAngles: !correct.showAngles, }, graph: { ...graph, showAngles: !graph.showAngles, }, }); }; // then in the checkbox <Checkbox ... onChange={this.handleAngleChange} />

Ooo good callout! They definitely could be separated out, but I think this PR is probably already large and long-lived enough as it is.😅

That sounds like a good clean up ticket for Interactive Graph's backlog however! It seems like this is the existing design pattern in this file, and we should probably update all of the checkbox logic together in order to maintain consistency.

anakaren-rojas · 2024-10-03T18:05:51Z

packages/perseus-editor/src/widgets/interactive-graph-editor/start-coords/util.ts

+export const getAngleEquation = (
+    startCoords: [Coord, Coord, Coord],
+    allowReflexAngles?: boolean,
+) => {


another nit suggestion: adding the default param vs using an ||, you can remove the declaration of allowReflex

- allowReflexAngles?: boolean, + allowReflexAngles: boolean = false, .... - const allowReflex = allowReflexAngles || false; - const angle = getClockwiseAngle(startCoords, allowReflex); + const angle = getClockwiseAngle(startCoords, allowReflexAngles);

anakaren-rojas · 2024-10-03T18:08:07Z

packages/perseus-editor/src/widgets/interactive-graph-editor/start-coords/util.ts

+) => {
+    const vertex = startCoords[1];
+    const allowReflex = allowReflexAngles || false;
+    const angle = getClockwiseAngle(startCoords, allowReflex);


also chaining might be helpful here, although idk how much that would improve readability

- const angle = getClockwiseAngle(startCoords, allowReflex); - const roundedAngle = angle.toFixed(0); + const roundedAngle = getClockwiseAngle(startCoords, allowReflex).toFixed(0);

SonicScrewdriver self-assigned this Sep 25, 2024

SonicScrewdriver commented Sep 25, 2024

View reviewed changes

SonicScrewdriver changed the title ~~Refactoring of all angle methods to be as dry and reusable as possible~~ Angle Graph bug fixes and refactoring Sep 26, 2024

SonicScrewdriver added 6 commits October 1, 2024 10:42

Initial work / prototype before switching tickets

d165d51

Refactoring of all angle methods to be as dry and reusable as possible

27b3de6

More consistent naming

43be051

Prettier

b2fb15d

Updating tests

4d7306b

Fixing a miss during the rebase

7e06ffd

SonicScrewdriver force-pushed the angle-equation-refactor branch from 4b959d6 to 7e06ffd Compare October 1, 2024 18:12

SonicScrewdriver commented Oct 1, 2024

View reviewed changes

SonicScrewdriver marked this pull request as ready for review October 1, 2024 20:18

khan-actions-bot requested a review from a team October 1, 2024 20:18

SonicScrewdriver requested review from nishasy, mark-fitzgerald and Myranae October 1, 2024 20:18

anakaren-rojas reviewed Oct 3, 2024

View reviewed changes

PR Feedback

fe0bef1

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Angle Graph bug fixes and refactoring #1666

Angle Graph bug fixes and refactoring #1666

SonicScrewdriver commented Sep 25, 2024 •

edited

Loading

SonicScrewdriver Sep 25, 2024

SonicScrewdriver Sep 25, 2024

github-actions bot commented Sep 25, 2024 •

edited

Loading

github-actions bot commented Sep 25, 2024 •

edited

Loading

SonicScrewdriver Sep 25, 2024

SonicScrewdriver Sep 25, 2024

SonicScrewdriver Sep 25, 2024

SonicScrewdriver Sep 25, 2024

SonicScrewdriver Sep 25, 2024

SonicScrewdriver Sep 25, 2024

SonicScrewdriver Sep 25, 2024

SonicScrewdriver Oct 1, 2024 •

edited

Loading

khan-actions-bot commented Oct 1, 2024

anakaren-rojas left a comment

anakaren-rojas Oct 3, 2024

SonicScrewdriver Oct 3, 2024

anakaren-rojas Oct 3, 2024

anakaren-rojas Oct 3, 2024

Angle Graph bug fixes and refactoring #1666

Are you sure you want to change the base?

Angle Graph bug fixes and refactoring #1666

Conversation

SonicScrewdriver commented Sep 25, 2024 • edited Loading

Summary:

Video Example:

Test plan:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Sep 25, 2024 • edited Loading

npm Snapshot: Published

github-actions bot commented Sep 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SonicScrewdriver Oct 1, 2024 • edited Loading

Choose a reason for hiding this comment

khan-actions-bot commented Oct 1, 2024

Gerald

anakaren-rojas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SonicScrewdriver commented Sep 25, 2024 •

edited

Loading

github-actions bot commented Sep 25, 2024 •

edited

Loading

github-actions bot commented Sep 25, 2024 •

edited

Loading

SonicScrewdriver Oct 1, 2024 •

edited

Loading