Average profile

[githubdoe]

Added different way to create average profile and display it with right click menu.
Added other things to right click menu of profile plot.
Added right click menu of Ronchi image to create image of multiple ronchi images.
Added if you move the avg or single profile it will also move the 16 diameters profiles once they are displayed.
Profile offset will happen whenever you select different wave fronts.

[gr5]

Oh. There are merge errors. This is related to my recent PR where I added the "origin" enum to a few functions. I can do the merge if you want Dale. I also marked this PR as draft. Search this web page for "draft" to see the link to switch it back.

[githubdoe]

Go ahead.

[gr5]

I did the merge. It was much easier than I had expected. It builds in QT5, but there is a QT6-only build error. I don't have QT6 installed on my laptop and I don't understand the error, so if @atsju could look at this error and fix it? Or I can fix it Saturday.

profileplotpicker.cpp: In member function ‘void profilePlotPicker::move(QPoint)’:
Error: profileplotpicker.cpp:152:39: error: invalid use of incomplete type ‘const class QwtText’
  152 |     emit offset(d_selectedCurve->title().text(), delta);
      |                 ~~~~~~~~~~~~~~~~~~~~~~^~

By the way, I love the new GUI for profile plot. I played with it for 15 minutes solid, lol. I don't like Dale's change to menu item "tools "stop auto invert".

Dale you should do a "git checkout averageProfile" then "git pull" either now or after this PR is complete although at that point you can just pull master.

[githubdoe]

Those were my temporary changes to keep the dialog from popping up until you had your permanent fix.

[githubdoe]

The stop auto invert is leftover from much earlier time when there was no invert dialog. It may no longer be needed.

[githubdoe]

I also forgot to mention the new profile line width items and fix to the color settings of the profile lines in the preferences.

[githubdoe]

Profile offset reset will happen whenever you select different wave fronts.

[gr5]

The stop auto invert is leftover from much earlier time when there was no invert dialog. It may no longer be needed.

I'm not sure if it's needed for most users but for me it's very helpful - well maybe only for testing. But I use it a lot. For testing. And it's kind of hidden in the menu system so I think it's fine.

[githubdoe]

I have pulled the code and it compiles. I see the square contour plot but do not see the vertical blue bar for the right side. It will still drag that control however.

I just realized an addition I want to add. That is overlay one ronchi with another to see how they are different. I bet AI can help me with that as well.

[gr5]

not see the vertical blue bar for the right side. It will still drag that control however.

That's not merged yet. It's in a PR that still needs approval.

Also the compiler error is only for QT6. I don't understand what the cause is and hope that Julien fixes it but if not I'll look at it Saturday. Julien may be celebrating Christmas right now.

I just realized an addition I want to add. That is overlay one ronchi with another to see how they are different. I bet AI can help me with that as well.

I prefer that you do those edits in any branch you want but if you do them in the averageProfile branch then they will get added to this PR and I prefer any changes related to a ronchi overlay feature to go into a seprate PR. So please don't push new changes to averageProfile until this PR is merged. But if you do we'll just enlarge the scope of this PR.

[gr5]

Also tomorrow I'll be quite busy with visiting family so may not notice anything in my emails nor anything github related. 😄

[atsju]

I think I fixed the build. I'm more busy than expected so I don't know how much time I have available.

Dale, you will need to pull the branch to avoid conflicts. As George explained: kindly avoid pushing new features to this branch until it's merged. It should probably already have been split into 2 or more PR but I don't want to refrain you developping so we will just take care of it as it is.
I will make a more formal code review as soon as I have more time.

Happy Xmas to you 2 :)

[github-actions]

🚀 New build available for commit 90db809
Download installer here

[githubdoe]

I would remove the ifdef notnowold. But then you would get my new feature as well.

[atsju]

I would remove the ifdef notnowold. But then you would get my new feature as well.

I'm not sure to understand.
If you are talking about the warning, the ifdef is not the cause. The problem is you have 2 macro after ifdef instead one. Compiler might not complain but it's not language compliant.

If you want to remove it and talk about "no new feature", go ahead and remove, your new feature is aleady in the code and compiled by default. Removing the old code is part of this PR. What we want to avoid is getting more unrelated features.

Just remember to pull the branch now to avoid conflicts. As George and I pushed changes

[gr5]

I would remove the ifdef notnowold. But then you would get my new feature as well.

Noted! I only played with the profile stuff. I didn't look at the Ronchi stuff yet. I didn't look at the code changes yet either. I'll try removing that ifdef and rebuild. But not today.

[atsju]

I did not test. Only code review.

[atsju]

I think this code and maybe more comes from when you modified the UI to show us a printscreen in discussion about the autoInvertDialog. But it seems unused. Should be removed.

[githubdoe]

Yes that code is my temp fix. It should not be merged.

[atsju]

magic number ? Needs to be settable in UI ?

[githubdoe]

Same as previous comment. Except filled in from the preference setting.

[atsju]

sorry I don't get this one. What one is "previous" ?
Does code need change so that 550 can be set in preference settings ?

[githubdoe]

That value and all values in the new settings structure generated by AI are values that will not be used by current code. The real values come from foucaultview UI and the 550 is the output wavelength specified in the preferences under the configuration menu. 550 is the wavelength of green light in nanometers. I'm impressed that AI figured out a value for [t there even though it did not have access to the code where that value is set.

AI generated those default values which would be used only if we ever write code outside the foucaultview.cpp file to generate ronchi images where we don't have access to the foucaultview UI.

[githubdoe]

I think nearly all the changes to Foucaultview.cpp and .h are AI generated. I looked it over just a small amount and then just tested to see if it did what I wanted. For me it is not worth the trouble to modify any of it's decisions. I like what it produced. Julian my have other ideas and can run them by me but I don't really want to have to spend any time on it.

[atsju]

Hi, sorry for delay, I haven't had much time available.
I closed some of my initial review comments. I can do the fix for most of them.

Remains for you :

• this one Average profile #309 (comment)
• I'm dense but I don't understand if the magic 550 value is something for the futur that cannot be changed right now (use a #define or better yet, use a constexpr) OR if this value is supposed to comme from UI but has not be linked to UI. To explain differently: is the 550 used ? if it's used, does it need to be settable ? Is the code finished ?

I will do review of added code later.

[githubdoe]

Concerning the 550.

There is now a new routine that users can call with this structure that makes ronchi images. The old code that made ronchi images has been refactored into using that new make ronchi image code. That new routine uses this structure with the 550 in it.

The caller can use those default values if desired however the refactored code fills them in and in the case of the 550 it fills that in from the preferences data. Go look at the code that references that structure and you will see. Using Qt Creator it is easy to find all code that references something using a right click on the highlighted something.

One way that structure could have been written is with no default values. Then there would be no 550. However AI elected to fill it with reasonable default values. If you think you need it put a comment out beside the 550 that describes it as the output wave length in nanometers.

[github-actions]

🚀 New build available for commit ed69f2b
Download installer here

[atsju]

Concerning the 550.

There is now a new routine that users can call with this structure that makes ronchi images. The old code that made ronchi images has been refactored into using that new make ronchi image code. That new routine uses this structure with the 550 in it.

The caller can use those default values if desired however the refactored code fills them in and in the case of the 550 it fills that in from the preferences data. Go look at the code that references that structure and you will see. Using Qt Creator it is easy to find all code that references something using a right click on the highlighted something.

One way that structure could have been written is with no default values. Then there would be no 550. However AI elected to fill it with reasonable default values. If you think you need it put a comment out beside the 550 that describes it as the output wave length in nanometers.

OK it's clearer. I had a look and actually all the 550 from these modifications are never loaded from settings as you think.
QtCreator probably mixed up OpticalTestSettings.outputLambda from the structure and the variable with same name double outputLambda; defined in surfacemanager.cpp
The whole usage of outputLambda is quite convoluted as you need to trigger ui updates when value is changed.

So yes I think this code is wrong in some places. While it's correct to do double outputLambda = 550.0; in foucaultView.h line 33,
it's not OK in fouclaultview.cpp lines 294 and 362. For the latest, it shall load from settings.

Let me know if I missed something

[githubdoe]

output_lambda good catch. The original code actual uses an external variable that is set by other things and was the awkward way I got that value to foucaultview. Yes the current code is wrong.

I have now fixed that in my version

[gr5]

I have now fixed that in my version

Great, can you commit and push that change? You will have to do another pull first but I doubt there are conflicts:
git pull averageProfile
git push averageProfile

[gr5]

I'm very close to approving this code. I want Dale's latest fixes and then I'm ready to approve, create a release, and do lots of testing. It would be nice to do the above two suggestions as well as that gets rid of some unused code.

[github-actions]

🚀 New build available for commit 1c4001a
Download installer here

[github-actions]

🚀 New build available for commit f7de6f0
Download installer here

[gr5]

I think it's fine to merge this now. Should I go ahead and just click the button? Or does someone else want to add anything first? I think this is ready. It's not thoroughly tested by me but I'll test it more thoroughly when we do a release.

[githubdoe]

Ok with me..