Update c++halcon samples

zivid / zivid-halcon-samples

HALCON code samples for Zivid

https://zivid.com

BSD 3-Clause "New" or "Revised" License

13 stars 6 forks source link

Update c++halcon samples #13

Closed chrisasc closed 4 years ago

chrisasc commented 4 years ago

Updated C++ Halcon samples after evaluating capture runtime using different methods.

CMakelists:

updated folder and sample names, they were still set to the old names

CaptureViaGenICam:

added a function to remove 0-value points (NaN points)

CaptureViaZivid:

updated conversion to Halcon to avoid unnecessary copy operations

chrisasc commented 4 years ago

Ready for next round. I am not sure about the comment I made in the code though...

SatjaSivcev commented 4 years ago

Review done. Can you run our C++ CI localy for these programs? Would it work if you just copy them on a local branch (and add stuff to CMakeLists)? Or is it more complicated?

Did you address this @chrisasc ?

chrisasc commented 4 years ago

Review done. Can you run our C++ CI localy for these programs? Would it work if you just copy them on a local branch (and add stuff to CMakeLists)? Or is it more complicated?

Yes, just did it! Ready for (hopefully) last round now

SatjaSivcev commented 4 years ago

I added one commit. Looks good to me. Please answer to one last comment I made. After we figure out what formatting to use we can merge.

chrisasc commented 4 years ago

Update from last two commits:

Ran the two samples through CI -> some minor changes fixed.
Added functionality to use local CI in the future. It does not build the halcon samples (as I dont know how to do that through bash), but It checks clang-format and clang-tidy.
Restructured the repo as we discussed. I think this is a good solution.

Ready for next round! Also,

I added one commit. Looks good to me. Please answer to one last comment I made. After we figure out what formatting to use we can merge.

was this the clang-tidy question Torbjørn answered?

torbsorb commented 4 years ago

It's clang-format which does this. With BinPackParameters: true then the result is

someFunction("GenICamTL", 1, 1, 0, 0, 0, 0, "progressive", -1, "default", -1, "false", "default", zividDevice, 0, 0,
                 &Framegrabber);

I don't like that either. This could have worked if the max line length was shorter in this case. For example with ColumnLimit: 80 (instead of 120):

someFunction("GenICamTL", 1, 1, 0, 0, 0, 0, "progressive", -1, "default",
                 -1, "false", "default", zividDevice, 0, 0, &Framegrabber);

However, that's not a good idea for the rest of the code.

I suggest we keep it as is.

torbsorb commented 4 years ago

I couldn't find a rule that would work selectively on function calls with a large number of (short) arguments.

chrisasc commented 4 years ago

It's clang-format which does this. With BinPackParameters: true then the result is
someFunction("GenICamTL", 1, 1, 0, 0, 0, 0, "progressive", -1, "default", -1, "false", "default", zividDevice, 0, 0,
                 &Framegrabber);
I don't like that either. This could have worked if the max line length was shorter in this case. For example with ColumnLimit: 80 (instead of 120):
someFunction("GenICamTL", 1, 1, 0, 0, 0, 0, "progressive", -1, "default",
                 -1, "false", "default", zividDevice, 0, 0, &Framegrabber);
However, that's not a good idea for the rest of the code.

I suggest we keep it as is.

I agree, it looks a bit stupid on this specific function, but I think in general it looks nice to have one argument per line.

What do you think @SatjaSivcev? Is it okay if I rebase and merge?

SatjaSivcev commented 4 years ago

One last thing. Move 'Procedures' folder into 'hdev' and make sure the path from Readme is correct and also from the hdev scripts. Scripts need to be able ti find the procedure.