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

Deformer - Filter #82

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

Conversation

jatinkhilnani
Copy link
Contributor

@jatinkhilnani jatinkhilnani commented Jul 28, 2020

Low-, band- and high-pass filter deformer (#1)

@bmcfee
Copy link
Owner

bmcfee commented Jul 28, 2020

@jatinkhilnani I didn't realize you were still working on this. Another student has also been working on this deformer, though her implementation may be pretty different from yours. Perhaps there's a way to combine them?

@jatinkhilnani
Copy link
Contributor Author

jatinkhilnani commented Jul 28, 2020

Continued on it once PR #80 was merged, to include any further feedback (if any). Should have checked with you first.

Based on the PR #83, the implementation seems more exhaustive as I was not sure of the annotation impact. Two items that I see have to be considered though.

  1. Handling of the cutoff parameter can be made concise and integrated
  2. test_deformers.py has conflict since it does not have modifications from PR Clipping deformer & cosmetic fixes #80

Additional points.
3. I think the three methods for RandomFilter can be merged in one
4. LinearFilter implementation can be taken from this PR

There are benefits of combining the two implementations, should I coordinate with the author then? Please suggest, thanks!

@jatinkhilnani
Copy link
Contributor Author

@bmcfee Should I proceed by taking code from PR #83 and making the suggested modifications along with LinearFilter implementation, or suggest otherwise.

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