Skip to content
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

fix: THREE.Math does not exist #517

Closed
wants to merge 1 commit into from

Conversation

GordonSmith
Copy link

THREE.Math was renamed to THREE.MathUtils in r113. THREE.PlaneBufferGeometry is deprecated.

Fixes #515

⚠️ All PRs have to be done versus 'dev' branch, so be aware of that, or we'll close your issue ⚠️

What kind of change does this PR introduce? bugfix

Can it be referenced to an Issue? If so what is the issue # ? 515

How can we test it? Follow the Three.js tutorial

Summary

Does this PR introduce a breaking change? Potentially for folks using Three.js older than v0.113.0?

Please TEST your PR before proposing it. Specify here what device you have used for tests, version of OS and version of Browser Edge 110.0.1587.41 (on Android)

Other information

THREE.Math was renamed to THREE.MathUtils in r113.
THREE.PlaneBufferGeometry is deprecated.

Fixes AR-js-org#515

Signed-off-by: Gordon Smith <GordonJSmith@gmail.com>
@kalwalt
Copy link
Member

kalwalt commented Feb 19, 2023

Thank you for this PR, i will look at in the next day. 🙂

@nickw1
Copy link
Collaborator

nickw1 commented Feb 25, 2023

@GordonSmith many thanks for this.

I think the bug arose from a time when we still had three.js lower than 0.113.

I have tested it out on desktop, using both pure three.js and A-Frame, and it works fine with no errors.

I don't think anyone will have older three, as the package-lock.json currently specifies version 0.146.

@kalwalt can we merge?

@kalwalt
Copy link
Member

kalwalt commented Feb 26, 2023

@nickw1 haven't tested yet, i think it should be ok but i think i will have a bit of time in the next days. I will let you know. 🙂

@kalwalt kalwalt added bug Something isn't working enhancement New feature or request dependencies Pull requests that update a dependency file labels Feb 26, 2023
@kalwalt
Copy link
Member

kalwalt commented Feb 27, 2023

We can't merge this PR as it is. @GordonSmith you need to switch to MathUtils (instead of Math) in device-orientation-control.js for example

const _q1 = new Quaternion(-Math.sqrt(0.5), 0, 0, Math.sqrt(0.5)); // - PI/2 around the x-axis
but there are other lines of code involved.
After rebuild the libs with the npm command npm run build and don't forget to run the prettier command too npm run format.
It wolud be nice if we upgrade the three.js version in the examples to a more recent one. Actually is pretty old: 132
Also the three.js npm package will require an upgrade.

@GordonSmith
Copy link
Author

@kalwalt - those Math calls were (and still are) just calling the JS built in Math library?

@kalwalt
Copy link
Member

kalwalt commented Feb 27, 2023

@kalwalt - those Math calls were (and still are) just calling the JS built in Math library?

It could be but we have to check, it could break also in other part of the AR.js library. Maybe @nickw1 have an answer.

@kalwalt
Copy link
Member

kalwalt commented Feb 27, 2023

referring to that line probably is using the JS built in library because there isn't a sqrt method https://threejs.org/docs/#api/en/math/MathUtils

@GordonSmith
Copy link
Author

@kalwalt - those Math calls were (and still are) just calling the JS built in Math library?

It could be but we have to check, it could break also in other part of the AR.js library. Maybe @nickw1 have an answer.

The imported name "MathUtil" didn't change (when you import { Math as MathUtil } only MathUtil is accessible in the JS...

@kalwalt
Copy link
Member

kalwalt commented Feb 27, 2023

@kalwalt - those Math calls were (and still are) just calling the JS built in Math library?

It could be but we have to check, it could break also in other part of the AR.js library. Maybe @nickw1 have an answer.

The imported name "MathUtil" didn't change (when you import { Math as MathUtil } only MathUtil is accessible in the JS...

Sorry, i want to say that upgrading the there.js version could break in other parts of Ar.js.

@nickw1
Copy link
Collaborator

nickw1 commented Feb 28, 2023

@kalwalt @GordonSmith yes that specific Math.sqrt() will just use inbuilt JS Math, as THREE.Math was imported as MathUtils.

I have tested @GordonSmith's fix on the basic location-based three.js and A-Frame examples and it works, but haven't tested a full range of examples.

@kalwalt what are the other lines of code which might cause a problem?

@GordonSmith
Copy link
Author

Sorry, i want to say that upgrading the there.js version could break in other parts of Ar.js.

This PR doesn't upgrade the three.js version?

@DougReeder
Copy link

Having just updated a number of A-Frame component to be compatible with the A-Frame 1.4.1 and the corresponding version of three.js, I'd like to make these points:

  1. (as noted above) Math.sin(), etc. is the JavaScript builtin Math, while Math.degToRad() and Math.radToDeg() is three.js. Recent versions of three.js changed the name to MathUtils, to reduce the confusion. It's not clear from the three.js docs if there are versions of three.js where you can use either name.
  2. three.js v 0.146.0, the current dependency, uses MathUtils.
  3. Recent versions of A-Frame and three.js drop support for non-buffered Geometry. However, BufferGeometry has been supported for a long time, so use of non-buffered Geometry can be changed to BufferGeometry without changing the version of three.js
  4. A-Frame 1.0.4 is three years old and unsupported, so the dependency really ought to be updated

@DougReeder DougReeder mentioned this pull request Mar 9, 2023
@DougReeder
Copy link

#523 would appear to be more complete than this.

@kalwalt
Copy link
Member

kalwalt commented Apr 4, 2023

Solved with #532 now that code is merged in the master branch. Thanks for your interest and colaboration!

@kalwalt kalwalt closed this Apr 4, 2023
@GordonSmith GordonSmith deleted the THREE_MATHUTILS branch April 5, 2023 14:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working dependencies Pull requests that update a dependency file enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants