added circular grid to ronchi and foucault images

[atsju]

closes #332

[atsju]

@githubdoe your blink function has been merged into master.
This new PR is only about the circular grid. This is waht you expected when you wanted to open the PR earlier today :)

Kindly have a look at the build warning in "files changed" tab once all automated builds are finished. They should all be fixable and fixed (except the for loop with floats which can be discussed).
I can have a look if you don't want to fix it.

[github-actions]

Cpp-Linter Report ⚠️

Some files did not pass the configured checks!

clang-tidy (v18.1.3) reports: 1 concern(s)

• foucaultview.cpp:217:5: warning: [clang-analyzer-security.FloatLoopCounter]

  Variable 'currentMM' with floating point type 'double' should not be used as a loop counter

    217 |     for (double currentMM = stepSizeMM; currentMM <= mirrorRadiusMM; currentMM += stepSizeMM) {
        |     ^                                   ~~~~~~~~~                    ~~~~~~~~~
  foucaultview.cpp:217:5: note: Variable 'currentMM' with floating point type 'double' should not be used as a loop counter
    217 |     for (double currentMM = stepSizeMM; currentMM <= mirrorRadiusMM; currentMM += stepSizeMM) {
        |     ^                                   ~~~~~~~~~                    ~~~~~~~~~

Have any feedback or feature suggestions? Share it here.

[github-actions]

Cpp-linter Review

Used clang-tidy v18.1.3

Have any feedback or feature suggestions? Share it here.

[github-actions]

clang-tidy diagnostic

foucaultview.cpp:217:5: warning: [clang-analyzer-security.FloatLoopCounter]

Variable 'currentMM' with floating point type 'double' should not be used as a loop counter

  217 |     for (double currentMM = stepSizeMM; currentMM <= mirrorRadiusMM; currentMM += stepSizeMM) {
      |     ^                                   ~~~~~~~~~                    ~~~~~~~~~
foucaultview.cpp:217:5: note: Variable 'currentMM' with floating point type 'double' should not be used as a loop counter
  217 |     for (double currentMM = stepSizeMM; currentMM <= mirrorRadiusMM; currentMM += stepSizeMM) {
      |     ^                                   ~~~~~~~~~                    ~~~~~~~~~

[gr5]

I think we should leave this code as is. I thought I'd rewrite it as an integer loop like this:

for (int rPx = stepSizeMM / mirrorRadiusMM * maxPixelRadius; rPx <= maxPixelRadius; rPx += stepSizeMM / mirrorRadiusMM * maxPixelRadius) {
    double currentMM = rPx * mirrorRadiusMM / maxPixelRadius;

And we could do the above replacement and it would work fine. I just swapped the 2 variables that are looping through (rPx and mirrorRadiusMM) and fixed the scaling for it to work the other way around. But I think it's clearer the first way and it's fine in this case. If you do % and set the stepping to 10% it will do 10% to 100% circles. There is a tiny chance it will miss the 100% circle. But I tried the existing program and it worked fine. Plus it's no big deal if the 100% circle isn't printed. People know where 100% is (it's the outer edge of the ronchi/foucault).

And for other cases such as stepping by 10mm or stepping by 1 inch etc, it's unlikely to ever end up exactly at the outer edge anyway.

Also my suggested code now has a small bug because we have rounded (or "floored") the step size to the nearest integer so it will understep in many cases. So really it's buggier if we use an integer in the loop.

So @atsju, is there some comment we can add to this line of code to tell it not to worry about a double as a loop counter?

[gr5]

To be clear, I'm leaning to agree with clang on that other complaint of a double in a loop, the one where it went from -1 to 1. I'm going to look at that code probably in the next few days. But in this case I think the double is the better way to do this code.

[github-actions]

🚀 New build available for commit eee6ba2
Download installer here

[githubdoe]

I think the float is just fine. The comparison is not "==" . Yes I know "==" is a problem and why. But this compare is a "<=". I did not create that code the AI did. However if I was writing it I probably would have done the same.

[atsju]

To clarify again. I'm fine with the clang warning that appears in comment section of they are about float comparison as long as you have a small look at it. This is OK. I will check how to remove them from comment to keep warning only in review pane + how to mute the warnings for intentional an verified cases like here.

However, I would like the compilation warning that are only seen in review tab to be fixed.

[gr5]

Oh!

I think this image shows all the warnings nicely. There are 3 connects with lambda functions in them as shown here in QT:

Fixing the "context" warning is trivial - just insert "this" as a new 3rd parameter and leave the rest as is.

Regarding the other warnings, I thought about this for a while and I think it's okay as is. The 3 connects are acting on actions of 3 buttons that can be clicked and then they call a lambda function passing all member variables many of which are referenced and may not exist if a user closes the window.

But you can't click on a button after the window is gone so it should be fine.

HOWEVER
I created a fix. Should I push it @githubdoe? I moved the temporary storage "colors" into the class as a member variable that stores the colors that the user chooses temporarily - only to be moved to more permanent storage if the user hits ok (versus cancel).

So 5 lines of code change. But really I should probably also refactor "colors" to "m_colors_temp" or something like that but that means like 15 lines of code change. What do you guys think? Here's my change that works:

and in foucault.h