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

Webpack Build for AR.js (follow-up) #116

Merged
merged 177 commits into from
Nov 16, 2021
Merged

Webpack Build for AR.js (follow-up) #116

merged 177 commits into from
Nov 16, 2021

Conversation

MikiDi
Copy link
Collaborator

@MikiDi MikiDi commented May 21, 2020

ℹ️ This is a follow-up PR for #86 . Whereas #86 serves more as a discussion-board, discussing broad context, this PR features the right source & target branch, allowing for technical review to make the PR merge-ready.

What kind of change does this PR introduce?

Webpack build-system instead of "cat build-system" 😉 , module-based code, usage of andypotato's
artoolkit5-js

Related issues

Summary

In order to comfortably integrate this great library in a modern web application (React, Angular, Ember.js, ...) it would be nice to have AR.js provided as a ES6 module (while keeping support for browser <script> usage). Refactoring the library to also use import/export internally, has the additional (and arguably most important) advantage of improved maintainability.
In order to achieve the above, I refactored the library to use imports/exports instead of relying on globals and introduced a build configuration for webpack. Since the jsartoolkit5 library is somewhat flawed when it comes to ES6 support, this PR depends on the work done by @andypotato in artoolkit5-js to make the library ES6-compatible.

Does this PR introduce a breaking change?

No. Functionality-wise, the end-result of this PR should do exactly the same as the current master build.

How can we test it?

pattern-marker build

Aframe: Host the basic example and use the aframe-ar.js build from the PR branch.
Threejs: default example and use the ar-threex.js build.

NFT-build

Host the nft t-rex example and use the aframe-ar-nft.js build from the PR branch.

Making changes and rebuilding

npm install
npm run build:dev

Device used for tests, version of OS and version of Browser

Desktop (webcam):

  • Fedora 30, Firefox 74.0
  • Opensuse tumbleweed, Chrome 95.0.4638.69
  • Ubuntu 20.04.1 Chrome 95
    Android device:
  • Oppo A72, Chrome 95.0.4638.74

Codebase changes

We don't want to introduce breaking changes, At this time you should use the same classes, methods and properties. The namespaces THREEx and ARjs are bundled in separate build now. We would create a doc section on this.

THREEx namespace classes

THREEx.ArBaseControls
THREEx.ARClickability
THREEx.ArMarkerCloak
THREEx.ArMarkerControls
THREEx.ArMarkerHelper
THREEx.ArSmoothedControls
THREEx.ArToolkitContext
THREEx.ArToolkitProfile
THREEx.ArToolkitSource
THREEx.ArVideoInWebgl
THREEx.HitTestingPlane

ARjs namespace classes

ARjs.MarkerControls
ARjs.Context
ARjs.Profile
ARjs.Source

TODO's

  • "threex" exports (figure out which classes should need to be exposed)
  • The nft video example the video is rotated 90° in respect of the marker, so is not flat and can not viewed as usual. (see @kalwalt 's testing here
  • Merge changes that happened on master starting from 04/04/2020
  • Merge changes that happened on master (version 3.3.2 and 3.3.3)
  • check if the worker-loader is correct
  • keep the same codebase design for all the classes in the THREEx and ARjs namespaces
  • fix nft examples/code in both threejs and aframe version, they fails with Error while intizalizing arController No canvas available -> partial fix switched to a previuos artoolkit5-js version
  • fix aframe/nft examples
  • production builds of the libs
  • removing multiple instances of Three.js warning, see the log message: three.module.js:49640 WARNING: Multiple instances of Three.js being imported. This will be fixed in another PR.
  • separate externals in webpack.config.js for the AFRAME and THREEx namespace.
  • restore master branch in aframe nft examples instead of ES6 branch. this should be done when merged in master
  • removing double ARtoolkit.js printing from artoolkit5-js package (this is normal because we instantiate two artoolkit instance) anyway not a real issue...
  • removing https://arjs-cors-proxy.herokuapp.com from examples, change paths with resources stored in githubusercontent.
  • fix issue with threejs/test_runner.html example error: Uncaught DOMException: Failed to execute 'getImageData' on 'CanvasRenderingContext2D': The canvas has been tainted by cross-origin data. This was caused because different baseUrl! see commit 283cc06 and comments Webpack Build for AR.js (follow-up) #116 (comment) Webpack Build for AR.js (follow-up) #116 (comment)
  • new Three.js 0.127.0 0.132.2 version in package.json and in the examples
  • removing THREE.Matrix4: .getInverse() in arjs-markercontrols.js arjs-markercontrols-nft.js because is deprecated and replaced with .matrix.copy(modelViewMatrix).invert()
  • fix for bug in removeMarker see commit 84e640e
  • apply changes from Change use of alert() to appending HTML div to display error messages #270 Update system-arjs.js / Aframe Build / Exponential Backoff #304 Fix server command for new contributors #344 and other commits (see last commits in this PR)

@kalwalt
Copy link
Member

kalwalt commented Nov 12, 2021

published new npm beta package 3.3.2-es6-beta-01 with commit 2beb20b

@kalwalt
Copy link
Member

kalwalt commented Nov 12, 2021

@nickw1 i applied the changes for location-based from 3.3.3 https://github.com/AR-js-org/AR.js/releases/tag/3.3.3 but when i try the examples i get errors like this:

core:a-node:error Failure loading node:   ReferenceError: localPosition is not defined
    at i.play (gps-camera.js:122)
    at i.<anonymous> (component.js:787)
    at initComponent (component.js:334)
    at i.updateProperties (component.js:302)
    at i.module.exports.Component (component.js:78)
    at new i (component.js:662)
    at HTMLElement.value (a-entity.js:332)
    at HTMLElement.value (a-entity.js:495)
    at HTMLElement.value (a-entity.js:463)
    at a-entity.js:249

Maybe i missed something? I think we are very close to a conclusion...

@kalwalt
Copy link
Member

kalwalt commented Nov 14, 2021

I have a fix for this issue, i will apply this evening.🙂

@kalwalt
Copy link
Member

kalwalt commented Nov 14, 2021

with commit 57928db i published a new npm beta package 3.3.3-es6-beta-01 i will start to think for a merge in the dev branch now!

@kalwalt
Copy link
Member

kalwalt commented Nov 15, 2021

There some issues with mobile devices, it depends which example you test but i think it's time to merge into dev, fix them when merged and eventually make other changes to improve the code.
The last commit in dev branch is 6812d75 so this is our reference in case we need to compare them.
I will start the merge tomorrow.

@nickw1
Copy link
Collaborator

nickw1 commented Nov 16, 2021

@nickw1 i applied the changes for location-based from 3.3.3 https://github.com/AR-js-org/AR.js/releases/tag/3.3.3 but when i try the examples i get errors like this:

core:a-node:error Failure loading node:   ReferenceError: localPosition is not defined
    at i.play (gps-camera.js:122)
    at i.<anonymous> (component.js:787)
    at initComponent (component.js:334)
    at i.updateProperties (component.js:302)
    at i.module.exports.Component (component.js:78)
    at new i (component.js:662)
    at HTMLElement.value (a-entity.js:332)
    at HTMLElement.value (a-entity.js:495)
    at HTMLElement.value (a-entity.js:463)
    at a-entity.js:249

Maybe i missed something? I think we are very close to a conclusion...

@kalwalt glad you have fixed this. Sorry, I have been ill the past few days, sorry I missed your original message!

@kalwalt
Copy link
Member

kalwalt commented Nov 16, 2021

@nickw1 no problem, if we find other issues we can fix them when merged.

@kalwalt kalwalt merged commit fccd194 into dev Nov 16, 2021
@kalwalt
Copy link
Member

kalwalt commented Nov 16, 2021

Thank you to all AR.js friends @MikiDi @nickw1 @nicolocarpignoli @andypotato and others that had contributed to this PR/feature, this is possible because your help!
Let's improve now the code in the dev branch, issues will arise and we have to fix them. The code is not perfect but this PR is becoming huge and we prefer smaller PR's that are easier to manage them... I will publish another beta npm package as soon as possible.

@kalwalt kalwalt deleted the ES6 branch December 22, 2024 22:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request ES6 module AR.js as a ES6 module
Projects
None yet
Development

Successfully merging this pull request may close these issues.