LA - Move colortoning labgrid from rgb to lab - issue #6132 #6173
Conversation
| @@ -346,7 +346,8 @@ ColorToning::ColorToning () : FoldableToolPanel(this, "colortoning", M("TP_COLOR | |||
| //------------------------------------------------------------------------ | |||
| // LAB grid | |||
| auto m = ProcEventMapper::getInstance(); | |||
| EvColorToningLabGridValue = m->newEvent(RGBCURVE, "HISTORY_MSG_COLORTONING_LABGRID_VALUE"); | |||
| // EvColorToningLabGridValue = m->newEvent(RGBCURVE, "HISTORY_MSG_COLORTONING_LABGRID_VALUE"); | |||
| EvColorToningLabGridValue = m->newEvent(LUMINANCECURVE, "HISTORY_MSG_COLORTONING_LABGRID_VALUE"); | |||
There was a problem hiding this comment.
I change the event..otherwise it doesn't work
In Rgbproc ==> event RGBCURVE
in Lab ==> event LUMINANCECURVE
|
I don't quite understand the changes here, and my first impression is that shifting this tool in the pipeline breaks backward compatibility. No? In fact, have we been considering backward compatibility with all the recent pipeline changes? Also, why is this specific to the LabGrid method and not the other methods from the color toning panel? |
|
@Thanatomanic Why this one, because it use Lab..in RGB jacques |
| @@ -3565,9 +3565,9 @@ void ImProcFunctions::rgbProc (Imagefloat* working, LabImage* lab, PipetteBuffer | |||
| for (int i = 0; i < tH; i++) { | |||
| Color::RGB2Lab(tmpImage->r(i), tmpImage->g(i), tmpImage->b(i), lab->L[i], lab->a[i], lab->b[i], toxyz, tW); | |||
There was a problem hiding this comment.
Considering the changes below, do we still need to perform this loop?
There was a problem hiding this comment.
yes because, It create Lab for after :)
| @@ -3184,9 +3184,9 @@ void ImProcFunctions::rgbProc (Imagefloat* working, LabImage* lab, PipetteBuffer | |||
| Color::RGB2Lab(&rtemp[ti * TS], >emp[ti * TS], &btemp[ti * TS], &(lab->L[i][jstart]), &(lab->a[i][jstart]), &(lab->b[i][jstart]), toxyz, tW - jstart); | |||
There was a problem hiding this comment.
Do we still need this loop with the commented out section below?
rtengine/dcrop.cc
Outdated
| @@ -1171,6 +1171,10 @@ void Crop::update(int todo) | |||
| bool cclutili = parent->cclutili; | |||
|
|
|||
| LUTu dummy; | |||
| bool hasColorToningLabGrid = params.colorToning.enabled && params.colorToning.method == "LabGrid"; | |||
There was a problem hiding this comment.
Why create a new variable? This could be directly part of the if () statement below.
rtengine/improccoordinator.cc
Outdated
| @@ -1286,6 +1286,10 @@ void ImProcCoordinator::updatePreviewImage(int todo, bool panningRelatedChange) | |||
| nprevl->CopyFrom(oprevl); | |||
| histCCurve.clear(); | |||
| histLCurve.clear(); | |||
| bool hasColorToningLabGrid = params->colorToning.enabled && params->colorToning.method == "LabGrid"; | |||
There was a problem hiding this comment.
See previous comment. No variable needed imo.
There was a problem hiding this comment.
Yes we can remove, I keep only for visibility..with "old" version.
This change is in - dcrop, improccoordinator, rtthumbnail, simpleprocess
You can modified and push this chnage if you want :)
Jacques
rtengine/rtthumbnail.cc
Outdated
| @@ -1438,6 +1438,12 @@ IImage8* Thumbnail::processImage (const procparams::ProcParams& params, eSensorT | |||
| CurveFactory::complexsgnCurve (autili, butili, ccutili, cclutili, params.labCurve.acurve, params.labCurve.bcurve, params.labCurve.cccurve, | |||
| params.labCurve.lccurve, curve1, curve2, satcurve, lhskcurve, 16); | |||
|
|
|||
|
|
|||
| bool hasColorToningLabGrid = params.colorToning.enabled && params.colorToning.method == "LabGrid"; | |||
There was a problem hiding this comment.
Same here about the variable
rtengine/simpleprocess.cc
Outdated
| @@ -1347,6 +1347,12 @@ class ImageProcessor | |||
| CurveFactory::complexsgnCurve(autili, butili, ccutili, cclutili, params.labCurve.acurve, params.labCurve.bcurve, params.labCurve.cccurve, | |||
| params.labCurve.lccurve, curve1, curve2, satcurve, lhskcurve, 1); | |||
|
|
|||
|
|
|||
| bool hasColorToningLabGrid = params.colorToning.enabled && params.colorToning.method == "LabGrid"; | |||
There was a problem hiding this comment.
Same here about the variable
|
@Thanatomanic You can modified and push this change if you want :) But no problem , I can do it :) jacques |
|
@Desmis For me the original bug is fixed, but when the Color Toning module is off, changing the L*a*b* color correction grid still triggers the progress bar. A non-active tool should not do that. Is there a fix for that? |
|
@Thanatomanic As you said, we really need a GUI specialist Jacques |
|
If no objections I will merge tomorrow :) |
Oh interesting. Could be a faulty listener or something. That's something for another issue. No objection for a merge of this PR 👍 |
|
The fixed the Labgrid error, so it it fine from that perspective. The DCP []Tone curve and []Look table aspect of the bug still remains. And based on the pervasiveness of the bug, I suspect it may show up elsewhere as well. You can either merge now, or keep the branch open until the bug is totally gone. (Sorry, I am not a dev so I don't know what standard practice is in a case like this) |
@Thanatomanic @chaimav
I made the same modification as for shadows-highlight (main) and local contrast (main).
Move the "lab" code from rgbproc to Lab
jacques