[Polygon] Remove duplicate points when determining if a polygon can be closed #1978

nishasy · 2024-12-11T01:37:03Z

Summary:

Right now, if a user were trying to close the polygon by clicking the last point, missed, created a new point instead as a result, and then tried to drag the new point to the first point to close out the polygon, the question would be marked wrong even if the polygon looks right.

Relatedly, if a user were to drag a middle point over another point in the polygon, even if the polygon looks right, it would be marked wrong.

To fix both of these issues, I'm making it so

the close button is only enabled if there are three or more unique points in the polygon
closing the polygon removes any duplicate points from the coords array. This allows the dragging to work more intuitively and the question to be graded fairly (based on how the polygon looks regardless of duplicate points)

Issue: https://khanacademy.atlassian.net/browse/LEMS-2711
(and also https://khanacademy.atlassian.net/browse/LEMS-2722)

Test plan:

yarn jest packages/perseus/src/widgets/interactive-graphs/graphs/utils.test.ts
yarn jest packages/perseus/src/widgets/interactive-graphs/mafs-graph.test.tsx
yarn jest packages/perseus/src/widgets/interactive-graphs/reducer/interactive-graph-reducer.test.ts

Storybook

Go to http://localhost:6006/?path=/story/perseus-widgets-interactive-graph--unlimited-polygon-with-mafs
Put down four points
Click to create a fifth point
drag the fifth point over to the first one
close the shape with the button
re-open the shape
confirm that the previous fifth point is now gone
move a middle point over another middle point
close and reopen the shape
confirm that the duplicate point is gone
get to a state of three points
move one point over another so it looks like there are two points
confirm that the close button is disabled

Before:

Screen.Recording.2024-12-12.at.2.21.03.PM.mov

After:

Screen.Recording.2024-12-12.at.2.21.45.PM.mov

…oint

github-actions · 2024-12-11T01:39:48Z

Size Change: +261 B (+0.02%)

Total Size: 1.27 MB

Filename	Size	Change
`packages/perseus/dist/es/index.js`	416 kB	+261 B (+0.06%)

ℹ️ View Unchanged

Filename	Size
`packages/kas/dist/es/index.js`	39 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-editor/dist/es/index.js`	688 kB
`packages/perseus-linter/dist/es/index.js`	22.2 kB
`packages/perseus/dist/es/strings.js`	4.12 kB
`packages/pure-markdown/dist/es/index.js`	3.67 kB
`packages/simple-markdown/dist/es/index.js`	12.5 kB

_{compressed-size-action}

…ick the button.

github-actions · 2024-12-11T20:08:42Z

npm Snapshot: Published

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

Example:

yarn add @khanacademy/perseus@PR1978

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

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

catandthemachines

Approved! Tested it out and it looks so sharp and really solves a lot of additional edge cases that make the polygon really great!

Added a comment about a way to condense the duplication function and make it more efficient but we should be good to go!

catandthemachines · 2024-12-11T01:47:15Z

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

@@ -493,6 +493,23 @@ function doMovePoint(
                return state;
            }

+            // If the last point is being moved to the same position as
+            // the first point, close the polygon.
+            if (


Love this! If you could add this test case to our interactive-graph-reducer.test.ts that would be perfect!

I think I ended up removing this code because this function should only be marking the polygon as closed? I noticed in the tests that the polygon's state changed to close even if it had an empty coords array, so I removed these lines to stay consistent with that behavior.

catandthemachines · 2024-12-13T01:04:53Z

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

@@ -200,8 +201,15 @@ function doClickPoint(

 function doClosePolygon(state: InteractiveGraphState): InteractiveGraphState {
    if (isUnlimitedGraphState(state) && state.type === "polygon") {
+        // We want to remove any duplicate points when closing the polygon to


Love these comments!

catandthemachines · 2024-12-13T01:26:03Z

packages/perseus/src/widgets/interactive-graphs/graphs/utils.ts

@@ -44,3 +45,20 @@ export const getIntersectionOfRayWithBox = (
 function isBetween(x: number, low: number, high: number) {
    return x >= low && x <= high;
 }
+
+export function getArrayWithoutDuplicates(array: Array<Coord>): Array<Coord> {
+    const returnArray: Array<Coord> = [];


Might I suggest using the spread operator or the filter method to remove the duplicates. I think they have more efficiencies built in that will make this run with higher performance.

https://medium.com/@collettemathieu/what-is-the-fastest-way-to-remove-duplicates-from-an-array-in-javascript-9e5b4d3f55e1

Additional caviat, our arrays will likely never be more than 20 points long if that. So we don't necessarily need to worry about it. But it would be nicer to have this code a little shorter and I think this article can help with it.

I was going to just create a set similar to that, but my understanding is that that doesn't work for array whose elements are also arrays or objects, because it doesn't consider two separate arrays equivalent even if their elements are the same.

@somewhatabstract

This PR was opened by the [Changesets release](https://github.com/changesets/action) GitHub action. When you're ready to do a release, you can merge this and the packages will be published to npm automatically. If you're not ready to do a release yet, that's fine, whenever you add more changesets to main, this PR will be updated. # Releases ## @khanacademy/[email protected] ### Patch Changes - [#2014](#2014) [`763d2d0f1`](763d2d0) Thanks [@somewhatabstract](https://github.com/somewhatabstract)! - Migrate off deprecated ID generation APIs - [#2009](#2009) [`b09d19b7b`](b09d19b) Thanks [@catandthemachines](https://github.com/catandthemachines)! - Fixing bug in creating concave shapes in unlimited polygons. - [#1978](#1978) [`81632c326`](81632c3) Thanks [@nishasy](https://github.com/nishasy)! - [Polygon] Remove duplicate points when determining if a polygon can be closed - [#1999](#1999) [`278527b08`](278527b) Thanks [@catandthemachines](https://github.com/catandthemachines)! - Fixing open polygon scoring issues within exercises and editors. ## @khanacademy/[email protected] ### Patch Changes - [#2014](#2014) [`763d2d0f1`](763d2d0) Thanks [@somewhatabstract](https://github.com/somewhatabstract)! - Migrate off deprecated ID generation APIs - [#1978](#1978) [`81632c326`](81632c3) Thanks [@nishasy](https://github.com/nishasy)! - [Polygon] Remove duplicate points when determining if a polygon can be closed - [#1999](#1999) [`278527b08`](278527b) Thanks [@catandthemachines](https://github.com/catandthemachines)! - Fixing open polygon scoring issues within exercises and editors. - Updated dependencies \[[`763d2d0f1`](763d2d0), [`b09d19b7b`](b09d19b), [`81632c326`](81632c3), [`278527b08`](278527b)]: - @khanacademy/[email protected] ## @khanacademy/[email protected] ### Patch Changes - [#2014](#2014) [`763d2d0f1`](763d2d0) Thanks [@somewhatabstract](https://github.com/somewhatabstract)! - Migrate off deprecated ID generation APIs

[Polygon] Close polygon if last point is moved to overlap the first p…

bfb7b67

…oint

nishasy self-assigned this Dec 11, 2024

Change the behavior so that it doesn't auto-close. The user has to cl…

f0413e4

…ick the button.

nishasy added 2 commits December 11, 2024 16:53

fix issue with duped points

bdd1702

Merge branch 'main' into polygon-move-last-point

2ba1958

nishasy changed the title ~~[Polygon] Close polygon if last point is moved to overlap the first point~~ [Polygon] Remove duplicate points when determining if a polygon can be closed Dec 12, 2024

nishasy added 4 commits December 12, 2024 13:14

Fix changeset and lint failures

681c667

rename stuff

8aaf423

Add tests

04a8264

prettier

f3f4663

nishasy marked this pull request as ready for review December 12, 2024 22:24

nishasy requested review from catandthemachines, anakaren-rojas and a team December 12, 2024 22:24

catandthemachines approved these changes Dec 13, 2024

View reviewed changes

nishasy added 3 commits December 13, 2024 16:17

Merge branch 'main' into polygon-move-last-point

284e60f

Merge branch 'main' into polygon-move-last-point

062762a

fix from merge

0cc8c19

nishasy merged commit 81632c3 into main Dec 16, 2024
8 checks passed

nishasy deleted the polygon-move-last-point branch December 16, 2024 19:10

khan-actions-bot mentioned this pull request Dec 16, 2024

Version Packages #2013

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Polygon] Remove duplicate points when determining if a polygon can be closed #1978

[Polygon] Remove duplicate points when determining if a polygon can be closed #1978

nishasy commented Dec 11, 2024 •

edited

Loading

github-actions bot commented Dec 11, 2024 •

edited

Loading

github-actions bot commented Dec 11, 2024 •

edited

Loading

catandthemachines left a comment

catandthemachines Dec 11, 2024

nishasy Dec 14, 2024

catandthemachines Dec 13, 2024

catandthemachines Dec 13, 2024

catandthemachines Dec 13, 2024

nishasy Dec 14, 2024

[Polygon] Remove duplicate points when determining if a polygon can be closed #1978

[Polygon] Remove duplicate points when determining if a polygon can be closed #1978

Conversation

nishasy commented Dec 11, 2024 • edited Loading

Summary:

Test plan:

Before:

After:

github-actions bot commented Dec 11, 2024 • edited Loading

github-actions bot commented Dec 11, 2024 • edited Loading

npm Snapshot: Published

catandthemachines left a comment

Choose a reason for hiding this comment

catandthemachines Dec 11, 2024

Choose a reason for hiding this comment

nishasy Dec 14, 2024

Choose a reason for hiding this comment

catandthemachines Dec 13, 2024

Choose a reason for hiding this comment

catandthemachines Dec 13, 2024

Choose a reason for hiding this comment

catandthemachines Dec 13, 2024

Choose a reason for hiding this comment

nishasy Dec 14, 2024

Choose a reason for hiding this comment

nishasy commented Dec 11, 2024 •

edited

Loading

github-actions bot commented Dec 11, 2024 •

edited

Loading

github-actions bot commented Dec 11, 2024 •

edited

Loading