webcamjs icon indicating copy to clipboard operation
webcamjs copied to clipboard

modularized

Open positlabs opened this issue 9 years ago • 9 comments

Resolves #63. Actually resolves #45. Related to #47.

Requires browserify to build. Install with npm install -g browserify, then run tests/modularize/build.sh.

The test uses commonjs modules, but it should behave the same across module systems if the wrapper works correctly (it seems to). The test creates four instances of the webcam; two of rtc and two of flash. No problems, so far.

It's a breaking change, so might be something for the next version. Docs and stuff will need updates.

Give it a spin and let me know what you think!

screen shot 2015-04-08 at 12 06 27 pm

positlabs avatar Apr 08 '15 19:04 positlabs

Wow, this is a huge change! You've basically rewritten the entire library, LOL. Either that, or GitHub just can't make heads or tails of the diff. It looks like virtually every line is changed.

Since I don't use any module loaders, this is a difficult pill for me to swallow. I don't want to have to use npm and browserify to install my library. I just want to drop the files into place and have it work. I guess I'll have to think about this. Perhaps for v2.0?

Seriously tho, thank you for all the time you put into this. This is a huge amount of work for a free, open source library. I have half a mind to just merge the PR, but I'm really afraid this is going to break everyone's implementations, and I'll have to spend hours learning about how these module loaders work, rewrite the docs, fix all the test pages, etc. etc. And then if people have problems installing or using it I won't be able to help. It's just going to be a huge time investment that I just can't make right now.

But I will absolutely make it my goal to learn about these module loaders for v2.0, and then merge this PR, or code up something similar.

Thanks again! I'm leaving this thread open.

  • Joe

jhuckaby avatar Apr 09 '15 16:04 jhuckaby

This change doesn't require you to use a module loader. It's totally optional. I wrote the test with modules because I wanted to be sure it worked. The module loader bit didn't change at all, actually. I just moved some things around to allow instance creation, and modularity by proxy. Any problems surrounding the module compatibility code will have existed before this change (it's pretty simple, and I don't think it's cause for concern).

I moved all of the functions into Webcam.prototype, and all of the instance vars get added in the constructor. I think too many big blocks got moved, so the diff is useless. The meat of the code didn't change much.

The main breaking change is how the webcam is set up. Instead of a singleton (singleton methods no longer exist, btw), you will create instances like this:

var webcam = new Webcam({...params});

Each instance needs an external interface method for flashNotify. Webcam.flashNotify doesn't work with modules or instances because the singleton no longer exists. To solve this, each instance creates a uuid-named method on the window for the EE, and pointed it to the instance's flashNotify.

Thanks for considering the merge. If it helps, I can watch this repo and respond to github issues as they arise.

positlabs avatar Apr 09 '15 17:04 positlabs

Okay, thanks for the explanation, and sorry for my misunderstanding. I'll do some more research, so I can properly understand these changes. I have to say tho, this feels like a v2.0 rewrite to me, so maybe I'll just start over from scratch, and take into account multiple cameras and module loaders from the get go.

Thanks again for all your help!

jhuckaby avatar Apr 09 '15 18:04 jhuckaby

Yeah, it's definitely something for a major release.

For now, though, I recommend removing the module-compatibility block because it breaks the flashNotify interface.

positlabs avatar Apr 09 '15 18:04 positlabs

I'm hoping this commit will help with that:

https://github.com/jhuckaby/webcamjs/commit/0f1cf7376489c1a3d24a07238d2538a937483295

jhuckaby avatar Apr 09 '15 18:04 jhuckaby

Hi this is #47 again

I was able to follow your guide

  • npm install -g browserify
  • run tests/modularize/build.sh. and run the tests/modularize/index.html

The Trouble I got is this:

The first two createWebcam with 'force_flash: true' option (line 29 ~ 39 of your index.js file, displayed below) capture the same images, ie images are taken by the same webcam, even I selected different webcams.

createWebcam({ force_flash: true, width: 320, height: 240, });

createWebcam({ force_flash: true, width: 320, height: 240, });

Would you please give me an advice? Thanks a lot.

myongjailee avatar Jan 09 '16 04:01 myongjailee

this is before clicking the snap buttons capture

and this is after click the snap buttons capture2

myongjailee avatar Jan 09 '16 04:01 myongjailee

I hadn't tried multiple webcams, but it should be possible. You will need to update the snap method. You can either cache a reference to the flash movie element, or assign a unique ID.

https://github.com/jhuckaby/webcamjs/blob/master/webcam.js#L367-L373

positlabs avatar Jan 10 '16 23:01 positlabs

@myongjailee how did you manage to use 2 webcams?

EdgarSantiago93 avatar Apr 18 '17 17:04 EdgarSantiago93