Upgrade mathbox to new npm package, threejs => r137 #330

sritchie · 2022-02-17T20:02:55Z

@ChristopherChudzicki, we're in business! This PR gets us onto the latest Mathbox, which can handle any version of three.js.

The script instantiation of everything is now gone. I didn't do any measuring of the production build size, but I bet it's smaller since mathbox is pulling in only what it needs from threejs now vs relying on the big global import.

The PR has comments inline describing in more detail what I had to change.

here are the new packages:

sritchie · 2022-02-18T00:25:04Z

Scratch that, my typo. It works great now, except for mouse interaction! Tackling that will be next, probably three.js upgrade / OrbitControls related.

sritchie

Some notes on the change.

sritchie · 2022-02-17T20:03:17Z

client/public/index.html


-    <link rel="manifest" href="%PUBLIC_URL%/manifest.json">
-    <link rel="shortcut icon" href="%PUBLIC_URL%/favicon.ico">
+    <link rel="manifest" href="%PUBLIC_URL%/manifest.json" />


sorry, my linter tidied this up. I'll back all of the linter changes out.

sritchie · 2022-02-18T05:32:55Z

client/public/index.html

@@ -17,7 +17,6 @@
    <script src="/mathquill.min.js"></script>
    <link rel="stylesheet" href="/mathquill.css">
    <!-- MathBox -->
-    <script src="/mathbox-bundle.min.js"></script>


sritchie · 2022-02-18T05:33:15Z

client/public/index.html

-    *  Some Comments:
-    *  - sincce there isn't a (relaiable, documented) mathbox npm package,
-    *    we load it as a script.
-    *  - Initializing mathbox inside a React Component led to some issues


I didn't make this change, so I'm not sure what this is about. Maybe there is a better way to initialize mathbox inside the react component that we can try.

I'm not sure either. I did not run into this issue when playing around with mathbox 2.1.0 today.

sritchie · 2022-02-18T05:34:27Z

client/src/components/ScrollWithOverflow/index.js

@@ -93,11 +94,11 @@ export default class ScrollWithOverflow extends React.PureComponent<Props, State
  coverRef: { current: null | HTMLDivElement }

  eventNames = [
-    'mousedown', 'mousemove', 'mouseup', 'wheel',
-    'touchstart', 'touchmove', 'touchend'
+    'pointerdown', 'pointermove', 'pointerup',


This change occurred in Three.js r120: https://github.com/mrdoob/three.js/wiki/Migration-Guide#r119--r120

Here's an example issue suggesting the fix that worked: mrdoob/three.js#20294 (comment)

sritchie · 2022-02-18T05:34:37Z

client/src/constants/colors.js

@@ -1,6 +1,6 @@
 // @flow

-const THREE = window.THREE
+import { Color } from "three/src/math/Color.js";


I've been enjoying these targeted imports.

sritchie · 2022-02-18T05:34:55Z

client/src/containers/MathBoxScene/components/MathBoxScene.js

@@ -27,6 +33,23 @@ type Props = {
  renderErrors: ErrorState,
  setError: SetError
 }
+
+export const mathboxElement = document.getElementById("mathbox");


okay, the next two consts are the meat. Do we want these declared somewhere else?

sritchie · 2022-02-18T05:35:10Z

client/src/services/LoopManager.js

-    this.canvas.addEventListener('touchstart', this.downHandler)
-    this.canvas.addEventListener('mouseup', this.mouseUpHandler)
-    this.canvas.addEventListener('touchend', this.touchExitHandler)
+    this.canvas.addEventListener('pointerdown', this.downHandler)


my understanding (could be wrong!) is that pointer replaces mouse and touch.

sritchie · 2022-02-18T05:35:30Z

client/src/services/LoopManager.js

    this.isPointerDown = false
    this.enterSlowMode()
  }

-  touchExitHandler = async (event: TouchEvent) => {


which makes this unnecessary. What am I missing?

sritchie · 2022-02-18T05:36:01Z

client/src/services/getCameraData.js

+
+import { mathbox } from "containers/MathBoxScene/components/MathBoxScene.js";
+
+export default function () {


I might suggest that this take mathbox as an argument?

sritchie · 2022-02-18T05:36:54Z

client/src/views/MainView/Scene.js

@@ -1,6 +1,7 @@
 import React, { PureComponent } from 'react'
 import MathBoxContainer from 'containers/MathBoxContainer'
 import MathBoxScene from 'containers/MathBoxScene'
+import { mathboxElement } from "containers/MathBoxScene/components/MathBoxScene";


easy as pie!

sritchie · 2022-02-18T05:44:26Z

@ChristopherChudzicki commenting here since I edited the description above; this all works great and is ready for review!

sritchie · 2022-02-18T06:19:28Z

client/src/services/getCameraData.js

+
+export default function () {
+  const position: Array<number> = mathbox.three.camera.position.toArray();
+  const lookAt: Array<number> = mathbox.three.controls.target.toArray();


this change from center to target happened in OrbitControls a few versions ago.

ChristopherChudzicki · 2022-02-19T19:33:49Z

@sritchie Awesome! Thanks for making this PR. I'm happy to take a look at the mouse interaction stuff. I did some very hacky things to enable this overflow:

I saw the issue you wrote in math3d-next... Will send an email shortly.

sritchie · 2022-02-19T19:37:32Z

Mouse interaction is all solved! The big thing remaining is getting tests to run now that we import three as a module. That seems to kill the test runner and I don’t see how to modify it…

…

On Saturday, February 19, 2022, Chris Chudzicki ***@***.***> wrote: @sritchie <https://github.com/sritchie> Awesome! Thanks for making this PR. I'm happy to take a look at the mouse interaction stuff. I did some very hacky things to enable this overflow: [image: Screen Shot 2022-02-19 at 2 30 21 PM] <https://user-images.githubusercontent.com/9010790/154816148-dd57475f-ca55-44e9-b248-16cb487d80c7.png> I saw the issue you wrote in math3d-next... Will send an email shortly. — Reply to this email directly, view it on GitHub <#330 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAARAA4DWAYC6ORYCKDVAGLU37WCRANCNFSM5OVW2W7A> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>. You are receiving this because you were mentioned.Message ID: ***@***.***>

-- Sam Ritchie

ChristopherChudzicki · 2022-02-21T01:47:58Z

@sritchie Re the failing tests: There were two separate issues:

Jest tests were failing because by default jest does not transpile node_modules, and the import/export syntax from mathbox, threestrap, shadergraph was causing syntax errors. I'm not really a webpack expert, but it might be worth targeting CommonJS for the node builds of those packages.

I fixed this with https://github.com/ChristopherChudzicki/math3d-react/pull/333/files#diff-1846122c2c83a486a3693f7966aa522c34cf489f674185c4da0d9221683fd81fR46
After fixing (1), App.test.js was failing because it couldn't initialize Mathbox (probably some lack of canvas support in JSDom, I'm not really sure). Previously Mathbox was initialized in the index.html file, so Jest never had to initialize it. But app.test.js was fairly useless, so I just deleted it.

I merged then fixed (1) on that separate PR.

Thanks again for this... it's an enormous improvement. As I've said... very exciting.

new mathbox

8914be3

sritchie mentioned this pull request Feb 17, 2022

Upgrade to module-based Mathbox, modern threeJS... interested? #302

Closed

working

e4cbb29

sritchie added 3 commits February 17, 2022 22:14

fix

5fde4ff

tidy

fe921bc

fix scene

4017604

sritchie commented Feb 18, 2022

View reviewed changes

sritchie changed the title ~~Upgrade mathbox to new npm package, remove script initialization~~ Upgrade mathbox to new npm package, threejs => r137 Feb 18, 2022

sritchie added 2 commits February 17, 2022 22:40

resolve

74e7c40

done

4ee7450

fix

dd371d6

sritchie commented Feb 18, 2022

View reviewed changes

try bumping node

d204f2e

ChristopherChudzicki merged commit 42e3eb6 into ChristopherChudzicki:master Feb 21, 2022

sritchie deleted the sritchie/new_mathbox branch February 21, 2022 01:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Upgrade mathbox to new npm package, threejs => r137 #330

Upgrade mathbox to new npm package, threejs => r137 #330

sritchie commented Feb 17, 2022 •

edited

Loading

sritchie commented Feb 18, 2022

sritchie left a comment

sritchie Feb 17, 2022

sritchie Feb 18, 2022

sritchie Feb 18, 2022

ChristopherChudzicki Feb 21, 2022

sritchie Feb 18, 2022

sritchie Feb 18, 2022

sritchie Feb 18, 2022

sritchie Feb 18, 2022

sritchie Feb 18, 2022

sritchie Feb 18, 2022

sritchie Feb 18, 2022

sritchie commented Feb 18, 2022

sritchie Feb 18, 2022

ChristopherChudzicki commented Feb 19, 2022

sritchie commented Feb 19, 2022 via email

ChristopherChudzicki commented Feb 21, 2022 •

edited

Loading


		import { mathbox } from "containers/MathBoxScene/components/MathBoxScene.js";

		export default function () {

Upgrade mathbox to new npm package, threejs => r137 #330

Upgrade mathbox to new npm package, threejs => r137 #330

Conversation

sritchie commented Feb 17, 2022 • edited Loading

sritchie commented Feb 18, 2022

sritchie 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

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

sritchie commented Feb 18, 2022

Choose a reason for hiding this comment

ChristopherChudzicki commented Feb 19, 2022

sritchie commented Feb 19, 2022 via email

ChristopherChudzicki commented Feb 21, 2022 • edited Loading

sritchie commented Feb 17, 2022 •

edited

Loading

ChristopherChudzicki commented Feb 21, 2022 •

edited

Loading