-
Notifications
You must be signed in to change notification settings - Fork 351
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Angle Graph bug fixes and refactoring #1666
base: main
Are you sure you want to change the base?
Conversation
@@ -405,6 +405,86 @@ class InteractiveGraphEditor extends React.Component<Props> { | |||
/> | |||
</LabeledRow> | |||
)} | |||
{this.props.correct?.type === "angle" && ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This adds the missing options to the Angle Graph Editor for controlling "showAngles" and "allowReflexAngles"
return ( | ||
<> | ||
{/* Current equation */} | ||
<View style={styles.equationSection}> | ||
<LabelMedium>Starting equation:</LabelMedium> | ||
<BodyMonospace style={styles.equationBody}> | ||
{getAngleEquation(startCoords)} | ||
{getAngleEquation(startCoords, allowReflexAngles)} | ||
</BodyMonospace> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need the allowReflexAngles option in order to correctly calculate the angle equation for the start coordinates.
npm Snapshot: PublishedGood news!! We've packaged up the latest commit from this PR (1a10f10) and published it to npm. You 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 |
Size Change: +751 B (+0.09%) Total Size: 867 kB
ℹ️ View Unchanged
|
@@ -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"; | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Question: What would the downsides of exporting it be?
// 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 ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Praise: Thanks for making it clear this function should not be used in the future!
Question: Are angles measured incorrectly where this is still used?
} | ||
const angle = getClockwiseAngle( | ||
coords, | ||
allowReflexAngles, | ||
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@@ -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", () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Whoops missed a TODO, I'll get this updated tomorrow.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Question: Do you still need to move this test? :)
// 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 a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
// 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 = ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
? graph.showAngles | ||
: null; | ||
const allowReflexAngles = | ||
graph.type === "angle" ? graph.allowReflexAngles : null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These options needed to be added in order to update the graph previews while editing these options in the Content Editor.
4b959d6
to
7e06ffd
Compare
return false; | ||
} | ||
const angle = getClockwiseAngle(coords, allowReflexAngles); | ||
return angle; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
.
GeraldRequired Reviewers
Don't want to be involved in this pull request? Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 😜
!!this.props.correct?.showAngles | ||
} | ||
onChange={() => { | ||
if (this.props.graph?.type === "angle") { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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}
/>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
export const getAngleEquation = ( | ||
startCoords: [Coord, Coord, Coord], | ||
allowReflexAngles?: boolean, | ||
) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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);
) => { | ||
const vertex = startCoords[1]; | ||
const allowReflex = allowReflexAngles || false; | ||
const angle = getClockwiseAngle(startCoords, allowReflex); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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);
${[5, 0]} | ${[0, 0]} | ${[-5, -5]} | ${"135° angle at (0, 0)"} | ||
${[5, 0]} | ${[0, 0]} | ${[5, -5]} | ${"45° angle at (0, 0)"} | ||
${[0, 5]} | ${[0, 0]} | ${[5, 0]} | ${"90° angle at (0, 0)"} | ||
${[-5, 5]} | ${[0, 0]} | ${[5, 0]} | ${"135° angle at (0, 0)"} | ||
${[-5, -5]} | ${[0, 0]} | ${[5, 0]} | ${"135° angle at (0, 0)"} | ||
${[5, -5]} | ${[0, 0]} | ${[5, 0]} | ${"45° angle at (0, 0)"} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Question: This change is because reflexive angles are not allowed by default now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They always weren't allowed by default ... technically. Previously, when you moved the bottom arm, you could accidentally generate reflexive angles regardless of the value of the allowReflexAngles
property. With the previous validation scoring, these angles would sometimes calculate as negative angles. Generating negative angles was a major sign of this historical bug and should never have been possible. 😅
Now that we're calculating these angles correctly, we're always being provided positive values. :)
@@ -2351,7 +2352,8 @@ class InteractiveGraph extends React.Component<Props, State> { | |||
throw makeInvalidTypeError("getAngleEquationString", "angle"); | |||
} | |||
const coords = InteractiveGraph.getAngleCoords(props.graph, props); | |||
const angle = GraphUtils.findAngle(coords[2], coords[0], coords[1]); | |||
const allowReflexAngles = props.graph.allowReflexAngles || false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Question: Do you still need this after updating the default to false from Anakaren's suggestion?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't, great catch! Thank you
it("should correctly calculate the angle for the given coordinates", () => { | ||
const point1 = [2, 2] as vec.Vector2; | ||
const point2 = [2, 0] as vec.Vector2; | ||
const vertex = [0, 0] as vec.Vector2; | ||
expect(findAngle(point1, point2, vertex)).toBe(45); | ||
const coords: [Coord, Coord, Coord] = [point1, vertex, point2]; | ||
const allowReflexAngles = false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Question: Does this still need to be defined after the change Anakaren suggested where the default is false? Haven't used defaults much, but I think it should just be false if the value is undefined, right? I could see it just being more clear to state it outright in a test, but just wanted to check :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No it doesn't, I think you're right!
@@ -256,19 +257,16 @@ function interactiveGraphValidator( | |||
) { | |||
const guess = userInput.coords; | |||
const correct = rubric.correct.coords; | |||
const allowReflexAngles = rubric.correct.allowReflexAngles || false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comment: Another place where you might not need to have || false
if (!coords) { | ||
return false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Question: Is there a validator test that covers this response? I can see there's one for answering invalid if guess.coords is missing; does that cover this?
findAngle(updated.coords[0], updated.coords[2], updated.coords[1]), | ||
getClockwiseAngle( | ||
updated.coords, | ||
updated.allowReflexAngles || false, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comment: Can possibly be shortened. Apologies if you can't actually shorten this and I just keep pointing out out 😅 Maybe just do a search for this line to find the other places if it can be updated?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for finding these!! I only updated the content editor function, and missed doing it on the getClockwiseAngle method itself. I've updated them all now, and it's much cleaner!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Truly beautiful work, Third. Thank you so much for your dedication to getting this right and to cleaning up all the loose odds and ends! Left comments about cleaning some stuff up, adding a test maybe, and just some random questions, but nothing blocking :)
5f748f1
to
86fef16
Compare
if ( | ||
this.props.graph?.type === "angle" && | ||
this.props.correct.type === "angle" | ||
) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For this checkbox, we use an if
statement to check the graph types; in the other one, we use an invariant
assertion. Is there a reason to prefer one pattern over the other in each case? Could we use the same pattern for both?
FWIW I prefer the invariant because it's more explicit about what's expected/possible, and there's less nesting of curly braces.
<InfoTip> | ||
<p> | ||
Allow students to be able to create | ||
reflexive angles. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the term is "reflex angle". At least, that's what all the results on DuckDuckGo are telling me.
reflexive angles. | |
reflex angles. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ooo thank you very much! I'll make sure to fix all these references.
<Checkbox | ||
label={ | ||
<LabelSmall> | ||
Allow reflexive angles |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Allow reflexive angles | |
Allow reflex angles |
coordsCopy[rel(a)], | ||
coordsCopy[rel(b)], | ||
coordsCopy[rel(vertex)], | ||
const allowReflexAngles = false; // Polygons do not have reflex angles |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure this is right — polygons can have reflex angles if they're concave. But it's not clear to me how this getAngle
function is being used. I don't understand what innerAngles
(below) is for. So this code might be doing the right thing.
What are the consequences of setting allowReflexAngles
to true vs. false here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can confirm that the polygon graph works identically to current prod, and that the reflexAngle setting does not appear to affect this either way. I've added a video to help illustrate!
However there IS a pre-existing weird bug in the original reducer logic that might be worth a separate ticket. 🤔 It seems like we sometimes switch between inner and outer angles. @Myranae might know more about this.
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:
Video Example:
Screen.Recording.2024-09-26.at.3.44.49.PM.mov
Issue: LEMS-2302
Test plan: