ROS2: Image proc non zero crop

[klintan]

As per issue #430 there are some missing nodes from image_proc package that needs to be added.

This is for the CropNonZeroNode.

It is built very very similar to the Rectify and Debayer node, so maybe review should be conjunction with #457 .

[SteveMacenski]

LGTM, is this just a straight port? Anything you want to mention?

[klintan]

LGTM, is this just a straight port? Anything you want to mention?

Nothing in particular, I need to do some more testing. But should be a straight port.

[SteveMacenski]

OK tell me when you're happy with it after testing all the relevent options

[klintan]

Rebased on top of the new master and fixed PR feedback. Fixed some other issues I forgot to handle when testing it. I couldn't find any issues now at least, after testing.

Let me know if I missed something or if you have any feedback on my changes.

[SteveMacenski]

Small stuff but looks fine

[klintan]

Tests failing on image_publisher not sure why, perhaps a temporary Circle issue, I don't have rights to rerun the workflow either. Should not affect this code.

[SteveMacenski]

LGTM

just needs circle ci building

[SteveMacenski]

@klintan pull in ros2 once I have #470 merged. This will fix builds so it should just be whatever changes you've made

[JWhitleyWork]

CI restarted. The error that came up is usually because the compiler ran out of memory.

[klintan]

@klintan pull in ros2 once I have #470 merged. This will fix builds so it should just be whatever changes you've made

Hm I did a rebase on the latest changes, but still failing :/

[JWhitleyWork]

@klintan - I'm testing this locally to see what's up. It's a compiler error but I'm not sure why it's happening yet.

[JWhitleyWork]

@klintan - I created klintan#3, which I think will solve the problem. Please test it.

[SteveMacenski]

@JWhitleyAStuff can you PR that and just merge it? That's been irritating me as well

[SteveMacenski]

I'm happy again when @JWhitleyAStuff is

[JWhitleyWork]

@klintan - Once you rebase on master, I'm good to merge.

[SteveMacenski]

You mean on ros2, right?

[JWhitleyWork]

Yes, sorry.

[klintan]

i'll rebase asap.

[klintan]

@JWhitleyAStuff I had to change
dst.create(src.rows / 2, src.cols / 2, cv::DataType<DstPixel>::type);
to
dst.create(src.rows / 2, src.cols / 2, cv::traits::Type<DstPixel>::value);
per opencv/opencv#10115 , which version of opencv do you run? (I'm on 3.4.5) otherwise it wouldn't build. Perhaps it's something with my env since circleci seems to be fine with that ?

Fixed all other issues. Perhaps the crop_decimate fix should be in a different PR if it's a problem.

edit: circleci does not build with that change, so reverted regardless. But I think it's worth looking into that issue.

[SteveMacenski]

@klintan that's a good use for the preprocessor ifs on the version, there's a bunch of examples in the code for supporting multiple opencv versions that way

[klintan]

@klintan that's a good use for the preprocessor ifs on the version, there's a bunch of examples in the code for supporting multiple opencv versions that way

Ok used a condition for the opencv version, that fixed the issue on circle and locally.

[SteveMacenski]

Just waiting on @JWhitleyAStuff feedback then

[JWhitleyWork]

LGTM!

[SteveMacenski]

@klintan any thing else you want to add before merging?