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

Enhancement in the tools/makem.js script and other fixes #96

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

kalwalt
Copy link

@kalwalt kalwalt commented Apr 2, 2020

This PR add these features:

  • --no-libar option to speed up the building process of the lib
  • description of this feature in the Readme.md and other small fixes.
  • fixed old links pointing to artoolkit repositroy (instead of artoolkitx) in the examples

description of the new script feature:
to build the whole libs run the build command with:

npm run build-local

with Docker ( !! recommended !! ):

docker exec emscripten npm run build-local

after this libar.bc is build and if you have done some changes in the code, but not in artoolkit5, you don't need to build libar.bc again so run:

npm run build-local-no-libar

with Docker ( !! recommended !! ):

docker exec emscripten npm run build-local-no-libar

In this way the build process will take about in half the time.

Related PR kalwalt#45 issue kalwalt#44

@kalwalt
Copy link
Author

kalwalt commented Apr 2, 2020

@nicolocarpignoli I would add also the ALLOW_MEMORY_GROWTH=1 as you said that is needed.

@kalwalt
Copy link
Author

kalwalt commented Apr 4, 2020

I simply tested adding FLAGS += ' -s ALLOW_MEMORY_GROWTH=1'; and rebuilding the libs with Docker, Did you make other modifies @nicolocarpignoli?

I think we should also improve the nft examples @ThorstenBux, should we present only the WASM version as you did in your https://github.com/ThorstenBux/ar-magazine/blob/master/src/threejs_wasm_worker.js ?
We could only leave two examples with wasm. The example of the sphere is the one with the model gltf. i would like to create a script that can be reused multiple times, example:

if (navigator.mediaDevices && navigator.mediaDevices.getUserMedia) {
        var hint = {
          audio: false,
          video: true
        };

        if (window.innerWidth < 800) {
          hint = {
            audio: false,
            video: {
                width: { ideal: 480 },
                height: { ideal: 640 },
                facingMode:
                    { exact:
                        "environment"
                    }
                },
          };
        }

        navigator.mediaDevices.getUserMedia(hint).then(function(stream) {
          video.srcObject = stream;
          video.addEventListener("loadedmetadata", function() {
            video.play();

            start(
              container,
              markers["pinball"],
              video,
              video.videoWidth,
              video.videoHeight,
              canvas,
              function() {
                statsMain.update();
              },
              function() {
                statsWorker.update();
              },
              null
            );
          });
        });
      }

became an external script and ypu need only:

// inside threejs_main_worker.html before end of </script></body>

<script src="nftLoader.js"></script>

nftLoader(markers, 640, 480, canvas, video, statsMain, statsWorker);

@kalwalt
Copy link
Author

kalwalt commented Apr 4, 2020

Testing code in my other repository here https://github.com/kalwalt/kalwalt-interactivity-AR/blob/master/nft/nftLoader.js used in this html example, basically you can simply:

<body>
// other html code here
    <script>
      /**
       * APP / ELEMENTS
       */
      var container = document.getElementById("app");
      var video = document.getElementById("video");
      var canvas = document.getElementById("canvas");

      nftLoader(container, video, 480, 640, canvas, "pinball", true);

    </script>
  </body>

@kalwalt
Copy link
Author

kalwalt commented Apr 5, 2020

@ThorstenBux in some machine you can experience "array buffer out of memory" without -s ALLOW_MEMORY_GROWTH=1 what do you think? Does It is better to add in your opinion?

@ThorstenBux
Copy link

I would add it yes. I recall on my iPhone 6s that it breaks without -s ALLOW_MEMORY_GROTH=1

@kalwalt
Copy link
Author

kalwalt commented Apr 5, 2020

I would add it yes. I recall on my iPhone 6s that it breaks without -s ALLOW_MEMORY_GROTH=1

i never experienced this, probably because i have tested only on Android, @nicolocarpignoli instead experienced this and we added in the version embedded for AR.js. I will add for sure.

@kalwalt
Copy link
Author

kalwalt commented Apr 5, 2020

Have you experienced this issue while trying one of my examples here as you said in this kalwalt#42 (comment) ?

@ThorstenBux
Copy link

No I haven't however I'm not sure with which device I tested that.
I recall that inside artoolkitX.js I have allowMemoryGrowth enabled and never had any issues with it which is why I would vote for having it :) .

kalwalt added 2 commits April 7, 2020 18:07
- switched to camera_para.dat

- simplyfied code for Fullscreen

- better end loading
@kalwalt
Copy link
Author

kalwalt commented Apr 7, 2020

@ThorstenBux i added -s ALLOW_MEMORY_GROTH=1 and i improved the NFT examples following your code in ar-magazine:

  • I used camera_para.dat for camera configuration
  • i removed the code involved in canvas styling manipulation
  • a little improvement for the loading ( correct deleting of the div with id="loading" )
  • deleting some calls to a not existent loading class in the body tag.

@ThorstenBux
Copy link

Thank you. I'll check why the build isn't working and merge then.

@kalwalt
Copy link
Author

kalwalt commented Apr 7, 2020

Thank you. I'll check why the build isn't working and merge then.

yes, bitrise is failing for some time, I forgot to tell you.

@kalwalt
Copy link
Author

kalwalt commented Apr 29, 2020

@ThorstenBux bitrise failing because the github-release don't find some of the input variables:

Issue with input: failed to parse config:
- APIToken: required variable is not present
- Username: required variable is not present
- Tag: required variable is not present

@ThorstenBux
Copy link

Some bug there. A PR should never push a release. I'm on it

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants