Common base class for CPImageCtrl and MaskImageCtrl

[johnfi...@gmail.com]

Thomas, is there any way that I could code/commit creation of a common base class for CPImageCtrl and MaskImageCtrl that you would consider acceptable to be merged to the default branch?

I think it would improve the maintainability of that code.  I think it would also give me a clearer way to do or redo some other changes that I hope can be merged in.  For me, it would also make it easier to maintain my private enhancements with less effort for me to merge your future changes into my version.

I assume creation of this base class should be started on a new branch (not on the branch with my previous code that you don't like).  I expect you want a first commit that just makes a common base class with no more than the minimum contents (meaning with less contents than provides any justification for having the class).  Then individual commits per function to replace common functionality that exists in the two classes with a single version the new base class.

[T. Modes]

johnfi...@gmail.com schrieb am Dienstag, 1. März 2022 um 18:46:40 UTC+1:

Thomas, is there any way that I could code/commit creation of a common base class for CPImageCtrl and MaskImageCtrl that you would consider acceptable to be merged to the default branch?

From an implementation point both classes are very different. They have only some minor and trivial functions in common - the main part is completely different implemented.

So it brings only little benefit and on the other side a high risk of breaking existing functions.

I assume creation of this base class should be started on a new branch (not on the branch with my previous code that you don't like). 

I don't like your branch because it has performance issues with normal 8 bit images and massive performance issues with float images as your confirmed. Furthermore it breaks existing features.

I did not see that you fixed these issues. Instead you want to start from begin in a new branch...

[John Fine]

On Wed, Mar 2, 2022 at 12:52 PM T. Modes <Thomas...@gmx.de> wrote:

From an implementation point both classes are very different. They have only some minor and trivial functions in common - the main part is completely different implemented.

So it brings only little benefit and on the other side a high risk of breaking existing functions.

I think you are very wrong about that.  But it is your decision.

 

I don't like your branch because it has performance issues with normal 8 bit images and massive performance issues with float images as your confirmed. Furthermore it breaks existing features.

I did not see that you fixed these issues. Instead you want to start from begin in a new branch...

I'm trying to find a way to cooperate with you.  Obviously I haven't figured that out yet.

I can't magically fix a performance problem that I can't duplicate.  I also can't fix a "broken" feature that so far as I can tell never works even without my changes (I tested on two very different Windows systems and three very different Linux systems).

I thought starting from scratch with a different branch is a better way to find a way to make changes you will find acceptable.

I really want to include the 400% and 800% zoom choices as well as increasing, rather than disabling, the magnifier at (or above) 200%.  The memory saving idea was my attempt to make those changes acceptable to you.  But in its current form that isn't going to happen.

I could restructure the memory savings so that it only applies to 400% and 800% zoom and make it faster for those two zoom levels (but I can't make it as fast as you say your version is for you).  I understand the objection to simply allowing 400% and 800%: On a low ram system the sudden large amount of swapping could make the program look broken rather than just slow.  If the memory saving feature automatically switched in for just those zoom levels, then on systems (unlike any I've found) in which the scrolling was originally smooth, the performance flaw of higher zoom would be understandable by almost any user and they could simply decide not to use the higher zoom if the poor scrolling matters that much.

On my systems, the scrolling was never smooth and my memory saving feature makes it smoother.  So if I coded it to switch on just for 400% and 800% by default, I'd like a hidden option to control that switching point.  But the slight improvement in scrolling I get is not that important to me vs having a high zoom with magnifier is necessary for my use of hugin, and I'd like to avoid as much as possible of future merge work taking changes from the official version into my own.

I also use the higher zoom in masks, which is one of the reasons I want a common base class, but far from the only reason.

[Gunter Königsmann]

If two classes have nothing in common from the implementation side deriving them from the same base class still might have a big advantage: You can make a pointer to an object of that base class and access all the common members without casting them and without writing code that is specific for dealing with one of these classes. Sometimes that allows to de-duplicate much of the code that uses the two classes...

[johnfi...@gmail.com]

On Monday, March 7, 2022 at 1:13:45 PM UTC-5 gunter.ko...@gmail.com wrote:

If two classes have nothing in common from the implementation side deriving them from the same base class still might have a big advantage: You can make a pointer to an object of that base class and access all the common members without casting them

In my opinion, that is the opposite of this situation:  I believe the two classes have quite a lot in common functionality, some of which has the same implementation now (with duplicated code) and other parts should have the same implementation.

But (for these two classes) I can't think of any use for a pointer to base class.  The owner/client of either object knows which one it is using and has no interest in the other, nor in any abstraction covering both.

So on the general topic of inserting common base classes, you have a valid point.  But regarding this specific question of adding a common base class, common implementation of matching functionality is the only factor.