Skip to content

Add cv.seamlessClone#614

Merged
justadudewhohacks merged 5 commits into
justadudewhohacks:masterfrom
foxxyz:feature/seamlessclone
Aug 22, 2019
Merged

Add cv.seamlessClone#614
justadudewhohacks merged 5 commits into
justadudewhohacks:masterfrom
foxxyz:feature/seamlessclone

Conversation

@foxxyz

@foxxyz foxxyz commented Aug 16, 2019

Copy link
Copy Markdown
Contributor

This pull request adds bindings for seamlessClone.

I'm on the fence whether this should also exist as a class method of cv::Mat or only as a global function. This pull request only implements the latter, due to:

  • Other bindings in the photo namespace do not have any cv::Mat bindings
  • seamlessClone takes multiple cv::Mat as input parameters and it would be confusing to figure out which one is being passed in.

Would appreciate your direction on whether class method bindings are also necessary - I'd be happy to add them if so.

Also let me know if you'd prefer additional tests.

@justadudewhohacks

Copy link
Copy Markdown
Owner

Thanks for the PR! In general looks good to me.

If you could refactor the worker to use the new CvClassMethodBinding as GaussianBlur does for example, that would be great! This also allows you to simply export this method as a class method as well as a global method, as described in the contribution guide.

As you figured out, there are no MatPhotoBindings files yet, but you can simply add them.

@foxxyz

foxxyz commented Aug 21, 2019

Copy link
Copy Markdown
Contributor Author

Thanks for the quick and detailed feedback!

I've made the changes as suggested - let me know if you'd like to see additional tests or have any other feedback before merge!

@justadudewhohacks

Copy link
Copy Markdown
Owner

Perfect thanks! Since you now added the class binding, could you also add the seamlessClone typings to Mat.d.ts.

After that it's ready to merge. :)

@foxxyz

foxxyz commented Aug 22, 2019

Copy link
Copy Markdown
Contributor Author

Thanks - I knew I forgot something...! All done.

@justadudewhohacks

Copy link
Copy Markdown
Owner

Awesome!

@justadudewhohacks justadudewhohacks merged commit e9ad6e5 into justadudewhohacks:master Aug 22, 2019
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