Rawtherapee: Spot removal tool

[Beep6581]

Originally reported on Google Code with ID 2255

I'm starting to code a dust/healing tool in the Pipette branch, but I'd like to know
if there's a difference in processing dust and healing?

I'll try to find the processing algorithm in Gimp's code base, but I'd appreciate if
someone could point me to this code directly, to save time.

Also, we have to find a place for this kind of tool. I'd suggest to create a new tab,
where this tool could be placed, as well as signature, framing. How should we name
this tab, and with which icon? I'd suggest "X-tra" as a name, and with a "pen" icon?

Reported by natureh.510 on 2014-02-19 14:46:57

[Beep6581]

Gimp's Heal Brush uses Poisson editing (Gimp devs told me that more than a year ago,
I assume it's true and hasn't changed). A better tool than the Heal Brush is Heal Selection.
I don't know if it uses the same math as the Heal Brush.

Downside of reconstructing is that if the dust is on some area of detail, that detail
will probably be lost because it will be reconstructed, while using a flat field (if
you have one) will not destroy it.

Maybe check out G'Mic, they've been working a lot on the patch-based Inpaint filter
and it works beautifully and is open source.
See the examples here. I know some are quite old, so I suppose today's Inpainting filter
would do an even better job: http://sourceforge.net/p/gmic/wiki/G%27MIC%20Inpainting%20results/

I don't think dust removal belongs in the same tab as framing, watermarking, and so
on. I'd consider dust removal to be a detail operation, so Detail tab.

Reported by entertheyoni on 2014-02-19 15:25:10

[Beep6581]

I won't be a healing BRUSH, like in Gimp, but a dust/healing tool, like in LR, i.e.
with 2 circles of variable size; source and dest.

A more advanced healing tool with custom shape (no brush, there isn't and will never
be brush in RT: it's 2D painting and RT only support vectors) can be done later, or
if someone.

Maybe I'm confusing what the healing tool is really about, so I'll keep the "dust removal"
words only, to be added in the Detail tab, if you want.

Reported by natureh.510 on 2014-02-19 16:19:35

[Beep6581]

We could call it "Spot Cleanup". 

Some background info on "competition":)

In ACR 8.x the "Spot Removal" tool, when clicked in image area, auto-finds the neighboring
area for the source to paint from.
In 8.x version this is no longer a point cleaning, but an area cleaning tool - one
could use the same circular brush to draw the selection for the destination area of
an irregular shape. Once selection is complete, the source to copy pixels from is auto-found.
My impression that behind the scene this executes a feature-matching algorithm. 

The corrected area can be re-selected to allow 
1. changing feather and opacity and 
2. to re-position either source or the destination.
When selection area is a circle, selection size (radius) can be re-adjusted as well.

The tool has two modes of operation - Heal and Clone. 
Tool is adjustable with following parameters: Radius, Feather, Opacity.

So much for the inspiration from a really very well working tool in ACR:)
It is exceptionally handy for cleaning up scans of film (I just went through 300 of
them).

Reported by michaelezra000 on 2014-02-20 03:15:01

[Beep6581]

It would likely be easier to implement cloning operation at first, feathering the borders.


What Healing does is more complex. The idea, I believe, is to retain the fine texture
in the destination area. I suppose the approach could be to decompose source and destination
in the frequency domain and then reassembly of the greater % low frequency from the
source and greater % of high frequency from the destination, then feathering the border.
Although, may be the mixing logic is more complex.

Reported by michaelezra000 on 2014-02-20 03:40:04

[Beep6581]

For now, I only have the healing tool code from Gimp under my hand, but I plan to use
it like a spot removal tool, i.e. with a circular shape with a feather parameter. It'll
be fine for a first try. Someone more skilled with this kind of stuff will be able
to make something more complex, but that's not the point of this issue.

Title changed to "Spot removal tool"

Reported by natureh.510 on 2014-02-20 17:12:49

[Beep6581]

Question: what should we display when clicking on the Spot tool?
1. all the source & destination circles, paired by a line
2. only points to see destination location, the source and destination circle appearing
when hovering the point. There would be less object on screen at a time if there are
lots of "dust" to edit.

Reported by natureh.510 on 2014-02-22 13:35:49

[Beep6581]

For the active spot (new or being edited) - show pair of source & destination areas
(circles?)
For the not active - only destination area outline.

Reported by michaelezra000 on 2014-02-22 13:58:14

[Beep6581]

Agreed - show both circles of currently edited spot (or when hovering over a spot),
and just destination circles of the others.

Reported by entertheyoni on 2014-02-22 20:00:28

[Beep6581]

Destination circles for unedited ones? Or just points to locate them?

Reported by natureh.510 on 2014-02-23 02:21:47

[Beep6581]

Here is the ACR interface:
http://i.imgur.com/ywMiUXI.jpg

The middle image illustrates active and not active areas. This is actually pretty handy.
However, one the tool is exited, all selections should disappear (just as it works
with the new gradient UI).

Reported by michaelezra000 on 2014-02-23 02:34:44

[Beep6581]

Well, the selection looks like a free hand selection, not a mathematically circular
one, like in LR. Which is surprising since there's a circular object above! What should
we do? Of course, the circular one is simpler to do.

The local editing philosophy will be a bite different: in ACR, you have an adjustment
brush or gradient tool with many sliders for the adjustment, but it is quite rare to
apply several parameter to the same zone. That's why in RT, the zone is defined inside
a tool and used in this tool only. Each tool will have its own zone (or list of zone
to be correct). But we're anticipating quite a lot here ;)

Reported by natureh.510 on 2014-02-23 10:19:43

[Beep6581]

The ACR approach of many tools per zone leads to very productive editing as zone neexs
to be selected once and then a variety of tools can be used there. This is feasible
in ACR since each tool is represented by a single slider only. In case or RT we have
many sliders per tool... so it would be too many sliders per zone.

if we go with the approach of multiple zones per tool, it would be helpful to copy&paste
masks from one tool to another to allow multi tool editing per zone (I find id very
useful actually).

Actually... may be there is a way to implement gui like in ACR (multiple tools per
mask) on a separate tab, but behind the scenes use multiple masks per tool?

Reported by michaelezra000 on 2014-02-23 13:03:49

[Beep6581]

About the free hand brush I ACR, it used to be circle only and became free hand inversion
8.x. if clicked once it still can draw a circle. On a subsequent click new circle is
added to the selection. To start a new zone one has to click New.

Reported by michaelezra000 on 2014-02-23 13:11:16

[Beep6581]

Re #12: Yes, I thought about a mechanism to reuse selections, and possibly associate
them to different tool by setting a name or an identifier. Drag'n'drop may also help,
I'll see.

Re #13: Okay. I'll restrict to a circular shape for the moment. I'm still trying to
figure out how the Gimp's code work :-/

Reported by natureh.510 on 2014-02-23 14:02:23

[Beep6581]

I was thinking about the pp3 representation of the healing tool.
May be a delimited list?
SourceX,SourceY,DestX,DestY,Radius,Feather,Opacity;....;SourceX-N,SourceY-N,DestX-N,DestY-N,Radius-N,Feather-N,Opacity-N

Reported by michaelezra000 on 2014-02-23 15:13:58

[Beep6581]

Re #15: Yes, that's what I planned to do.

Reported by natureh.510 on 2014-02-23 15:25:12

[Beep6581]

Here is a Work In Progress patch of the Spot tool. It's very unlikely that I'll finish
it, and I hope that someone will be able to take it over. The algorithm does not work,
and even filling the bitmap with yellow color doesn't seem to work. I guess that I'm
not poking the right portion of the image. In fact, there's a major problem: when previewing
this tool, if the source location is outside the image, there will be no possible preview
of the target point, since RT only compute the displayed part of the image! There is
also no real image coordinate system throughout the engine (well, I haven't seen anything
simple to use...).

The GUI is not complete either, but usable for testing purpose (e.g. you can't delete
points, you have to delete everything through the reset button).

I guess it'll be another 'XMP like' abandoned patch, but I felt that it would be better
to publish the current state of the work than nothing at all.

If you still want to test it: use the CTRL point with the left mouse button to add
a point, and drag it away to position the source location. The inner circle is the
full effect area, the outer circle represent the feather area. You can use the left
mouse button over each item to move or resize them. Flying over another point will
display the editing widget to that other point (only one can be edited at the same
time).

I've also added more comments for the Doxygen doc of the Edit class.

Reported by natureh.510 on 2014-08-01 20:41:08

---

• _Attachment: 2255-Spot - WIP.patch_

[Beep6581]

The patch compile fine.

But, I have a runtime error '"spot-normal.png" not found'==> std::ios_base::failure"

There are files spot-xx.svg in tools/source_icons, but nothing in data...

Can be did I miss something

:) 

Reported by jdesmis on 2014-08-02 05:55:11

[Beep6581]

It seems that I've forgotten few files, right. Add this patch over the previous one.

Reported by natureh.510 on 2014-08-02 16:15:51

---

• _Attachment: 2255-Spot - WIP-addentum.patch_

[Beep6581]

Hi Hombre, thanks for working on this feature:)!
When applying "2255-Spot - WIP-addentum.patch " I get an error "Patch file does not
contain a valid patch"

Reported by michaelezra000 on 2014-08-02 17:25:04

[Beep6581]

I don't know what happened. Here is the 3 png files to copy to rtdata/images/Dark[&
Light]/actions

Reported by natureh.510 on 2014-08-02 19:08:55

---

• _Attachment: spot-icons.zip_

[Beep6581]

Hombre this is a great progress and lots and lots of work, very educational also, thanks
again:)

Reported by michaelezra000 on 2014-08-04 02:06:27

[Beep6581]

Hombre, regardless whether the features makes it or not, I want you to know how much
your efforts are appreciated :)

Reported by entertheyoni on 2014-08-05 08:46:45

[Beep6581]

Je préfère m'exprimer en français...

D'abord bravo pour cette approche, qui permet l'ouverture vers le contrôle local. Comme
le dit Michael, c'est un sacré boulot !

j'ai essayé de bricoler le code pour voir.

De mon point de vue, ce n'est pas l'algo (ou je ne pense pas, même si je crois il y
a quelques changements à faire...à vérifier...mais actuellement cela ne fonctionne
pas) qui est en cause, mais la gestion mémoire. Il y a des plantages à répétition.

J'ai essayé de changer dans "improccordinator" et "dcrop" la façon d'appeler "spot"
notamment en passant 2 variables Labimage, une pour "aller" et l'autre "retour"...mais
rien de convaincant.

Rien n'y fait et annuler ou non #if 0 ne change rien.

Je pense que l'erreur est cachée, peut être (???) dans les fonctions "copydata" que
tu as introduites (ou ailleurs...quand je pense au temps passé dans "denoise" pour
une lettre !!!)

Je continue à prospecter, et à partir de jeudi j'aurais un peu de temps disponible,
les enfants quittent Fréjus... 

Amicalement

Jacques

Reported by jdesmis on 2014-08-05 12:10:14

[Beep6581]

Merci pour cette revue de code. En effet, j'ai observé des plantages mémoires à répétition,
donc je vais quand même essayer de produire un truc stable. Mais le problème majeur
demeure: il faut que le cercle source ET destination soient dans la zone visualisé
pour que le cercle en quastion fasse sa correction, ce qui n'est pas pratique du tout!

Pour les bases de l'édition local, peut-être pour la GUI (un petit peu), mais le pipeline
est à revoir complètement.

To everyone:

Maybe we should do this correction in the early stage of the pipeline, where we can
use getImage as much as the number of circle, i.e. right after denoising maybe. I 
think it would be possible to display the current state of the image during the editing
session. In that case, doing this feature may be doable without too much effort, provided
that the Gimp's algo is working fine...

Reported by natureh.510 on 2014-08-06 18:07:19

[Beep6581]

I remember the heal brush in Gimp ?<2.8 was not working correctly. Maybe it works fine
in 2.8 but it was not entirely fine in 2.6 and maybe 2.7 (2.7 was the dev branch for
2.8). Either way if works fine now. I've been using 2.9 for months, and 2.9 is the
dev branch for 2.10. If you use current code it should be fine.

Reported by entertheyoni on 2014-08-06 20:04:01

[Hombre57]

I've restarted the development of the Spot Removal tool. The GUI is functional, but I have some interrogations about how to deal with the pipeline.

In my current implementation, the processing is done on the image displayed in the Preview window after the transform processing. This means that each spot entry must have its source area completely visible and its destination area at least partly visible to process that entry. Also, if the zoom rate is too low (small image) and the final circle size is 4 pixels of diameter (IIRC), then that entry is not displayed either. So depending on the zoom rate and the position of the crop window, some Spot entry will be processed, and other won't.

You can also imagine other problem if the user change the transform parameters and the coarse transform ones.

The other way round would be to put this new tool before transformation (even coarse transform ?). In this case, when pressing on the Edit button, the initial image (corresponding to this step of the pipeline) would be displayed, and then the user will be able to put entries here and there in an absolute image space, whatever the transform will be right after. The downside of this is that I have to lock all other tools as long as we'll be in this Edit mode, otherwise the WB pipette or the rotate tool (e.g.) will not make much sens with this displayed image.

I can open a new branch if you need to dig in the code before answering, but I'd prefer to commit the enhanced-edit-mecanism first and rebase the Spot tool on the new master.

[Hombre57]

Forgot to mention: if a source area is partly crossing the image border, the processing of that entry is skipped (but still exist as an entry).

[sguyader]

@Hombre57, thanks for trying to develop this tool. It is one of the most wanted tools left, and this will make RT the very best and complete raw development software.

[Beep6581]

each spot entry must have its source area completely visible and its destination area at least partly visible to process that entry

It would be nice if the user was notified somehow when healing won't work, e.g. cursor changes, or the spot circle on the canvas gets a line through it or some kind of indication.

The LCP tool can lead to serious preview issues at certain zoom levels. This could be an argument for having spot healing before transform. Also remember that Ingo is working on lensfun support which could influence this tool.

Both solutions seem simple and intuitive, as long as the user gets feedback about what's happening.

[Hombre57]

In fact, it is even worse than that : let's say that entry 1 is not computed because out of scope, but entry 2 IS computed, but wrong because it's source area is the destination area of entry 1 (which should have been altered).

The modifications are re-entrant (as it should be), so ignoring some entry will make this tool quite tricky to use.

I think the best I should do is moving this tool before the Transform tool, where I'll be able to copy all required area from the InitialImage, even if out of scope. So, still some work to do :unamused:

[Hombre57]

I've looked at the transform function. It covers CA, Distortion, Rotation, Perspective, Gradient, PCVignetting, Vignetting, LCP.

Almost everything could be done after the Spot tool, but what about the CA feature (chromatic aberration) ? Wouldn't it produce better result for both the Spot tool and the CA tool to adjust CA before the Spot removal ? And is it okay to isolate CA from the lens distortion corrections ? @heckflosse @Beep6581 @michaelezra @adamreichold

[heckflosse]

@Hombre57 Which ca removal do you mean? We've three of them currently.

[heckflosse]

@Hombre57 Ca removal via LCP and/or via future LensFun support should not have a serious influence to Spot tool (if that's what you question)!

[Beep6581]

Wouldn't it produce better result for both the Spot tool and the CA tool to adjust CA before the Spot removal

Yes, but.

1. Is it much more work for you to do that?
2. Will the spot tool be used on parts of the photo which are affected by CA? In other words, if the spot tool is used for dust removal from the sky, CA is not an issue there, it would only be an issue on non-plain areas. What can the spot tool do?
3. Is the course of action you take now limited in scope to this spot dust removal tool only, or will it influence any future local editing tools?

is it okay to isolate CA from the lens distortion corrections ?

There are currently three, and in the future four, places in the UI where we handle CA:

1. Raw CA auto-correction,
2. RGB CA manual correction,
3. LCP CA correction,
4. and in the future lensfun CA correction.
   Is that doable?
   Would it make sense to the user?
   And what does isolating them in the UI achieve?

[Hombre57]

@heckflosse
I'm referring to the ImProcFunctions::needsTransform () function, which include the CA correction from LensProfParams and from CACorrParams (from the Raw tab).

@Beep6581

1. it seems to be complex, at least for me, but I had to ask.
2. the raw tab's CA correction does shrink each r/g/b color channel differently, and thus change even plain area that generally has some grain
3. I don't really understand what you're saying here, but the I just want to insert the Spot removal tool to the right place in the pipeline. Actually, the pipeline groups some tools or filters into a single processing loop (e.g. with ImProcFunctions::transformHighQuality and ImProcFunctions::rgbproc), but if you want local editing, you'll have to split these group so you can apply multiple instance of their subpart (think about local editing of Exposure, Contrast, Saturation separatly). There's clearly an issue in the pipeline along the GUI problem.

Would it make sense to the user?

Not really for the user, but for Image Quality.

And what does isolating them in the UI achieve?

The problem is not in the UI, nothing has to be touched here, but in the pipeline.

[heckflosse]

@Hombre57 needsTransform does not include the ca correction params from Raw tab. And that's perfectly fine. Or do I miss anything?
Edit: If Spot removal depends on raw ca correction, it also depends on demosaic method because demosaic is after raw ca correction. That would be clearly wrong...

[Hombre57]

@heckflosse You're right, it's refers to the Lens/Geometry CA corr. Sorry for the confusion.

[heckflosse]

@Hombre57 :)

[Hombre57]

Just for your information, I've uploaded my WIP patch of the Spot Removal tool. I'm afraid that I'll not be the one that will finish it, because of lack of time.

The GUI should be working, but the engine part is still to be done with the changes explained above.

If nobody take it over, it'll be a dead branch that you can delete if you want.

[Hombre57]

Sooooooooo... I had the time and will of merging dev into the spot-removal branch. If I find time to work on it again, I'd like to have answer to those questions first :

1. at which step should spot-removal be applied ?
2. what if the user transform the image (coarse transform, lens transform, rotate tool, ...) ? Do we want to update the point location ?

My thoughts on this is that it should be placed just after demozaicing and color-space conversion. The benefits are :

• you don't care of the transformations, the spots will always be removed because they are at fixed place. The image may be took in portrait mode, which mean that if you want to fix some dust on the sensor, we need to apply the same "initial" transform to the spot locations, i.e. the spot locations will have to be expressed in non-rotated and absolute coordinate (i.e. sensor coordinate)
• you don't need to process the source area, getting small parts of the buffered demozaiced image will be sufficient

The downside is that RT need to display that dull and untransformed image while editing the spots. If I manage to enter a special display for spot image, then it should be doable for the crop tool too. But that's a different story.

@agriggio @Beep6581 @Desmis @Floessie @heckflosse Do you agree with this behavior ? Any comment ?

[heckflosse]

@Hombre57 Maybe even before demosaic? But that's really just a thought...

[Desmis]

@Hombre57
I think that "the sooner in the process" would be welcome, but it does not have to complicate the algorithm. Perhaps just after demoisaicing
jacques

[Beep6581]

It's a difficult question to answer not knowing the order of the pipeline. I would like to say that "just after demosaicing" is fine, but what about exposure adjustment or DCP tone curve? i.e. if I'm working on a HDR DNG which opens up as black and needs +6EV to see anything, will the spot preview be 0EV or +6EV? If 0EV then the user is "flying blind". Maybe that's ok in practice... I agree to go with what you suggested (just after demozaicing) and see how it works out.

[metal450]

I was exploring options to move away from LR, and between DarkTable & RawTherapee, I do prefer RawTherapee. However, spot removal is an essential tool that I use constantly. Has this feature (no activity in over a year) been abandoned, or is it still somewhere on the horizon?

[Hombre57]

@metal450 It's not abandoned, I merged dev into spot-removal branch just yesterday, and am working on it actually. However RT's engine has not been designed for this kind of feature (especially the preview window), so it'll still require some time before it's complete. So yes it's on the horizon, but you know, the horizon is always at a constant distance from you :confused: , so I'll say it's on top of my TODO list.

[metal450]

Ah - well, awesome to know it's at least still being worked on! Thanks for your super quick reply :)

[Hombre57]

@metal450 It's here, at last ! Not merged yet, but very close to.

Please test the spot-removal branch. Shouldn't that tool be better named Spot Healing ?

How to use the tool (i.e. documentation so far)

In the Detail tab, enable the Spot Removal tool and click on the _On preview editing_ button. The preview now display the image without crop and without processor intensive tools (e.g. Denoising, Wavelets, CIECam, Black & White, etc...), and most importantly, without transformation (Rotate, Perspective, Lens Correction [To be confirmed, I don't use it], etc).

To add a spot, press the Ctrl key while clicking over the location you want to edit, and drag the secondary circle to the source location. During the drag, you can release the Ctrl key.

By hovering the centers or the circles, you can move the source location, the destination location, the inner radius (where the effect is maximal) and the feather radius (where the effect progressively tends to zero). Those radius are paired, i.e. identical for the source and destination location.

To remove a spot, right click over it (this should probably change to Ctrl-Right click, for security reason ?). To remove all spots at once, click the Reset button in the tool's pane.

The tool's preview is visible at all zoom rates, as well as in detail windows, where the on preview widgets can be operated too. The preview is actually not shown in the thumbnails. When panning, the coarse image might not show the effect, or show it without color management. That's the last known caveat.

Some screenshots, for private use only (I don't have the permission from the model, though I cropped the image to make it anonymous).

On preview editing
Before/After view
Output with tool disabled
Output with tool enabled

[metal450]

Very cool!! Thanks for following up, & for the details :D

[Hombre57]

Note :To see the new icon from commit b4d7798, and if you've already built this branch, you'll have to cleanup the cache/svg2png folder, or at least delete the spot-normal.png file.

[Beep6581]

@Hombre57 shall we put this on the 5.7 milestone (out 4 days ago, woo!), or do you need a few months for shine and polish?

[heckflosse]

I played with the spot removal and healed some dust spots, which worked fine :+1:
Then I selected neutral profile from profile combobox and got this crash:

(rawtherapee.exe:11060): glibmm-ERROR **: 11:42:45.659:
unhandled exception (type std::exception) in signal handler:
what: vector::_M_range_check: __n (which is 0) >= this->size() (which is 0)

I will try to reproduce...

[Hombre57]

@Beep6581 It'll require some polishing, I have another algorithm that I'd like to test first, instead of a simple cloning tool. If both are working, should we keep both and let the user select on a per Spot basis ? So yes, it'll be for 5.8.

[heckflosse]

Easy to reproduce:
1) create one healing spot
2) select neutral profile => crash

[heckflosse]

@Hombre57 I built using clang and got a lot of new warnings. See the attached log file
clbuild.log

[Hombre57]

I'm looking into the clang warning, @Floessie converted all class to struct in procparams.h in commit 02d6187cd880cd3b1a3804610e0e33ec181e9db0, and I'd love to know why, just out of curiosity. I hope to have solved everything, but I might need other cbuild.log. Could you make another try after pulling please ?

[heckflosse]

@Hombre57 Sure

[Hombre57]

It doesn't crash, but there's a problem with portrait shots, the spots are not always rendered in the preview (no problem for the output image I guess).

[heckflosse]

@Hombre57

[ 44%] Building CXX object rtgui/CMakeFiles/rth.dir/batchqueuepanel.cc.obj
In file included from Z:/H2/rt56/rtgui/batchqueuepanel.cc:21:
In file included from Z:/H2/rt56/rtgui/preferences.h:26:
In file included from Z:/H2/rt56/rtgui/rtwindow.h:22:
In file included from Z:/H2/rt56/rtgui/filepanel.h:23:
In file included from Z:/H2/rt56/rtgui/batchtoolpanelcoord.h:23:
In file included from Z:/H2/rt56/rtgui/toolpanelcoord.h:55:
Z:/H2/rt56/rtgui/spot.h:106:17: warning: 'Spot::getCursor' hides overloaded virtual function [-Woverloaded-virtual]
    CursorShape getCursor (int objectID);
                ^
Z:/H2/rt56/rtgui/editcallbacks.h:74:25: note: hidden overloaded virtual function 'EditSubscriber::getCursor' declared here: different qualifiers ('const' vs unqualified)
    virtual CursorShape getCursor (int objectID) const;
                        ^
In file included from Z:/H2/rt56/rtgui/batchqueuepanel.cc:21:
In file included from Z:/H2/rt56/rtgui/preferences.h:26:
In file included from Z:/H2/rt56/rtgui/rtwindow.h:22:
In file included from Z:/H2/rt56/rtgui/filepanel.h:23:
In file included from Z:/H2/rt56/rtgui/batchtoolpanelcoord.h:23:
In file included from Z:/H2/rt56/rtgui/toolpanelcoord.h:55:
Z:/H2/rt56/rtgui/spot.h:120:10: warning: 'tweakParams' overrides a member function but is not marked 'override' [-Winconsistent-missing-override]
    void tweakParams(rtengine::procparams::ProcParams& pparams);
         ^
Z:/H2/rt56/rtgui/../rtengine/tweakoperator.h:42:18: note: overridden virtual function is here
    virtual void tweakParams(procparams::ProcParams& pparams) = 0;
                 ^
2 warnings generated.
[ 45%] Building CXX object rtgui/CMakeFiles/rth.dir/batchtoolpanelcoord.cc.obj
In file included from Z:/H2/rt56/rtgui/batchtoolpanelcoord.cc:20:
In file included from Z:/H2/rt56/rtgui/batchtoolpanelcoord.h:23:
In file included from Z:/H2/rt56/rtgui/toolpanelcoord.h:55:
Z:/H2/rt56/rtgui/spot.h:106:17: warning: 'Spot::getCursor' hides overloaded virtual function [-Woverloaded-virtual]
    CursorShape getCursor (int objectID);
                ^
Z:/H2/rt56/rtgui/editcallbacks.h:74:25: note: hidden overloaded virtual function 'EditSubscriber::getCursor' declared here: different qualifiers ('const' vs unqualified)
    virtual CursorShape getCursor (int objectID) const;
                        ^
In file included from Z:/H2/rt56/rtgui/batchtoolpanelcoord.cc:20:
In file included from Z:/H2/rt56/rtgui/batchtoolpanelcoord.h:23:
In file included from Z:/H2/rt56/rtgui/toolpanelcoord.h:55:
Z:/H2/rt56/rtgui/spot.h:120:10: warning: 'tweakParams' overrides a member function but is not marked 'override' [-Winconsistent-missing-override]
    void tweakParams(rtengine::procparams::ProcParams& pparams);
         ^
Z:/H2/rt56/rtgui/../rtengine/tweakoperator.h:42:18: note: overridden virtual function is here
    virtual void tweakParams(procparams::ProcParams& pparams) = 0;
                 ^
2 warnings generated.

[Hombre57]

@heckflosse Could you give a last try ? I should have removed all the warnings now. The bug I reported in my last comment is solved too.

[heckflosse]

@Hombre57 Sure, but will take a bit longer this time as I'm just compiling capture_sharpening branch...

[heckflosse]

@Hombre57 Compiling latest spot_removal now...

[heckflosse]

@Hombre57

Z:/H2/rt56/rtgui/spot.h:106:17: warning: 'getCursor' overrides a member function but is not marked 'override' [-Winconsistent-missing-override]
    CursorShape getCursor (int objectID) const;
                ^
Z:/H2/rt56/rtgui/editcallbacks.h:74:25: note: overridden virtual function is here
    virtual CursorShape getCursor (int objectID) const;
                        ^
1 warning generated.

[Hombre57]

@heckflosse It will take several centuries since I'll get used to this override thing...

[Hombre57]

@heckflosse Done in latest commit, hopefully.

[heckflosse]

@Hombre57 Fixes confirmed

[Floessie]

Builds and runs fine with GCC 8.3 on Debian 10 amd64/i386, Clang 7 on Debian 10 amd64, and GCC 9.2 as well as Clang 8 on Debian unstable ppc.

The UI is super intuitive, slim and easy to use. :+100:

I'm looking into the clang warning, @Floessie converted all class to struct in procparams.h in commit 02d6187, and I'd love to know why, just out of curiosity.

A struct usually indicates the use as a data-only, freely accessable, OO-agnostic collection of elements, whereas class is used for more sophisticated things (e.g. a class has getters and setters to access members). I classified the ProcParams data collections as mainly data-only, thus I chose struct for them. Sorry, if that caused you trouble.

Best,
Flössie

@heckflosse Here's a patch to fix some Clang 8 warnings on non-SSE2 builds:

diff --git a/rtengine/amaze_demosaic_RT.cc b/rtengine/amaze_demosaic_RT.cc
index ffb68dbdd..55e6f7281 100644
--- a/rtengine/amaze_demosaic_RT.cc
+++ b/rtengine/amaze_demosaic_RT.cc
@@ -768,7 +768,7 @@ void RawImageSource::amaze_demosaic_RT(int winx, int winy, int winw, int winh, c

                         //if both agree on interpolation direction, choose the one with strongest directional discrimination;
                         //otherwise, choose the u/d and l/r difference fluctuation weights
-                        if ((0.5 - varwt) * (0.5 - diffwt) > 0 && fabsf(0.5 - diffwt) < fabsf(0.5 - varwt)) {
+                        if ((0.5f - varwt) * (0.5f - diffwt) > 0.f && fabsf(0.5f - diffwt) < fabsf(0.5f - varwt)) {
                             hvwt[indx >> 1] = varwt;
                         } else {
                             hvwt[indx >> 1] = diffwt;
@@ -1236,7 +1236,7 @@ void RawImageSource::amaze_demosaic_RT(int winx, int winy, int winw, int winh, c
                         //first ask if one gets more directional discrimination from nearby B/R sites
                         float pmwtalt = xdivf(pmwt[(indx - m1) >> 1] + pmwt[(indx + p1) >> 1] + pmwt[(indx - p1) >> 1] + pmwt[(indx + m1) >> 1], 2);

-                        if (fabsf(0.5 - pmwt[indx1]) < fabsf(0.5 - pmwtalt)) {
+                        if (fabsf(0.5f - pmwt[indx1]) < fabsf(0.5f - pmwtalt)) {
                             pmwt[indx1] = pmwtalt;   //a better result was obtained from the neighbours
                         }

@@ -1304,7 +1304,7 @@ void RawImageSource::amaze_demosaic_RT(int winx, int winy, int winw, int winh, c

                     for (int cc = 12 + (FC(rr, 2) & 1), indx = rr * ts + cc, indx1 = indx >> 1; cc < cc1 - 12; cc += 2, indx += 2, indx1++) {

-                        if (fabsf(0.5 - pmwt[indx >> 1]) < fabsf(0.5 - hvwt[indx >> 1]) ) {
+                        if (fabsf(0.5f - pmwt[indx >> 1]) < fabsf(0.5f - hvwt[indx >> 1]) ) {
                             continue;
                         }

[Hombre57]

Thanks for the explanation @Floessie, it didn't caused trouble, but I didn't knew the difference and use case. I'll convert the Rectangle class to a struct on next commit then.

[Floessie]

@Hombre57 You mean the one in editwidgets.h?

[Floessie]

When editing the spots in similar regions, it's not immediately clear, which of the linked spots is the source and which one is the destination. Would it be possible to have a little indicator like an arrow head towards the destination?

[Thanatomanic]

@Hombre57 I just tested your tool and it works really well! Well done.
A few things:

• Please indicate which spot is the source and which is the destination @Floessie was first
• Is there a techical reason why the feather is rather limited? Is it reasonable to make it at least as big as the chosen spot diameter?
• Working on spots in 1:1 zoom is relatively slow. Not so surprising of course, when you consider the image needs full processing with every change. Still, not ideal.
• Would it be possible to make the feather line thinner or dotted/dashed? This gives a better visual cue to its purpose imo, and is seen in other software too.
• When you enable the tool, this icon is different from the tool's icon. Not sure if that's intended, or desirable.
  
• A little bug: enable the tool, then F3/F4 to another image. The icon still shows the tool as active, but it isn't.

[Hombre57]

@Floessie

You mean the one in editwidgets.h?

No I meant from rtengine/spot.h

When editing the spots in similar regions, it's not immediately clear, which of the linked spots is the source and which one is the destination. Would it be possible to have a little indicator like an arrow head towards the destination?

Yeah, I'm also annoyed by not distinguishing source and dest. I'll think about something, but I don't think it can be on the linking line, as is can be very short or non existent.

@Thanatomanic

Is there a techical reason why the feather is rather limited? Is it reasonable to make it at least as big as the chosen spot diameter?

I don't think it's so limited : if inner spot's radius is R, the feather max radius is 2xR. Isn't this enough ? Would like the opinion from others too.

Working on spots in 1:1 zoom is relatively slow. Not so surprising of course, when you consider the image needs full processing with every change. Still, not ideal.

It's quite fast here, of course for Release builds and with latest commit. When you click on the Edit button, it enter into a mode where some tools are deactivated, because they are too time consuming or change the geometry (so all tools from the Geometry tab are disabled), excepted the Coarse transform (the image is already _coarse transformed_ at the location of the Spot tool in the pipeline). I still kept the Exposure tool active, as well as Tone Compression and @agriggio 's HDR Tone compression, so that you won't have dark HDR images when editing the spots. The full image is not entirely processed for this tool, however it's almost at the beginning of the pipeline.

If I forgot to disable some tools, you can have a look here to tweak the procparams during the spot editing, and tell what is missing.

Would it be possible to make the feather line thinner or dotted/dashed? This gives a better visual cue to its purpose imo, and is seen in other software too.

Ok, I'll do that.

When you enable the tool, this icon is different from the tool's icon. Not sure if that's intended, or desirable.

The icon shown in the toolbar is common to all on preview editing, would it be curve adjustment or operation on widgets. Probably we should find a more general icon, but I'm against having one more icon.

A little bug: enable the tool, then F3/F4 to another image. The icon still shows the tool as active, but it isn't.

I'll have a look.

Thanks for the testings !

[Hombre57]

@Thanatomanic @Beep6581

A little bug: enable the tool, then F3/F4 to another image. The icon still shows the tool as active, but it isn't.

Fixed in branch editbuttonfix because bug in dev too. Could you test ? It's a minor fix and should be part of 5.7 IMHO.

[Beep6581]

I'm also annoyed by not distinguishing source and dest. I'll think about something, but I don't think it can be on the linking line, as is can be very short or non existent.

You could make one of the circle's outlines a different thickness or style to the other, e.g. dotted.

[Desmis]

@Hombre57

I come back at home... with Internet !
I am always in vacation with my family, but I test this branch

Works fine, idea evoqued by @Beep6581 is good :)

jacques

[Hombre57]

Branch editbuttonfix merged to dev (commit 6b12f299e698be8e26efd30f8594afb56bf00d6a ), which has been merged back in spot-removal-tool.

[Floessie]

@Hombre57

No I meant from rtengine/spot.h

Then: yes. :+1:

Here's a little compaction patch (only test-compiled):

diff --git a/rtengine/spot.cc b/rtengine/spot.cc
index 2244d882e..dc8f92c70 100644
--- a/rtengine/spot.cc
+++ b/rtengine/spot.cc
@@ -23,18 +23,6 @@
 #include "imagesource.h"
 #include <iostream>

-namespace
-{
-
-// "ceil" rounding
-template<typename T>
-constexpr T skips(T a, T b)
-{
-    return a / b + static_cast<bool>(a % b);
-}
-
-}
-
 namespace rtengine
 {

@@ -48,14 +36,12 @@ public:
     };

     struct Rectangle {
-    public:
         int x1;
         int y1;
         int x2;
         int y2;

-        Rectangle() : x1(0), y1(0), x2(0), y2(0) {}
-        Rectangle(const Rectangle &other) : x1(other.x1), y1(other.y1), x2(other.x2), y2(other.y2) {}
+        Rectangle() : Rectangle(0, 0, 0, 0) {}
         Rectangle(int X1, int Y1, int X2, int Y2) : x1(X1), y1(Y1), x2(X2), y2(Y2) {}

         bool intersects(const Rectangle &other) const {
@@ -65,23 +51,23 @@ public:

         bool getIntersection(const Rectangle &other, std::unique_ptr<Rectangle> &intersection) const {
             if (intersects(other)) {
-                if (!intersection) {
-                    intersection.reset(new Rectangle());
-                }
-                intersection->x1 = rtengine::max(x1, other.x1);
-                intersection->x2 = rtengine::min(x2, other.x2);
-                intersection->y1 = rtengine::max(y1, other.y1);
-                intersection->y2 = rtengine::min(y2, other.y2);
-
-                if (intersection->x1 > intersection->x2 || intersection->y1 > intersection->y2) {
-                    intersection.release();
+                std::unique_ptr<Rectangle> intsec(
+                    new Rectangle(
+                        rtengine::max(x1, other.x1),
+                        rtengine::max(y1, other.y1),
+                        rtengine::min(x2, other.x2),
+                        rtengine::min(y2, other.y2)
+                    )
+                );
+
+                if (intsec->x1 > intsec->x2 || intsec->y1 > intsec->y2) {
                     return false;
                 }
+
+                intersection = std::move(intsec);
                 return true;
             }
-            if (intersection) {
-                intersection.release();
-            }
+
             return false;
         }

@@ -101,15 +87,15 @@ public:
             return *this;
         }

-        Rectangle& operator/=(const int &v) {
+        Rectangle& operator/=(int v) {
             if (v == 1) {
                 return *this;
             }

             int w = x2 - x1 + 1;
             int h = y2 - y1 + 1;
-            w = w / v + (w % v > 0);
-            h = h / v + (h % v > 0);
+            w = w / v + static_cast<bool>(w % v);
+            h = h / v + static_cast<bool>(h % v);
             x1 /= v;
             y1 /= v;
             x2 = x1 + w - 1;
@@ -117,7 +103,7 @@ public:

             return *this;
         }
-};
+    };

 private:
     Type type;
@@ -173,7 +159,7 @@ public:
         }
     }

-    SpotBox& operator /=(const int& v) {
+    SpotBox& operator /=(int v) {
         if (v == 1) {
             return *this;
         }

Yeah, I'm also annoyed by not distinguishing source and dest. I'll think about something, but I don't think it can be on the linking line, as is can be very short or non existent.

How about a litte "S" and "D" in a corner of the spot's bounding box?

[Beep6581]

S and D are not localizable.

[Floessie]

Do they have to? Any alternative unambiguous symbol? Coloring the circles?

[Hombre57]

@Floessie Thanks for the patch, I'll add it to the next commit.

To differentiate S and D, I'll opt for dashed line for the source inner circle.

I'm actually trying to solve issue #4858 for 5.7, and possibly #5310 which seem connected, so the Spot tool will wait few days.

[Beep6581]

@Floessie well yes, RT should be localizable. I suggested dotted vs solid lines, but other software, such as the one from that Mexican housing company, use yellow vs white circles.

[Hombre57]

@Floessie @Beep6581 I made some try with dashed circles for source location : for small circles it's very ugly.

I think differentiating by the color would be a simpler and more elegant solution. I'd suggest :

1. source : white / dest : green
2. source : white / dest : yellow
3. source : yellow / dest : green
4. ???

Circles are orange when hovered, and red when resized. The link will stay white.

[Beep6581]

Better to avoid strong colors over the preview (same reason RT uses a neutral color scheme by default), so I vote for 2.

[Beep6581]

I was wrong about the Mexican brick manufacturer (the yellow color was actually from the screencasting software), it uses varying line thickness and an arrow - https://youtu.be/vi9JKMIb7Vw?t=106

[Hombre57]

@Beep6581 & all : I've added source/dest differentiation with dashed circles. If you find this ugly, I can switch to plain circles and yellow destination.

[Hombre57]

@Beep6581 This patch won't be ready for 5.8 (unless I can find some free time and you're okay with that), but contrary to my initial plan, I won't be able to add a real spot healing tool. The actual feature is _only_ a clone tool that can serve to suppress unwanted spots.

I don't think it's worth to postpone its merge into dev because of that. It'll still be possible to add a new mode later one, with graphical element to switch between them.

[Beep6581]

@Hombre57 then let's put it on the 5.9 milestone.

[eathtespagheti]

can't wait to see this in 5.9!!

[Hombre57]

@eathtespagheti The Spot tool is somewhat ready, the only big remaining problem is _coarse transformation invariance_. I mean that if you rotate or flip the image after the placement of the spots, they won't follow the transformation.

That's something I wished to avoid, but ImageSource::getImage include the coarse transformation, and that's where I'm actually stuck.

So if you, @Beep6581, @heckflosse and others agree on committing as is, it can be done rapidly.