Fix a bug where line-ending arrows would flip around or vanish #1263

benchristel · 2024-05-13T23:31:17Z

The bug happened when -0 was passed as a coordinate (yes, -0 is a distinct
value in floating point, because the sign bit is stored separately from the
mantissa).

This PR patches the bug by handling horizontal and vertical lines as a special
case, and then refactors to a design where the special-casing is no longer
necessary. It's probably easiest to read the PR one commit at a time.

Issue: LEMS-1955

Test plan:

yarn dev
Play around with the linear graphs at localhost:5173

Math.abs(xAtYMin) is not the right way to fix the -Infinity issue - it results in the line going off the edge of the graph when the graph has a nonsquare range like [[-10,1],[-10,10]].

khan-actions-bot · 2024-05-13T23:31:32Z

Gerald

Required Reviewers

@Khan/perseus for changes to .changeset/giant-seals-camp.md, packages/perseus/src/widgets/interactive-graphs/locked-line.tsx, packages/perseus/src/widgets/interactive-graphs/graphs/utils.test.ts, packages/perseus/src/widgets/interactive-graphs/graphs/utils.ts, packages/perseus/src/widgets/interactive-graphs/graphs/components/angle.tsx, packages/perseus/src/widgets/interactive-graphs/graphs/components/movable-line.tsx

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

github-actions · 2024-05-13T23:33:01Z

npm Snapshot: Published

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

Example:

yarn add @khanacademy/perseus@PR1263

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

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

github-actions · 2024-05-13T23:34:05Z

Size Change: +474 B (+0.06%)

Total Size: 836 kB

Filename	Size	Change
`packages/perseus-editor/dist/es/index.js`	267 kB	-33 B (-0.01%)
`packages/perseus/dist/es/index.js`	401 kB	+507 B (+0.13%)

ℹ️ View Unchanged

Filename	Size
`packages/kas/dist/es/index.js`	38.1 kB
`packages/kmath/dist/es/index.js`	4.27 kB
`packages/math-input/dist/es/index.js`	80.5 kB
`packages/math-input/dist/es/strings.js`	1.73 kB
`packages/perseus-core/dist/es/index.js`	908 B
`packages/perseus-error/dist/es/index.js`	878 B
`packages/perseus-linter/dist/es/index.js`	21.8 kB
`packages/perseus/dist/es/strings.js`	3.22 kB
`packages/pure-markdown/dist/es/index.js`	3.68 kB
`packages/simple-markdown/dist/es/index.js`	12.4 kB

_{compressed-size-action}

codecov · 2024-05-13T23:44:53Z

Codecov Report

Attention: Patch coverage is 86.66667% with 4 lines in your changes are missing coverage. Please review.

Project coverage is 69.90%. Comparing base (896a159) to head (68ae088).
Report is 2 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1263      +/-   ##
==========================================
+ Coverage   69.08%   69.90%   +0.82%     
==========================================
  Files         472      476       +4     
  Lines      101103   101283     +180     
  Branches     7216    10856    +3640     
==========================================
+ Hits        69843    70802     +959     
+ Misses      31082    30481     -601     
+ Partials      178        0     -178

Files	Coverage Δ
...ets/interactive-graphs/graphs/components/angle.tsx	`78.81% <100.00%> (+0.98%)`	⬆️
...eractive-graphs/graphs/components/movable-line.tsx	`100.00% <100.00%> (ø)`
...eus/src/widgets/interactive-graphs/locked-line.tsx	`99.27% <100.00%> (ø)`
...eus/src/widgets/interactive-graphs/graphs/utils.ts	`93.44% <83.33%> (-4.84%)`	⬇️

... and 144 files with indirect coverage changes

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 896a159...68ae088. Read the comment docs.

jeremywiebe

This feels much more clear now and needs less complex calculations (the clock analogy). Thanks!

jeremywiebe · 2024-05-13T23:54:49Z

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

+ * @param initialPoint - A point that the ray passes through. Must be different from initialPoint.
+ * @param throughPoint - The starting point of the ray.


This re-arrangement of comments feels like the descriptions now directly conflict with the parameter name.

@param throughPoint - The starting point of the ray So it's not a through point but a starting point?

Good catch!

jeremywiebe · 2024-05-14T00:02:57Z

packages/perseus/src/widgets/interactive-graphs/graphs/components/movable-line.tsx

@@ -148,10 +148,10 @@ const Line = (props: LineProps) => {
    if (extend) {
        const trimmedRange = trimRange(range, graphDimensionsInPixels);
        startExtend = extend.start
-            ? getIntersectionOfRayWithBox(start, end, trimmedRange)
+            ? getIntersectionOfRayWithBox(end, start, trimmedRange)


Not directly related to this PR, but Nisha recently changed the locked line component to simply use point1 and point2 for lines. This feels like it might be applicable for the MovableLine component(s) as well as for line and ray they don't really have a start/end but rather two points that the line/ray intersect. 🤔

For a future PR if you agree.

jeremywiebe · 2024-05-14T00:05:21Z

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

        default:
-            return [xMax, yAtXMax];
+            return [0, 0];


Is this basically stating that we'll never hit this default? If so, could you comment on that as I might be confused for a bit if I arrived at this line of code without having read this PR or surrounding code.

jeremywiebe · 2024-05-14T00:06:23Z

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

-        // 6 o'clock to 8:59
-        case yDiff < 0 && xDiff <= 0:
-            return xAtYMin < xMin ? [xMin, yAtXMin] : [xAtYMin, yMin];
+        // does the ray intersect the left or right edge?


I'm not sure if it's obvious to others, but this is:

Suggested change

// does the ray intersect the left or right edge?

// does the ray intersect the left or right edge within the y graph range?

right?

@jeremywiebe

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/math-input@19.1.0 ### Minor Changes - [#1259](#1259) [`c07644f63`](c07644f) Thanks [@jeremywiebe](https://github.com/jeremywiebe)! - Update all Perseus peer dependencies to match webapp exactly (commit: 2bd290251a8) - @khanacademy/wonder-blocks-clickable v4.2.1 - @khanacademy/wonder-blocks-core v6.4.0 - @khanacademy/wonder-blocks-popover v3.2.2 - @khanacademy/wonder-blocks-timing v5.0.0 - @khanacademy/wonder-blocks-tokens v1.3.0 - @khanacademy/wonder-stuff-core v1.5.2 - aphrodite v1.2.5 - katex v0.11.1 - react v16.14.0 - react-router v5.2.1 - react-router-dom v5.3.0 ## @khanacademy/perseus@22.5.0 ### Minor Changes - [#1259](#1259) [`c07644f63`](c07644f) Thanks [@jeremywiebe](https://github.com/jeremywiebe)! - Update all Perseus peer dependencies to match webapp exactly (commit: 2bd290251a8) - @khanacademy/wonder-blocks-banner v3.0.42 - @khanacademy/wonder-blocks-button v6.3.1 - @khanacademy/wonder-blocks-clickable v4.2.1 - @khanacademy/wonder-blocks-core v6.4.0 - @khanacademy/wonder-blocks-data v13.0.7 - @khanacademy/wonder-blocks-dropdown v5.3.0 - @khanacademy/wonder-blocks-form v4.5.1 - @khanacademy/wonder-blocks-icon v4.1.0 - @khanacademy/wonder-blocks-layout v2.0.32 - @khanacademy/wonder-blocks-link v6.1.1 - @khanacademy/wonder-blocks-pill v2.2.1 - @khanacademy/wonder-blocks-popover v3.2.2 - @khanacademy/wonder-blocks-progress-spinner v2.1.1 - @khanacademy/wonder-blocks-switch v1.1.16 - @khanacademy/wonder-blocks-tokens v1.3.0 - @khanacademy/wonder-blocks-tooltip v2.3.1 - @khanacademy/wonder-blocks-typography v2.1.11 - @khanacademy/wonder-stuff-core v1.5.2 - @popperjs/core v^2.10.2 - classnames v1.1.4 - create-react-class v15.6.3 - katex v0.11.1 - prop-types v15.6.1 - react-popper v^2.2.5 - [#1261](#1261) [`f92c52412`](f92c524) Thanks [@Myranae](https://github.com/Myranae)! - Fix bug where arrow keys did not move all line segments ### Patch Changes - [#1256](#1256) [`9e2f404bb`](9e2f404) Thanks [@benchristel](https://github.com/benchristel)! - Update the styling of interactive circle graphs rendered with Mafs - [#1263](#1263) [`1f03243ba`](1f03243) Thanks [@benchristel](https://github.com/benchristel)! - Fix a bug in Mafs linear graphs where the arrows on the ends of the lines would sometimes disappear or flip around - [#1260](#1260) [`896a159a6`](896a159) Thanks [@nishasy](https://github.com/nishasy)! - Rename "start point" and "end point" to "point 1" and "point 2" respectively in Interactive Graph locked line settings - [#1250](#1250) [`3806759ea`](3806759) Thanks [@benchristel](https://github.com/benchristel)! - Internal: Refactor MovableLine component - Updated dependencies \[[`c07644f63`](c07644f), [`c07644f63`](c07644f)]: - @khanacademy/perseus-linter@0.4.0 - @khanacademy/math-input@19.1.0 - @khanacademy/pure-markdown@0.3.4 ## @khanacademy/perseus-editor@6.3.0 ### Minor Changes - [#1265](#1265) [`ef684516c`](ef68451) Thanks [@nishasy](https://github.com/nishasy)! - LockedLineSettings summary now uses a line swatch instead of a color swatch - [#1259](#1259) [`c07644f63`](c07644f) Thanks [@jeremywiebe](https://github.com/jeremywiebe)! - Update all Perseus peer dependencies to match webapp exactly (commit: 2bd290251a8) - @khanacademy/wonder-blocks-accordion v1.3.1 - @khanacademy/wonder-blocks-button v6.3.1 - @khanacademy/wonder-blocks-banner v3.0.42 - @khanacademy/wonder-blocks-clickable v4.2.1 - @khanacademy/wonder-blocks-core v6.4.0 - @khanacademy/wonder-blocks-dropdown v5.3.0 - @khanacademy/wonder-blocks-form v4.5.1 - @khanacademy/wonder-blocks-icon-button v5.2.1 - @khanacademy/wonder-blocks-switch v1.1.16 - @khanacademy/wonder-blocks-tooltip v2.3.1 - @khanacademy/wonder-blocks-tokens v1.3.0 - @khanacademy/wonder-blocks-typography v2.1.11 - @khanacademy/wonder-stuff-core v1.5.2 - classnames v1.1.4 - create-react-class v15.6.3 - prop-types v15.6.1 ### Patch Changes - [#1260](#1260) [`896a159a6`](896a159) Thanks [@nishasy](https://github.com/nishasy)! - Rename "start point" and "end point" to "point 1" and "point 2" respectively in Interactive Graph locked line settings - [#1262](#1262) [`88c48a784`](88c48a7) Thanks [@nishasy](https://github.com/nishasy)! - Locked line coordinates update on input change instead of on input blur - [#1254](#1254) [`e4b893c76`](e4b893c) Thanks [@nishasy](https://github.com/nishasy)! - Locked line summary reflects line kind - Updated dependencies \[[`9e2f404bb`](9e2f404), [`1f03243ba`](1f03243), [`896a159a6`](896a159), [`3806759ea`](3806759), [`c07644f63`](c07644f), [`c07644f63`](c07644f), [`f92c52412`](f92c524)]: - @khanacademy/perseus@22.5.0 - @khanacademy/math-input@19.1.0 ## @khanacademy/perseus-error@0.3.0 ### Minor Changes - [#1259](#1259) [`c07644f63`](c07644f) Thanks [@jeremywiebe](https://github.com/jeremywiebe)! - Update all Perseus peer dependencies to match webapp exactly (commit: 2bd290251a8) - @khanacademy/wonder-stuff-core v1.5.2 ## @khanacademy/perseus-linter@0.4.0 ### Minor Changes - [#1259](#1259) [`c07644f63`](c07644f) Thanks [@jeremywiebe](https://github.com/jeremywiebe)! - Update all Perseus peer dependencies to match webapp exactly (commit: 2bd290251a8) - prop-types v15.6.1 ### Patch Changes - Updated dependencies \[[`c07644f63`](c07644f)]: - @khanacademy/perseus-error@0.3.0 ## @khanacademy/pure-markdown@0.3.4 ### Patch Changes - Updated dependencies \[[`c07644f63`](c07644f)]: - @khanacademy/perseus-error@0.3.0 ## @khanacademy/perseus-dev-ui@1.5.7 ### Patch Changes - Updated dependencies \[[`c07644f63`](c07644f), [`c07644f63`](c07644f)]: - @khanacademy/perseus-linter@0.4.0 - @khanacademy/math-input@19.1.0 - @khanacademy/pure-markdown@0.3.4

benchristel added 12 commits May 13, 2024 12:42

Swap initialPoint and throughPoint args

ce063f1

Add failing test cases

a7c8ca1

Refactor case statement to remove ternaries

92c443a

Always use >= and <

761edb9

Math.abs(xAtYMin) is not the right way to fix the -Infinity issue - it results in the line going off the edge of the graph when the graph has a nonsquare range like [[-10,1],[-10,10]].

Fix test by special-casing slope 0

f848cb8

Symmetrize

379bace

Add paragraph breaks

0183a5c

Special-case vertical and horizontal lines

8410a5d

Consolidate cases that had the same return value

13d7308

Symmetrize

b6899c5

De-duplicate cases

93f56d8

Deduplicate

50c2371

benchristel self-assigned this May 13, 2024

benchristel requested review from handeyeco, jeremywiebe and Myranae May 13, 2024 23:31

khan-actions-bot requested a review from a team May 13, 2024 23:31

benchristel added 2 commits May 13, 2024 16:35

Fix lint

f65d1de

Add changeset

03c7a24

jeremywiebe approved these changes May 14, 2024

View reviewed changes

benchristel added 2 commits May 13, 2024 17:10

Add test for default branch

7d7a4e8

Comments

68ae088

benchristel merged commit 1f03243 into main May 14, 2024
13 checks passed

benchristel deleted the benc/fix-line-arrows branch May 14, 2024 19:01

khan-actions-bot mentioned this pull request May 14, 2024

Version Packages #1253

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix a bug where line-ending arrows would flip around or vanish #1263

Fix a bug where line-ending arrows would flip around or vanish #1263

benchristel commented May 13, 2024

khan-actions-bot commented May 13, 2024 •

edited

github-actions bot commented May 13, 2024 •

edited

github-actions bot commented May 13, 2024 •

edited

codecov bot commented May 13, 2024 •

edited

jeremywiebe left a comment

jeremywiebe May 13, 2024

benchristel May 14, 2024

jeremywiebe May 14, 2024

jeremywiebe May 14, 2024

jeremywiebe May 14, 2024

		* @param initialPoint - A point that the ray passes through. Must be different from initialPoint.
		* @param throughPoint - The starting point of the ray.

	// does the ray intersect the left or right edge?
	// does the ray intersect the left or right edge within the y graph range?

Fix a bug where line-ending arrows would flip around or vanish #1263

Fix a bug where line-ending arrows would flip around or vanish #1263

Conversation

benchristel commented May 13, 2024

Test plan:

khan-actions-bot commented May 13, 2024 • edited

Gerald

github-actions bot commented May 13, 2024 • edited

npm Snapshot: Published

github-actions bot commented May 13, 2024 • edited

codecov bot commented May 13, 2024 • edited

Codecov Report

jeremywiebe left a comment

Choose a reason for hiding this comment

jeremywiebe May 13, 2024

Choose a reason for hiding this comment

benchristel May 14, 2024

Choose a reason for hiding this comment

jeremywiebe May 14, 2024

Choose a reason for hiding this comment

jeremywiebe May 14, 2024

Choose a reason for hiding this comment

jeremywiebe May 14, 2024

Choose a reason for hiding this comment

khan-actions-bot commented May 13, 2024 •

edited

github-actions bot commented May 13, 2024 •

edited

github-actions bot commented May 13, 2024 •

edited

codecov bot commented May 13, 2024 •

edited