search

ChristopherChudzicki / math3d-react

A user-friendly 3d graphing calculator for the web. Create, share, and animate 3d mathematical scenes.
https://www.math3d.org
Mozilla Public License 2.0
249 stars 34 forks source link

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

Closed sritchie closed 2 years ago

sritchie commented 2 years ago

@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 commented 2 years ago

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

sritchie commented 2 years ago

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

ChristopherChudzicki commented 2 years ago

@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:

Screen Shot 2022-02-19 at 2 30 21 PM

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

sritchie commented 2 years ago

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 https://github.com/ChristopherChudzicki/math3d-react/pull/330#issuecomment-1046089413, 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 commented 2 years ago

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

  1. 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

  2. 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.