ASC-CDL color balance filter.
ClosedPublic
Actions

Authored by woltherav on Oct 7 2017, 4:53 PM.

Details

Reviewers

Group Reviewers

Commits

R37:b3f9a490011a: [Feature] ASC-CDL color balance filter(Slope, Offset, Power)
R37:3f81a96dc7d5: [Feature] ASC-CDL color balance filter(Slope, Offset, Power)

Summary

So, I set out to fix this one: https://bugs.kde.org/show_bug.cgi?id=355747

This isn't exactly what they're asking for, but as noted here, this formula is much more in line with the needs for floating point workflows.

The gui isn't quite the same as it is in other applications, but it is a start. In particular I am trying to figure out how to handle power sensibly.

However, considering the filter is so painfully simple, I am wondering whether the added value is the filter itself(which can be reproduced with the multiply, addition and gamma light modes on fully filled layers), or whether there's a bigger pipeline concern that I am not able to discern here.

Diff Detail

Repository

R37 Krita

Lint

Automatic diff as part of commit; lint not applicable.

Unit

Automatic diff as part of commit; unit tests not applicable.

woltherav created this revision.Oct 7 2017, 4:53 PM

Folks that use the CDL may not know or understand implementation details. Also note that there is a clamp in there, that may be worth putting a toggle on. It is the subject of some discussions.

Also consider the UI, which is typically presented as the usual three wheels, one control per channel. A sample would be something such as LiveGrade.

As far as I can see the filter looks fine, but I'm not sure that the name in the filter menu is the clearest (and we should sort filters in the submenus by alphabet, I guess). It might also be good to have some explicit help text in the filter setttings widget to make clear what the filter is doing.

The filter's implementation looks fine, except one problem:

if the filter has a non-zero "offset" value, KisFilterASCCDL::needsTransparentPixels() should return true, otherwise false.

Right now filtering semi-transparent layers creates an incorrect result:

Otherwise is patch looks okay. I also support the idea of @troyjamessobotka about adding three color selection wheels. It would make color selection much easier.

PS:
I have no idea how people use this filter though.

This revision is now accepted and ready to land.Oct 9 2017, 1:36 PM

Added selectors and "needs transparent pixels" boolean.

There's still the following issues:

The selectors cannot be initialised with the image color space, because the color space a filter is initialised with is nonsense.
The selectors are always the shape that is selected in the color selector settings, there's no way to force a selector type.
It is possible to selector a power of higher than 1, but only in a floating point color space, and only when using the color button, and only if you have first picked the color from the floating point space because of the first mentioned point.(Though, our palettes can now handle floating point colors, so I suposse someone could use those as well)
As a collary to 3, I cannot access any kind of information about the current ocio settings from the filter so I cannot use any of that.

I can only fix 2, but that will need changes to that selector, and I will need a week to refigure out how to use that code, if I still want this selector to be usuable in QT designer.

The other two I cannot do anything about, I am not familiar enough with the filter code. I am not sure how people use this filter either, everything regarding color balance filters is hidden inside youtube videos because people who do video editing think they can edit videos.

EDIT: Slope, offset, power is supossed to be the human readable name of this filter, but I agree it is an awkward name that is bad at explaining it is supossed to be a color balance filter.

Closed by commit R37:b3f9a490011a: [Feature] ASC-CDL color balance filter(Slope, Offset, Power) (authored by woltherav). · Explain WhyOct 11 2017, 1:35 PM

This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

		Path
M		plugins/filters/CMakeLists.txt (1 line)
A	M	plugins/filters/asccdl/CMakeLists.txt (11 lines)
A	M	plugins/filters/asccdl/kis_asccdl_filter.h (61 lines)
A	M	plugins/filters/asccdl/kis_asccdl_filter.cpp (127 lines)
A	M	plugins/filters/asccdl/kis_wdg_asccdl.h (62 lines)
A	M	plugins/filters/asccdl/kis_wdg_asccdl.cpp (103 lines)
A	M	plugins/filters/asccdl/kritaasccdl.json (9 lines)
A	M	plugins/filters/asccdl/wdg_asccdl.ui (105 lines)

Diff	ID	Base	Description	Created	Lint	Unit
Base			Base
Diff 1	20452			Oct 7 2017, 4:44 PM	★	★
Diff 2	20574			Oct 10 2017, 4:09 PM	★	★
Diff 3	20589	1e250d6	R37:b3f9a490011abb5c7c717789ba68c55f89897db8	Oct 11 2017, 1:35 PM	★	★