Fix orbit controls

[diarmidmackenzie]

This PR fixes orbit-controls to work with A-Frame 1.4.1, and updates the example to use A-Frame 1.4.1.

This was hitting errors with quaternion.inverse() and THREE.Math.degToRad, but rather than tweak individual lines of code, I went ahead and updated to the latest version of OrbitControls from THREE.js, which will hopefully make for easier maintenance in future.

I created a new example (a torus-knot in environment component), as the old example used supercraft, and can't be used with latest A-Frame version.

[vincentfretin]

This replaces the PR #322

[diarmidmackenzie]

I made that change to the comment.

Not sure about the other approach, currently three is a devDependency (via aframe), but not a dependency. Wouldn't three have to become a dependency for this to work?

Other components such as text-geometry also use the approach of copying the specific required module, rather than using npm. I think there is value in consistency of approach.

[vincentfretin]

You can just keep aframe as a devDependency, you don't really need to add three or super-three as a direct dependency. If you define three as an alias of super-three in the webpack config, you can use three as an indirect dependency, it will import from super-three when you use import { OrbitControls } from "three/examples/jsm/controls/OrbitControls.js". Doing this way has the inconvenient of not knowing which version of this file was used at the time of building the bundle because it will use the latest aframe/super-three version at the time the package was released because of "aframe": "*", unless you pin the version in packages.json or push the package-lock.json. Advantage is if you consume the package as an npm package and you have a similar webpack config or other bundler like vite properly configured, and use a more recent version of aframe, it will import the file from the latest version and not the OrbitControls.js version from the repo. Including the jsm files in the repo like this has the tendency to not be kept up to date. Example in aframe-inspector, we used to have an old GLTFExporter.js file in the repo (btw we still have a very old TransformControls in the repo and we have no idea which version it is and if there was some changes to it). When I updated the versions for aframe 1.4.0, I removed GLTFExporter.js from the repo, I pinned three dependency to 0.145.0 in package.json, and we now import GLTFExporter like this import { GLTFExporter } from 'three/examples/jsm/exporters/GLTFExporter'.

[vincentfretin]

@diarmidmackenzie I added two additional fixes on top of your branch https://github.com/diarmidmackenzie/superframe/pull/2 You can merge in your branch so it appears here.

[vincentfretin]

I created a branch where I removed budo and updated it to latest webpack and karma-webpack for orbit-controls with the import from three/examples/jsm/controls/OrbitControls.js as an example https://github.com/vincentfretin/superframe/commits/remove-budo if you're interested.

[diarmidmackenzie]

I created a branch where I removed budo and updated it to latest webpack and karma-webpack for orbit-controls with the import from three/examples/jsm/controls/OrbitControls.js as an example https://github.com/vincentfretin/superframe/commits/remove-budo if you're interested.

OK interesting. I wasn't familiar with using externals in package.json like this.

[dmarcos]

Anything pending here?

[diarmidmackenzie]

I'd say not. @vincentfretin has a proposal for how we could handle the code imported from THREE.js more cleanly, but it's applicable to more than just this component, and not required to get this working with 1.4.1.

I think best handled as a separate PR (if anyone wants to do it at all).

[vincentfretin]

Yes, you can merge the PR in this state. I can do another PR to update all the packages to replace budo by latest webpack dev server if you're interested. As you know you can't use budo with https on Ubuntu 22.04 because you have an error when generating the self-signed certificate and it doesn't support ES6 modules.