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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Count extended fingers & build path modification #1

Merged
merged 2 commits into from Jul 25, 2018
Merged

Count extended fingers & build path modification #1

merged 2 commits into from Jul 25, 2018

Conversation

nathanshelly
Copy link
Contributor

Hi,

I've been using your patch for a school project and have found it incredibly valuable, thank you so much for making it! If you're interested here are two commits that I think would make using the patch a slightly nicer experience. The two are completely unconnected and can be cherry picked if you would like one but not the other (assuming you want either at all). A little more detail on each commit:

  1. Count extended fingers only - currently the numfingers value output by the patch is always as many fingers as one has on their hand (for me always five, don't have any friends with less fingers to test with 馃槃 ). I needed to count the number of extended fingers in the course of my project so I added extended() to the patch to do so (apologies if you're aware of this and purposely wanted total number of fingers vs extended).
  2. Generate target in repo root - changes the target path to generate the patch at the root of the repo. This one is definitely opinionated, I found it slightly confusing that the build process generated a target outside the repo and in a max5 folder. I think generating it to the root might be a little more clear to users what they've generated.

Thanks again for making such a helpful tool! Please let me know if there's anything you'd like from me to help this pull request go through!

@nathanshelly
Copy link
Contributor Author

Closing since you haven't expressed any interest in this, if at a later point you'd like to implement these would love to talk then!

@JulesFrancoise
Copy link
Owner

Hi @nathanshelly

My apologies, I didn't get a chance to look into it yet!
I reopened the PR to keep it in mind and will try to have a look.

Thanks!
Jules

@nathanshelly
Copy link
Contributor Author

No worries whatsoever! Just figured I wouldn't leave an open ended PR if you were busy with other things 馃槂. If you get a chance to have a look and have any questions please let me know!

@JulesFrancoise JulesFrancoise merged commit 7eb969a into JulesFrancoise:master Jul 25, 2018
@JulesFrancoise
Copy link
Owner

It looks straightforward so I merged your PR.
I will add a new release when I get my hand on a leap motion (I don't have one at the moment).
Thanks for your contribution!

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.

None yet

2 participants