Reviewed wavefront code

[atsju]

See issue #62 for details.
I only cleaned the manual destructor and added init for some missing fields of constructor.

I decided to not do a deep copy inside move constructor because it could have performance impact. I'm supposing Dale did it like this on purpose.
Cleaner solution would be to create explicit functions "deepCopy" and "lightCopy" but as it works I prefer not to touch.

code is documented so just be careful

    wavefront( const wavefront &wf); // copy constructor doing deep copy of cv::Mat
    wavefront( wavefront && ) = delete;	// move constructor, deleted because unused
    wavefront& operator=( const wavefront & ) = default; // copy operator not doing deep copy of cv::mat
    wavefront& operator=(wavefront &&) = delete; // move operator, deleted because unused

consider this close #62

[github-actions]

🚀 New build available for commit aafad68
Download installer here

[gr5]

This makes me nervous (line 49 - the regions copy)

1. Does it do a deep copy?
2. Previous behavior you got a regions member that was empty and ready to be filled with fresh regions. Does the existing code expect this everywhere or does it not care? Did you check all the times he adds regions and might we get doubled regions?

[gr5]

On the other hand this might fix some bug I didn't really know was easy to fix that I forgot about. Maybe regions can get lost after certain operations and maybe that's bad.

[atsju]

Thank you for your review; your comments are always spot on.
The copy constructor does deep copy of the matrix. it would be very strange to not deep copy everything else. Or one should create a dedicated function.

1. Yes it does a deep copy (not immediately but if it ever gets modified) https://doc.qt.io/qt-6/qlist.html#QList-2
2. I have not found any place where there is risk of reusing old data that shouldn't.

My conlusion : this modification shall not introduce a bug. If it ever does it will just confirm that code relied on some sort of "side effects".

Now that I looked deeper in the code I think it might be worse it to make the copy constructor =default it won't do the deep copy anymore so i will touchup the code where required (I believe it's only 2 places). Everything will be much more explicit and we will rely on 0 side effect.

[atsju]

Ok in addition I also confirmed it does not fix any bug...

[github-actions]

🚀 New build available for commit 29b9bee
Download installer here

[atsju]

Hi @gr5 ,

I dug a bit more on this topic. Today this is the curretn usage of copy
Copy assignment paths (shallow today)
surfacemanager.cpp:1852 in rotateThese
surfacemanager.cpp:1924 in subtract
surfacemanager.cpp:3256 in resize
surfacemanager.cpp:3280 in changeWavelength
surfacemanager.cpp:3299 in flipHorizontal
surfacemanager.cpp:3324 in flipVertical
zernikesmoothingdlg.cpp:118 in dialog working copy refresh
Copy constructor paths (deep today)
surfacemanager.cpp:2344
surfacemanager.cpp:2389

And I see 2 solutions:

1. make copy constructor =default this will make it a shallow copy. and there are 2 places where I will add explicit clone.
   Positive : Class is consistent: copy always means shallow copy.
   Negative : copy is shallow copy which is not the best choice in this case. Wavefronts are never meant to be shared so copy operator should be deep.

2. delete copy constructor and operator. Replace with functions shalowCopy and deepCopy.
   Positive : everything is much more explicit and maintainable
   Negative: slightly more code change

3. Of course : don't touch what isn't brocken => default destructor is still recommended.

Could you advise ?

[gr5]

I love #2 where you have "shallowCopy()" and "deepCopy()". To me that makes things so much more clear. I do not like using libraries where this is hard to figure out. Does opencv already have 2 functions: "copy()" and "clone()"? I don't know which is which if so. I prefer the shallow and deep explanation. To me it's clear.

@githubdoe - what do you think? leave it as is but add comments that say "shallow copy"? Or create 2 functions to make the code more clear?

[githubdoe]

Touching that is very dangerous. Pick option #3. Unless you find something broken for the user.

George. At one time OpenCV documentation was very clear that when working with cv::Mat an "=" and the like did not do a deep copy and the clone() was the way to make a deep copy. Now it is harder to find the documentation with AI intervention and not showing it. Wavefront tired to be memory and CPU efficient when working with them and passing them around.

Changing how that works is a huge risk.

[gr5]

Changing how that works is a huge risk.

Well @atsju looked at it pretty carefully so I'm not too worried. I would recommend as a minimum commenting where it's not clear which are deep and which are shallow copies. The first line of code mentioned by @atsju above is this:

*wf = *oldwf; // copy everything to new wavefront including basic things like diameter,wavelength,origin

does not use "clone" yet it does a deep copy.

I'm the one who added that comment. About a year ago.

Perhaps my mention of "clone()" was a distraction as we are talking about class wavefront, not cv::mat which is what I was talking about just above.

[atsju]

Well typically this one comment is probably incorrect. The line = is just a shallow copy. Not a deep copy. The rest of matrixes is copied later but not here.

My proposal is really just rename the 7 occurrence of = to shalowCopy()
And the 2 =constructor to deepCopy().

While I agree there is 0 bug today, this renaming would be without risk and should -in my opinion- ease maintenance and code understanding.

Maybe I could just write the code and you decide after that if you want to keep it or not. Now that I have checked everything 5 times, it will be really easy to implement.

[gr5]

Okay so there we go. It must at least copy the things in my comment (wavelength, diameter). I assume I put in a breakpoint and saw it do the copy. It's not clear to me if it uses the operator= versus the copy contstructor or... what it does there. Without putting in a breakpoint on the next line and in the copy constructor and checking what gets copied I wouldn't know.

operator= is not defined explicitly. Does c++ default to the copy constructor (which exists)? I don't know. It's not clear to me. I could go back and reread c++ or ask an ai but I will probably quickly forget. To me it would be better if operator= was explicitly defined. Or if we just didn't use operator = for wavefronts.

[githubdoe]

Sigh. Understanding what operator = does is fundamental to C++ . Go learn it.

but to begin with
When the = sign is used, it can trigger either the copy constructor or the copy assignment operator depending entirely on whether a new object is being created.

A copy constructor in C++ is called whenever a new object is initialized from an existing object of the same class.

Key Scenarios

Initialization: When a new object is created and explicitly initialized with an existing object (MyClass obj2 = obj1; or MyClass obj2(obj1);).

Pass-by-value: When an argument is passed to a function by value.

Return-by-value: When a function returns an object by value (though compiler optimizations like Return Value Optimization (RVO) may sometimes omit this).

Exception handling: When an exception object is thrown or caught by value.

[gr5]

I actually looked it up right after I posted.

[githubdoe]

Yes it is complex but important to remember when writing complex classes. Too long ago for me to remember all the ins and outs of why I did what I did on the wave front. It works and I hate to change any of it's logic. If it is only comments then ok. If it is code then I don't have time to test it and can not be relied on to approve if it is not addressing any known bug.

[atsju]

OK I made the commit. I still need to merge latest changes from master.

To clarify again, previously the copy operator and the copy constructor did not do the same job. I'm not aguing about the necessity of shallow or deep copy. Current code works fine and I did not find any flaw (I spent quite some time to find one). Deep and shallow copy are correctly used on purpose.
The thing that I'm proposing to fix is that these 2 operators are supposed to always do the same. Having copy constructor doing a deep copy and the copy operator doing a shallow copy is very border line (to me it sounds like buying a set of scredriver labelled "flat screwdriver" but one of them is actually something slightly different).
Moreover, some fields were not copied. That also very unexpected/wrong from an = operator point of view.

What I did in last commit is just explicit the deep VS shallow copy. I did not change behavior of the code.

I think this helps maintainability and readability but I let you owner of final decision.

PS: When using a shallow copy, one shall not modify any matrix before creating a new one really owned by wavefront or you will modify data from a different wavefront.
It's OK today.

[github-actions]

🚀 New build available for commit 9a8ce62
Download installer here

[atsju]

Ok build fails because of weaker copy elision in C++14. this code would require C++17 to compile.
I will implement option 1 instead. There will be even less code changes

[gr5]

But it sounds like Dale wants option 3.

[gr5]

I don't like option 3. It's confusing that

wavefront a; 
a=b; 

copies some things and

wavefront a(b); 

copies more things.

[githubdoe]

OK, I understand. That is good. Sorry I was too busy to understand exactly. I just looked at wavefront.cpp and realized that it is very sparse. Go ahead with the comment modifications.

[githubdoe]

I'm sorry i was too busy to understand before. yes go ahead with any of the options you guys think is best. I had not understood that atjsu had added copy operations to wavefront years ago. In fact I just needed one in my live video processing. Thanks atjsu. Sorry for my confusion.

[githubdoe]

In addition I get a clang warning about padding being excessive that I did not understand. AI explained it to me. If we reorder the variables in the class so that the all the doubles are after all the larger things like mats and then all the bools are after the doubles then padding is optimized and the class takes up less room. So now I'm recommending that be done as well.

[github-actions]

🚀 New build available for commit d2cd734
Download installer here