nebgnahz / cv-rs

Rust wrapper for OpenCV (manual at this point)
https://nebgnahz.github.io/cv-rs/cv/
MIT License
203 stars 41 forks source link

WIP: Add Background Subtractors. #106

Open yuvallanger opened 5 years ago

yuvallanger commented 5 years ago

Following up on issue #105. Does it look sane to you? Please do not pull yet. I want to add more of its methods.

Pzixel commented 5 years ago

So far so good.

P.S. Hint of the day: if you aren't ready yet start PR name with WIP: 🙂

yuvallanger commented 5 years ago

Sorry for imposing. Got advice on writing tests? @Pzixel

Pzixel commented 5 years ago

just write them. I'm not sure, there is not special advices here. Setup-act-assert, classic loop

yuvallanger commented 5 years ago

Biggest patch I had ever wrote. What do you think, @Pzixel? I wrote some tests, too!

Pzixel commented 5 years ago

Looks great.

I can't look at it right now, but I gonna come back later.

P.S. Check for formatting, because some of changes are just format ones, and CI server won't allow it ;)


Why so much force pushes? I'l squash all commits into one when merging. It's much safer and more convenient. You can have buzillion of commits and they all becomes one.

yuvallanger commented 5 years ago

Looks great.

I can't look at it right now, but I gonna come back later.

Thank you.

P.S. Check for formatting, because some of changes are just format ones, and CI server won't allow it ;)

I didn't realize that. I will look and see what's what tomorrow.

Why so much force pushes? I'l squash all commits into one when merging. It's much safer and more convenient. You can have buzillion of commits and they all becomes one.

The following is one part thinking out loud and one part asking for advice:

I will create another branch to work on, then merge with this one. I don't know if I should keep, squash, or fixup all the tiny bug fixing commits along the way. Maybe I should just let it accumulate and decide right at the end. I will commit fixup ones with message "f".

Pzixel commented 5 years ago

Just keep pushing commits as they go. There is no shame having ~30-40 commits with no sane message since they anyway will go away. Example from my private repo

image

becomes

image

When merging

yuvallanger commented 5 years ago

I do not understand the appveyor test failure. Locally it is running fine. My OpenCV version is 3.4.5.

Got any ideas on how to find the reason it had failed?

Pzixel commented 5 years ago

Look at output:

    Running `C:\projects\cv-rs\target\debug\deps\test_video_analysis-3a0f73e01162fd28.exe`
running 6 tests
test knn_tests::test_create_background_subtractor_knn_apply ... ok
test knn_tests::test_create_background_subtractor_knn_default ... ok
test mog2_tests::test_create_background_subtractor_mog2_apply ... ok
error: test failed, to rerun pass '--test test_video_analysis'
Command exited with code -1073741819

Considering it's only failing on Windows it's some windows-specific problem. Either it's something windows can't run (then it should be hidden via cfg) or there is some bug linux is not affected of.

yuvallanger commented 5 years ago

Should I add something like #[cfg(not(windows))]? Wouldn't that mask the missing coverage?

Pzixel commented 5 years ago

Firstly you should figure out what's wrong on Windows. It's like 99% chance that you're doing something wrong: e.g. if you do Command::new("curl") to make an HTTP request it will work in linux, but won't on windows.

yuvallanger commented 5 years ago

The furthest I can go in making it work on Windows is generalizing the path. I don't have a Windows machine I can use.

Pzixel commented 5 years ago

I wouldn't be able to run it on windows till next week.

You probably could check it on windows VM.

In worst case we have to configure it out via cfg and open "up to grabs" issue, but I hope it won't be required.

vadixidav commented 5 years ago

I can follow through on this PR after #117 is merged.