hiukim / mind-ar-js

Web Augmented Reality. Image Tracking, Face Tracking. Tensorflow.js
MIT License
2.22k stars 415 forks source link

ES Modules. Decouple ThreeJS. Switch to Vite. Offline compiler #311

Closed jmschrack closed 1 year ago

jmschrack commented 1 year ago

Main Changes

Minor Changes

Maintainer Notes for @hiukim

hiukim commented 1 year ago

@jmschrack I have briefly went through your changes once. It's amazing! This can definitely help us move forward! Since it's a huge PR and an very important update, I'll do some more testings before I merge it and push a new release. For now, I have a few quick questions / feedback.

  1. vite config

I saw that there are difference between the build config of vite.config.dev.js and vite.config.prod.js, can you help me understand why they differ? More specifically:

assetsInclude (why needed only in dev?)
publicDir: (why needed only in prod?)
plugins: (I guess it's not necessary since it's empty?)
build: {
  copyPublicDir: (why needed only in prod?)
  rollupOptions: {
    external: (why included `three/examples/jsm/` in dev
  }
}
  1. custom tfjs

I saw you added custom tfjs in an earlier commit: https://github.com/hiukim/mind-ar-js/commit/60eb03b85f42a126cdabb147d2bcafdca28944c1 which was later removed from the vite config. I guess we are not using the custom tfjs anymore, right?

  1. .npmignore Does it has any effect? since we have files in package.json

  2. opencv.js How did you build the updated opencv.js?

  3. ui css How did you build the css string in ui/ui.js

  4. CSS3DRenderer I have a little bit concern over this since this file is copied from a specific version of three.js which might not align with the external threeJs version we import. Since you already mentioned there are troubles externalizing this, I'm sure you have already tried many things without success. If there is no easy solution, I guess we can ignore it for now, but it looks like something that we need to handle.

  5. threejs examples I guess there are some missing updates in the face tracking threeJS examples. They are still referencing window.MINDAR.FACE

  6. compiler.js and compiler.worker.js Might need some code refactoring there to remove code duplication. I'm thinking creating another util file under ./tracker/

  7. detector.js I'm very impressed! You basically rewrote another version of the detector with CPU code. Node.js image compilation is a long asked feature.

Can you refactor the detect method a bit though if you don't mind? I think we can keep all the internal functions, e.g. _applyFilter(), and move the tf calls e.g. tf.tidy(()=>{ return tf.engine().runKernel('BinomialFilter', { image: inputImageT });}); inside the _applyFilter

This way, we can basically keep the detect method unchanged, but only updated those internal methods. It's more consistent with the rest of the code and probably make the commit history cleaner.

jmschrack commented 1 year ago
  1. Vite Config differences: By default, if you have a directory named public, Vite will copy any files in there into the output directory. Since MindAR is generating libraries, I disabled copy/publicDir. However, during development, it's a convenient feature for easily including external files so I left it enabled in the dev build. As for assetsInclude:**/*.html, that's left over from some of my research. I can remove that to prevent confusion. As for "three/examples/jsm/' , I can't get Webpack, Rollup, or Vite to correctly externalize it. The only use for this was for CSS3DRenderer (which I solved by moving into the libs folder). For consistency, I left it in on the dev config, but I forgot to do that in the prod config. I can add that back in.
  2. Custom TFJS: TFJS comprises about ~80% of the resulting library size, and I think a custom TFJS build in the future will be helpful. In the short term I was doing some preliminary tests to make sure it would work with Vite (it does). But I haven't tested it rigorously enough so I decided to remove it from this PR. Looks like I removed the config but I forgot to un-version from git.
  3. .npmignore It's redundant, but the files property is newer in the npm spec. I'm just in the habit of doing both.
  4. OpenCV.js I did not rebuild. Opencv.js was architected in the pre-ESM era to auto detect what type of module was calling it and to try and provide the right export type. (CJS, UMD, Script, or IIFE) However, it doesn't support modern ESModules. In all cases it's just exporting the cv var and calling it when the module is hoisted. So I just removed the defunct boilerplate and added in an export to the var.
  5. ui.scss compile npx sass src/ui/ui.scss:ui.css
  6. CSS3DRenderer I understand the concern. Since it relies mostly on the internal matrix calculations of ThreeJS, it should be pretty stable. ($5 to whoever figures out how to properly externalize the ThreeJS addons. It's probably something simple I'm missing but I'm giving up on it for now.)
  7. Face Examples three-facemesh,html is missing part of an update. I was trying to show an alternate way to import the library via importmap whereas the others show the script embed way. I've pushed up the fix for this file already. I'll defer to you on which style you think should be the default way. I left the window.MINDAR.FACE/IMAGE mostly intact for backwards compatibility
  8. Compiler/Worker Agreed. I just did that to get the offline compiler working.
  9. Detector Sure!
jmschrack commented 1 year ago

@hiukim Found a solution for the CSS3DRenderer concern! Vite can apparently take a function for the external so we can use some string functions to dynamically determine what should and shouldn't be marked as external. This fixed the issue of ThreeJS getting accidentally bundled in.

I've also added a resolve alias for 'three/addons/':'three/examples/jsm/' . This matches the official ThreeJS documentation for using addons online. I've also updated the examples to match.

(By experimenting with writing out what files are getting passed in, it looks like ThreeJS Addons tend to get resolved to a full file system path, which is why it doesn't get caught the normal way!)

hiukim commented 1 year ago

@jmschrack Looks great! Thanks for the hard work!

Just merged your PR. I'll tidy up some code and create a new release! :)

jmschrack commented 1 year ago

It's nice to see 2 months of work pay off! :)

hiukim commented 1 year ago

@jmschrack

1) FYI, I found some memory leak in some webgl kernals. If you run tf.memory(), you will see. Check out this fix: https://github.com/hiukim/mind-ar-js/commit/12d0c514cc28ef9b8c096b2ce55746a61cb6ad03

2) Can you confirm we can use inline worker for compiler as well? https://github.com/hiukim/mind-ar-js/commit/e33de6fa055973ef638315e19a3f7b6bfabcc077

There was a path resolve error for the compiler worker and looks like it fixed that. Also, I saw you were using inline worker for Controller.

jmschrack commented 1 year ago
  1. I'm surprised that tf.tidy() doesn't catch that!

  2. This problem seems specific to the examples/image-tracking/compile.html as the ../../ messses up the Worker URL. However, In-lining the compiler worker creates a problem for the offline compiler. ?worker&inline is a Vite specific feature, so it creates a failure to import when running via NodeJS Instead, add base:'./', to the moduleConfig in the dev&prod configurations. https://github.com/jmschrack/mind-ar-js/blob/vite/vite.config.dev.js This fixes the compile.html example for me, keeps OfflineCompiler working as well.

Squareys commented 1 year ago

@hiukim I wonder, is there still a aframe/threejs independent library that works on the web? It looks like mindar-image.prod.js is now for node environments only and doesn't work in Wonderland Engine 🤔 I'll stick to older version for now...

hiukim commented 1 year ago

@Squareys It should still works on the web. You can still install with HTML script tag. Ref: https://hiukim.github.io/mind-ar-js-doc/installation

Squareys commented 1 year ago

Ideally, I would use the ES Modules, but when I tried importing, it would give me errors about node-fetch not found and others. Let my try again and open an issue to continue the conversation in case the errors still appear.