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

This is an awesome project. I want to contribute :-) #104

Open
serapath opened this issue May 11, 2017 · 6 comments
Open

This is an awesome project. I want to contribute :-) #104

serapath opened this issue May 11, 2017 · 6 comments

Comments

@serapath
Copy link

serapath commented May 11, 2017

Hello auduno.

Luckily it's possible to open issues on this repository. Most forks don't allow this.

I just checked the "network" of the repo and i found the following very recently active forks:

Both authors seem to be after the same thing. Better module support, so that it would work nicely as a commonjs dependency in other projects.


In the last days I was working on a very large and serious refactoring of the code base.
I did not touch the core parts at all, because i honestly don't have a clue how this module is doing what it's doing, but it's good at it :-)

The "refactoring" might be a bit too massive to be an ok pull request so i might just publish it as it's own module. ...but then again, i would like to receive future updates and it would be a little bit of maintenance work to always pull the latest features into my fork so i was wondering if you would be open in theory to merge such a serious refactoring.

I would also be willing to adapt my changes to make them "mergable" if you want.

@mccoder
Copy link

mccoder commented May 12, 2017

https:/mccoder/clmtrackr

My repository ready to Pull Request.
I fixed Worker which crashed browser. And my repository supported modules.
but i don't now wthat do with file package.json
Change name of npm module before PR on not.

@serapath
Copy link
Author

serapath commented May 12, 2017

hey, yes i saw it.
i first based my pull request of of your repo, but figured that i want a more serious refactoring and get rid of grunt and other stuff - also there were not a lot of changes yet, but now it seems your fork is again the most recent version and has some more serious changes.

I will try to rebase my fork and changes on your branch i guess. One question - I try to add dependencies from npm instead of having a copy of certain projects in the repo.

You seem to have edited js/jsfeat_detect_worker.js and js/jsfeat_detect.js.

If you want to see my current progress, it's here: https:/serapath-contribution/clmtrackr

@serapath
Copy link
Author

Just figured out, that the original jsfeat lib seems to be https:/inspirit/jsfeat
Maybe that's what is published here https://www.npmjs.com/package/jsfeat or maybe not.

The library has a network where I am not sure what is the most up2date version, see:

Maybe it's worth making a fork and publishing it to npm - but it might be worth contacting the library author first? :-) Here are the forks that seem to have some changes (don't know if relevant):

I know i would like to require('jsfeat') and not to have a javascript file copied into clmtrackr itself - what about you?

@mccoder
Copy link

mccoder commented May 12, 2017

@serapath My last commit not atomic.
It contains merge from main repo. And fix Worker after pull request #83

changes in js/jsfeat_detect_worker.js
In order to use only one Woker I decided that jsfeat't have to initialize every time. So I carried initialize it outside the method self.onmessage. It left only the code that is responsible for the calculations.

Changes in js/jsfeat_detect.js

Since the Declaration of the method "self.onmessage" moved to a function, changed method of function call.

   if (useWebWorkers) {
      //Courtesy of stackoverflow		      //Courtesy of stackoverflow
    Worker.createURL = function(func_or_string){		 +      Worker.createURL = function (func_or_string) {
        var str = (typeof func_or_string === 'function')?func_or_string.toString():func_or_string;		 +          var str = (typeof func_or_string === 'function') ? '(' + func_or_string.toString() + ')(self)' : '  self.onmessage = ' + func_or_string;
        var blob = new Blob(['\'use strict\';\nself.onmessage ='+str], { type: 'text/javascript' });		 +          var blob = new Blob(['\'use strict\';\n' + str], {type: 'text/javascript'});
        return window.URL.createObjectURL(blob);		 +          return window.URL.createObjectURL(blob);
    }
 };

Initialize single worker

if (useWebWorkers) {
       this.singleWorker = Worker.create(findFaceWorker);
        this.singleWorker.addEventListener('message', function (e) {
            this.faceDetected(e, this.lastCallback);
            this.lastCallback = null;
        }.bind(this), false);
 }

Retain callback before each call, since it is impossible to bring in a Worker
this.lastCallback = callback;

@mccoder
Copy link

mccoder commented May 12, 2017

require('jsfeat') this better.

I just don't understand why were included unsupported project.

@serapath
Copy link
Author

serapath commented May 12, 2017

my fork is still "Work in Progress".

I would like to remove grunt and for example make clmtrackr instead use var jsfeat = require("./jsfeat")
Even better for me would be require('jsfeat') and add all changes you already did to jsfeat to that project instead. If the author of jsfeat doesn't respond or doesn't want to merge, i would publish another module instead.


I checked all forks and found two which had changes not included in the main jsfeat yet.

Can you take a look at:

Do you think these are optimizations that should be added into jsfeat in clmtrackr?


UPDATE:

oh, i just saw, that those authors actually made pull requests which are the only ones still open:

I just tried to reach out to the author on twitter https://twitter.com/serapath/status/863139445184225280

If they don't respond, maybe it's possible to fork jsfeat and maybe merge the pull requests from above AND add your changes too, publish it to npm and then require it as a dependency in clmtrackr.

If you agree with this approach I would do that after waiting for maybe 1 day or so for the author to respond.
It's still possible to afterwards make a pull request so the original author of jsfeat can merge everything if he wants.

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

No branches or pull requests

2 participants