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.

Sign up for GitHub

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

Jump to bottom

Adds canny edge detection #87

Merged
merged 3 commits into from
Feb 8, 2019
Merged

Adds canny edge detection #87

merged 3 commits into from
Feb 8, 2019

Conversation

joelgallant
Copy link
Contributor

Attached to Mat instead of making it a function with an src, dst signature, seemed like that is the preference in this lib.

joelgallant
joelgallant commented Jul 10, 2018
View reviewed changes
src/lib.rs Outdated
fn into(self) -> Result<T, String> {
if self.error.value.is_null() {
Ok(self.value)
Ok(self.value.into())
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unnecessary change, but I think this makes sense?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the PR on the first place.

I follow yagni principle so I think we don't need to add it until it's not required somewhere.

@Pzixel
Copy link
Collaborator

Pzixel commented Jul 10, 2018

The format checking is failing. You are probably uses invalid cargo fmt. You need have exactly same version, because they are a bit incompatible with each other. I will create a PR with fmt update because it's a shame.

@Pzixel Pzixel mentioned this pull request Jul 10, 2018
Merged
@Pzixel
Copy link
Collaborator

Pzixel commented Jul 10, 2018

Merge latest master. It should do the trick

@joelgallant joelgallant force-pushed the canny-edge-detection branch from dfca5c8 to 1ed46f8 Compare July 14, 2018 19:29
@Pzixel
Copy link
Collaborator

Pzixel commented Jul 14, 2018 •
edited
Loading

CI currently uses rustfmt 0.8.2-nightly (5e59925 2018-07-02). You can check your version via cargo fmt --version. If your version differs then your code may not pass.

Don't forget to merge latest master to get correct .travis file. It was recently changed.

Sorry if it looks complicated, when you get familiar with it it won't take this much time.

@joelgallant joelgallant force-pushed the canny-edge-detection branch from 59f685e to cc7fbd9 Compare July 14, 2018 20:48
@joelgallant
Copy link
Contributor Author

Oops, should have checked first. Re-pushed with correct fix. (and is rebased on master)

@Pzixel
Copy link
Collaborator

Pzixel commented Jul 14, 2018

Okay, now cargo fmt check passed, and it failed on clang one (we are using clang-format-5 if I remember correctly). And if you don't mind I'l post some some review comments.

P.S. Sorry for this much iterations, I think I just had to link out contributing readme which contains all required links.

Pzixel
Pzixel requested changes Jul 14, 2018
View reviewed changes
native/imgproc.cc Outdated Show resolved Hide resolved
tests/test_imgproc.rs Show resolved Hide resolved
@joelgallant joelgallant force-pushed the canny-edge-detection branch from cc7fbd9 to ef9e931 Compare October 5, 2018 16:43
@joelgallant
Adds canny edge detection
7143d86
@joelgallant
Addresses PR feedback
a81f45a 
- Adds an EmptyResult type and corresponding CEmptyResult
- Adds error case for edge detection test
@joelgallant joelgallant force-pushed the canny-edge-detection branch from ef9e931 to a81f45a Compare October 5, 2018 16:45
@joelgallant
Copy link
Contributor Author

joelgallant commented Oct 5, 2018 •
edited
Loading

Okay, I've updated this PR with a failing test and returning an EmptyResult @Pzixel

@Pzixel
Copy link
Collaborator

Pzixel commented Feb 8, 2019

sorry, my new year finally ended :)

Thank you for your PR.

@Pzixel Pzixel merged commit b602103 into nebgnahz:master Feb 8, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Pzixel Pzixel Pzixel requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@joelgallant @Pzixel