Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft: Fix #17178 JzCzhz color picker #17209

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

kofa73
Copy link
Contributor

@kofa73 kofa73 commented Jul 27, 2024

No description provided.

@kofa73 kofa73 mentioned this pull request Jul 27, 2024
@jenshannoschwalm
Copy link
Collaborator

  1. Looking at the right spot :-)
  2. It would be better to have the impossible conversions reported in a readable form
// fallback, but this shouldn't happen
      dt_print(DT_DEBUG_ALWAYS,
               "[colorpicker] unknown colorspace conversion from %s to %s\n",
               dt_iop_colorspace_to_name(image_cst), dt_iop_colorspace_to_name(picker_cst));
      _color_picker_work_4ch(source, roi, box, pick, NULL, _color_picker_rgb_or_lab, 100);

you will notice while playing with the pickers in the blending gui that there are more conversions requiring support.

  1. Your check (effective_cst == IOP_CS_JZCZHZ && picker_cst == IOP_CS_LAB) seems wrong to me. I think we must test for all cases effective_cst == IOP_CS_LAB and convert according to the chosen picker_cst.
    It would be correct and also might be the easiest way to convert IOP_CS_LAB data to IOP_CS_RGB exactly as we do for the pixelpipe (see pixelpipe_hb.c around line 1130) and use those converted data for processing.

@ralfbrown ralfbrown added bugfix pull request fixing a bug scope: UI user interface and interactions labels Jul 27, 2024
@kofa73
Copy link
Contributor Author

kofa73 commented Jul 29, 2024

I can start working on it on 1 August, but just a little bit every day (I'm supposed to be on vacation then). I not done by the 7th, I won't be able to touch it for a week (will be away from my computer).

@jenshannoschwalm
Copy link
Collaborator

This is something new for 5.0 so no reason to make haste

@TurboGit TurboGit added this to the 5.0 milestone Aug 6, 2024
@TurboGit TurboGit added the wip pull request in making, tests and feedback needed label Aug 6, 2024
@TurboGit TurboGit marked this pull request as draft August 6, 2024 20:46
@kofa73 kofa73 force-pushed the 17178-fix-JzCzhz-color-picker branch from 8970637 to abc23e9 Compare August 17, 2024 15:01
@kofa73
Copy link
Contributor Author

kofa73 commented Aug 17, 2024

@jenshannoschwalm I only got some time to sit down and have another look now.

With your suggested formatted debug message, I get:
unknown colorspace conversion from IOP_CS_LAB to IOP_CS_JZCZHZ, and you are right, I mixed them up.

With that fix, I do get a usable mask:
image

I don't yet get how to implement your suggestion regarding RGB, but I'll check more later.

Regarding comments like

// FIXME: the mean calculation of hue isn't always right, use circular mean calc instead?

Wouldn't it be better to convert the whole selected area into linear RGB, calculate the mean there, and convert that to the picker space?

@jenshannoschwalm
Copy link
Collaborator

The problem here is the "range" calculation around hue zero. So if the picker finds data around red the range should be marked correctly.

You will find code in the area picker representation in color equalizer.

@jenshannoschwalm
Copy link
Collaborator

Are you still working on this or would you want me to take over?

@kofa73
Copy link
Contributor Author

kofa73 commented Sep 23, 2024

I have not touched it in a while. I hope to get back to it. Give me two weeks, and if still nothing, please take over.

@kofa73
Copy link
Contributor Author

kofa73 commented Sep 26, 2024

Hi @jenshannoschwalm , I need a bit of help: do you mean I should use dt_ioppr_transform_image_colorspace ? If yes, what should I submit for the module? Maybe I completely misunderstood, sorry. I hope I can spend a bit of time on this during the coming weekend.

@jenshannoschwalm
Copy link
Collaborator

I think i may have put you on a not-so-good track.

For the RGB it's probably fine to convert each pixel (as you did for the Jz stuff) via dt_XYZ_to_Rec709_D50() after dt_Lab_to_XYZ().

@kofa73 kofa73 force-pushed the 17178-fix-JzCzhz-color-picker branch from abc23e9 to 6cbd22d Compare September 29, 2024 11:33
@kofa73
Copy link
Contributor Author

kofa73 commented Sep 29, 2024

Added, but source = Lab conversions seem broken. I have now set up KDevelop, and am happy to work on this.

local_contrast with scene referred blending space:

'g' channel_
This looks reasonable:
image
But this does not:
image

'B' channel (running into negative numbers?)
image

'Jz':
image
image

'Cz' is broken:
image

'hz' seems to be 'lagging'
image
image
image
image

Here is the colour wheel used for testing:
colour_wheel

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugfix pull request fixing a bug scope: UI user interface and interactions wip pull request in making, tests and feedback needed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants