search

AR-js-org / AR.js

Image tracking, Location Based AR, Marker tracking. All on the Web.
MIT License
5.3k stars 909 forks source link

fix: THREE.Math does not exist #517

Closed GordonSmith closed 1 year ago

GordonSmith commented 1 year ago

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

kalwalt commented 1 year ago

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

nickw1 commented 1 year ago

@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 commented 1 year ago

@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 commented 1 year ago

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 https://github.com/AR-js-org/AR.js/blob/8662537d2dc1701afc4eadfbf7b04db3341199de/three.js/src/location-based/js/device-orientation-controls.js#L15 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 commented 1 year ago

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

kalwalt commented 1 year ago

@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 commented 1 year ago

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 commented 1 year ago

@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 commented 1 year ago

@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 commented 1 year ago

@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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

https://github.com/AR-js-org/AR.js/pull/523 would appear to be more complete than this.

kalwalt commented 1 year ago

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