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 #86

Closed
wants to merge 51 commits into from
Closed

Webpack Build for AR.js #86

wants to merge 51 commits into from

Conversation

MikiDi
Copy link
Collaborator

@MikiDi MikiDi commented Apr 19, 2020

ℹ️ This is a draft PR. Its aim is to provide a forum to discuss the changes introduced and seek some help in order 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.

I started off with getting the regular (non-nft) build to work and feel like I'm quite far, but got somewhat stuck now. When running the basic example, the initialization process goes well, markers are detected when in view (it runs through updateWithModelViewMatrix), but the gltf model ( 🦖 ) doesn't show. When debugging I discovered the A-Frame inspector (I have little prior experience with aframe/three.js). The weirdest thing is that when you run the example with the A-Frame inspector, everything works as expected! 😮 This leads me to suspect that there's a minor bug in the aframe/three.js related part, which perhaps can be easily solved with some more background knowledge ... I can't seem to figure it out ...
Before continuing work to get the NFT build going (and on a longer term refactor out the separate builds and code duplication between nft/non-nft ), I'd like to get this fixed ... Is this something you could help out with maybe @kalwalt @nicolocarpignoli ?

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?

Host the basic example and use the aframe-ar.js build from the PR branch.
Add A-Frame inspector and press <ctrl> + <alt> + i to inject the inspector (make sure to press ⏯️ )
In order to make changes and rebuild:

npm install
npm run build:dev

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

Desktop (webcam), Fedora 30, Firefox 74.0

@kalwalt
Copy link
Member

kalwalt commented Apr 19, 2020

@MikiDi 🎉 😄 I am very interested on the ES6 support, It is on my to do list! The old cat / make system is not suitable anymore in my opinion. I will review your PR, also providing support/help if needed. But i need a bit of time for testing...

@kalwalt
Copy link
Member

kalwalt commented Apr 19, 2020

Not tested but maybe the problem is in this line: https://github.com/MikiDi/AR.js/blob/0a69d3696fade29845702c67558c8aa79ab0ef8a/three.js/src/threex/arjs-context.js#L262
We had this problem with the new jsartoolkit5 lib, and we need to uncomment the multiplication because an internal change in the Artoolkit5 C++ code. See #60 #59 #68

@MikiDi
Copy link
Collaborator Author

MikiDi commented Apr 19, 2020

That did it 👌 thx a lot. Will get back here when the NFT build is working :-)

@MikiDi
Copy link
Collaborator Author

MikiDi commented May 17, 2020

Funny that I read your comment just after implementing it that way :-D Fixed the bugs I had in artoolkit. It's coming along ... it builds and runs, but doesn't detect the T-Rex atm ... hope to clean up some more later today

@kalwalt
Copy link
Member

kalwalt commented May 17, 2020

Funny that I read your comment just after implementing it that way :-D Fixed the bugs I had in artoolkit. It's coming along ... it builds and runs, but doesn't detect the T-Rex atm ... hope to clean up some more later today

Yeah, maybe some telepathic force here! I willl test if ican, Do you receive some error in the dev console?

@kalwalt
Copy link
Member

kalwalt commented May 17, 2020

I just tested the aframe/examples/image-tracking/nft . It works fine for me, it is detected and i can see the trex. You need to be very close to the image to be tracked, and consider that the trex model is a quite heavy to download. For testing maybe you can replace with a model hosted on your pc. Anyway Well done! 😄

@MikiDi
Copy link
Collaborator Author

MikiDi commented May 17, 2020

Hah! Better than here 😄 ... Had been wondering already if the webcam I test on is of high enough quality for image-based tracking ... Don't think that the model-size is an issue ...
So ... next steps to get this ready for merge ... exports for the "threex" classes? The more I look at it, the more I would find it logical that only the "threex"-part (the "real AR.js library") be in this repo, and that the aframe components would reside in another repo, so that eventually there would be only 1 build per repo (after having merged the NFT and non-NFT code). There's a lot to talk about ... should need to take some time to think & write out more of a plan. A call might be useful two, if you're up for that ... Will start off next week by opening a new PR, based on the ES6 branch in this repo.

@kalwalt
Copy link
Member

kalwalt commented May 20, 2020

Hah! Better than here smile ... Had been wondering already if the webcam I test on is of high enough quality for image-based tracking ... Don't think that the model-size is an issue ...

i have not better devices believe me! 😄

So ... next steps to get this ready for merge ... exports for the "threex" classes?

Yes, that will be nice in my opinion!

The more I look at it, the more I would find it logical that only the "threex"-part (the "real AR.js library") be in this repo, and that the aframe components would reside in another repo, so that eventually there would be only 1 build per repo (after having merged the NFT and non-NFT code). There's a lot to talk about ... should need to take some time to think & write out more of a plan. A call might be useful two, if you're up for that ... Will start off next week by opening a new PR, based on the ES6 branch in this repo.

Two repository will mean break the AR.js project i think. Altough we could create a git submodule. I don't know How may think @nicolocarpignoli about this problem. I would also listen also other people/devs.

@MikiDi MikiDi mentioned this pull request May 21, 2020
19 tasks
@andypotato
Copy link

Just FYI, I finally came along to test @MikiDi pull request for artoolkit5-js - I can confirm it's working beautifully and merged the request into the main branch. Thanks a million for this contribution!

@inakineitor
Copy link

Hello! Is the webpack build completed? And is there anything we can do to contribute and move it further along?

@andypotato
Copy link

Hello! Is the webpack build completed? And is there anything we can do to contribute and move it further along?

The PR has been ready for quite a while but hasn't been reviewed or tested by @kalwalt or @nicolocarpignoli yet. Is there anything we can do to assist you guys?

@nicolocarpignoli
Copy link
Member

nicolocarpignoli commented Aug 28, 2020

Hi guys, unfortunately I have no time for this. I think that @inakineitor @andypotato or others can test this by yourself.

If this works, we will create an alternative AR.js version to start, so there is no worries to break the current build.
Feel free to test and share the results here, so we can follow progresses.

@kalwalt
Copy link
Member

kalwalt commented Oct 16, 2020

@MikiDi @nicolocarpignoli and all here, I will come back on this! i hope to have time to make a detailed review on this PR. Better to continue the discussion on the follow up PR #116 😄

@MikiDi
Copy link
Collaborator Author

MikiDi commented Oct 19, 2020

@kalwalt Cool! Let me know if I can help out. The only thing on my virtual todo list, was to expose the "old" api classes (the ones that currently are exposed through the intermediary build). Apart from that, it will mostly be manual work to catch up with the diff that has been growing is my guess ...

@MikiDi
Copy link
Collaborator Author

MikiDi commented Mar 20, 2021

The setup of a webpack-based build is being discussed further in #116 . Closing this one.

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.

None yet

5 participants