clmtrackr icon indicating copy to clipboard operation
clmtrackr copied to clipboard

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

Open serapath opened this issue 7 years ago • 6 comments

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:

  • https://github.com/mccoder/clmtrackr
  • https://github.com/michaelmausler/clmtrackr

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.

serapath avatar May 11 '17 20:05 serapath

https://github.com/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.

mccoder avatar May 12 '17 17:05 mccoder

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.

  • What is this about?
  • Wouldnt it be better to make those changes in https://www.npmjs.com/package/jsfeat or otherwise at least fork the project?

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

serapath avatar May 12 '17 18:05 serapath

Just figured out, that the original jsfeat lib seems to be https://github.com/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:

  • https://github.com/inspirit/jsfeat/network

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):

  • https://github.com/sminogue/jsfeat
  • https://github.com/atomicguy/masque
  • https://github.com/zodiac/jsfeat
  • https://github.com/ndrewh/jsfeat
  • https://github.com/Orange-OpenSource/jsfeat
  • https://github.com/Quasimondo/jsfeat (some forks are older, but might contain some relevant changes?)

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

serapath avatar May 12 '17 19:05 serapath

@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 avatar May 12 '17 20:05 mccoder

require('jsfeat') this better.

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

mccoder avatar May 12 '17 20:05 mccoder

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:

  • https://github.com/Orange-OpenSource/jsfeat/commit/4f8656fa4be8b65a9f75f84b9093143caceaf8a7
  • https://github.com/Orange-OpenSource/jsfeat/commit/c52e5925f25dd2b1329bb168c01f17f8fa4348e3
  • https://github.com/Orange-OpenSource/jsfeat/commit/3dcff02e185f5b84d9d63e7d172d3f747a30ab25 And maybe also this:
  • https://github.com/sminogue/jsfeat/commit/461bfed5acd70a477d3f3d800c97d17c49e66871

~~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:

  • https://github.com/inspirit/jsfeat/pull/67
  • https://github.com/inspirit/jsfeat/pull/18

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.

serapath avatar May 12 '17 21:05 serapath