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

Initial Color Eq docs based on documentation by Boris. #668

Closed
wants to merge 8 commits into from

Conversation

paperdigits
Copy link
Contributor

@paperdigits paperdigits commented Jul 20, 2024

@paperdigits paperdigits marked this pull request as draft July 20, 2024 19:20
@s7habo
Copy link

s7habo commented Jul 20, 2024

Hi Mica, here I am. How do we proceed now? I'm almost finished with Manual as a document. Tomorrow I can offer it.

@paperdigits
Copy link
Contributor Author

@s7habo if you click on the "Files Change" tab on this page, you'll see the page I added. You can either add comments, click and edit it to propose changes, or make comments that I can accept as changes. You can see the flow for that here: https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/reviewing-changes-in-pull-requests/reviewing-proposed-changes-in-a-pull-request

Feel free to add whatever you'd like!

## options

white level
: Set the upper bound for the brightness correction via the slider or picker.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The default is fine for most images.

@elstoc
Copy link
Contributor

elstoc commented Jul 20, 2024

FYI we want to try and avoid too many images if possible, since it's a pain to re-create them every time the UI or functionality changes.

@s7habo
Copy link

s7habo commented Jul 20, 2024

@paperdigits is it ok for you if I comment like this?

I would hate to make sugestion about changes in the code itself because I have no idea how to use this language.

FYI we want to try and avoid too many images if possible, since it's a pain to re-create them every time the UI or functionality changes.

@elstoc I understand that this means more work, but it also means a lot of work to explain things to users afterwards, when there are hardly any visual examples for functions that are sometimes explained in a very technical way.

I will be happy to re-create them afterwards if the changes require it. When the time has come, you are welcome to contact me.

@paperdigits
Copy link
Contributor Author

@s7habo yeah this is fine. If you can "suggest" the changes, then I can just click "accept" instead of having to copy and paste.

Thanks for the work, the range selection stuff is completely unrecoverable from just the UI itself :D

@elstoc
Copy link
Contributor

elstoc commented Jul 20, 2024

@s7habo I don't mean no images but let's try to limit the number if we can and make them simple enough that they won't need too much maintenance later on. If we are using images to illustrate concepts, it's better to include only as much of the UI as you need to explain things. For example if you're trying to illustrate the shape of the curve when a parameter is applied, then if you only show the curve itself and not the rest of the module UI, it will probably never need to be updated again.

@s7habo
Copy link

s7habo commented Jul 20, 2024

.If we are using images to illustrate concepts, it's better to include only as much of the UI as you need to explain things. For example if you're trying to illustrate the shape of the curve when a parameter is applied, then if you only show the curve itself and not the rest of the module UI, it will probably never need to be updated again.

Ok. I will try to pay attention to it. I will continue now and then we can talk about the details.

@s7habo
Copy link

s7habo commented Jul 20, 2024

The point “effect radius” is still open. I'll deal with that tomorrow.

@s7habo
Copy link

s7habo commented Jul 21, 2024

I am now finished. I also have the whole thing as an odt document which I can offer together with screenshots.
I can also provide the raw files that were used for screenshots.


_Visualization indicator_
![grafik](https://github.com/user-attachments/assets/be6b7a37-fa9f-4f93-9af4-01c5c05f71a6)
shows which pixels have been selected. Red color indicates the selection, and pixels in the blue area are not taken into account.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that if a blend mask is active, the visualization can only be seen in areas where the blend mask selects the module's output.

@jenshannoschwalm
Copy link
Contributor

A BIG thank you to all of you.

BTW I will check how/if the mask visualizing restriction mentioned by @ralfbrown can be avoided.

@ralfbrown
Copy link
Contributor

I haven't looked at the code, but it's pretty clear the mask visualization uses the RGB channels rather than the alpha channel. It would take a reworking of the yellow masks generation in gamma.c to generate the red/blue masks with a new mask display mode (or two) and have the equalizer put the "strength" value on the alpha channel.

@jenshannoschwalm
Copy link
Contributor

Yes but we have a way to pass thru the RGB data.

@paperdigits
Copy link
Contributor Author

@elstoc I hope this is good enough for a first draft.

@paperdigits paperdigits marked this pull request as ready for review August 16, 2024 06:40

This module works in darktable's UCS 22 color space.

The entire hue range is represented as a curve with eight overlapping sections divided by eight fixed nodes. Choose whether you wish to adjust (select) pixels based on their hue, saturation, and brightness. You can then use the equalizer interface nodes, on their respective tabs, to adjust the hue, saturation, and brightness of ranges of pixels selected via this method.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can't choose to adjust/select by saturation or brightness. All of the selections are by hue. It should perhaps say something like "Pixels are selected based on their hue, and the module can be used to adjust the hue, saturation, or brightness of those selected pixels"


The entire hue range is represented as a curve with eight overlapping sections divided by eight fixed nodes. Choose whether you wish to adjust (select) pixels based on their hue, saturation, and brightness. You can then use the equalizer interface nodes, on their respective tabs, to adjust the hue, saturation, and brightness of ranges of pixels selected via this method.

The distances between nodes cannot be changed but all nodes together can be moved by +/- 23 degrees along the color wheel using the node placement slider, allowing you to fine-tune the selection of a specific color.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need to specify +/- 23 degrees here


## hue tab

Click+drag the control points on the curve to change the color of all pixels where the mask includes the given color.
Copy link
Contributor

@elstoc elstoc Aug 16, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the first time we've mentioned the word "mask" (without defining what is meant)


Color Equalizer is essentially an implementation of the Color Zones module, but works in the scene referred part of darktable's pixelpipe.

This module works in darktable's UCS 22 color space.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think most users are going to care about this

@elstoc
Copy link
Contributor

elstoc commented Aug 16, 2024

BTW I've created a separate issue around central documentation of color pickers (#673)

@elstoc
Copy link
Contributor

elstoc commented Aug 17, 2024

As the discussion is getting a bit hard to follow, and I wanted to do a bit of reorganisation, I have created another draft in a new PR #674

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants