Repository navigation
masks: flexi masks, one panel to rule them all - #22553
masterpiga wants to merge 6 commits into
Conversation
Flexi masks store a module's mask as a tree of groups. This adds the storage it needs, without anything that reads it yet: nothing sets DEVELOP_MASK_FLEXI or creates the new form types, and the classic group renderer ignores every new field, so every mask renders as it did with masks v6. - the group point gains a refinement, a name, a group opacity and a preset note, appended to the struct (16 to 240 bytes). The v6 to v7 step sets the group opacity to 1.0; zero is neutral for the rest - new form types DT_MASKS_PARAMETRIC and DT_MASKS_RASTER, with their point structs in blend.h, and DEVELOP_MASK_FLEXI for mask_mode - new state bits for group markers, group operators and modifiers and per-element flags, with static asserts that the roles never overlap A blob stores each point at the size of the masks version that wrote it. The masks history loader and the XMP format 2 importer now step through it with dt_masks_point_stride and zero-fill what an older point lacks; reading a v6 blob at the v7 size would run past its end. Every place that builds a group point now zeroes it, so no indeterminate byte reaches a blob. Older darktable versions cannot read a v7 group blob. The data model is described in dev-doc/masks_data_model.md.
masterpiga
added
priority: low
|
@masterpiga : Just a note that I will be mostly away from my computer for the week-end. |
d571c5f to
98047ea
Compare
A flexi mask is a tree of groups (see dev-doc/masks_data_model.md). This adds everything that renders and converts one, with nothing that creates one yet: DEVELOP_BLEND_VERSION stays 14, nothing sets DEVELOP_MASK_FLEXI, and the migration is compiled but not called, so every mask still renders through the classic fold. - group.c: the flexi fold, _group_get_mask_roi_flexi, selected by DEVELOP_MASK_FLEXI. Each group folds its visible members in list order with its one operator, then applies its refinement, inversion and opacity. The combiners are exported for reuse, and a per-thread depth guard stops a cyclic tree from recursing without end - parametric.c, raster.c: the new form types, a blendif channel and another module's raster mask as group elements - masks.c: group markers and the tree operations on them, including the conversion of a classic list into one-operator groups and the simplification of the result; marker-aware id allocation, cleanup and hashing, with group opacity and refinement in the mask hash - blend.c: per-element and per-group refinement, the drawn-mask cache keyed on the pipe's own forms, and dt_develop_blend_legacy_params_ext carrying the history row a conversion will be written back under - pixelpipe, imageop: the refinement bypass snapshot, raster-mask users inside flexi groups, and pruning of stale raster consumers - migrate_legacy.c: the classic to flexi conversion, and develop.c's plumbing for the forms it synthesizes while history loads Also fixes dt_iop_copy_image_roi, whose per-line fast path read before the start of the input buffer for a negative RoI offset.
The migration has to render every existing mask the same, and no set of invented test cases covers what users actually built. These tools let a user hand over a reproducer, and let anyone re-run the check on it. They do nothing unless their flag is given. - --harvest-masks FILE exports every mask configuration in a library as JSON, opening the library strictly read-only before startup can lock or upgrade it. --harvest-masks-xmp DIR FILE does the same from the XMP sidecars under DIR. The output holds numbers and module names only: no file names, shape or group names, EXIF or image data - --verify-masks FILE replays each harvested mask on a generated probe image, before and after migration, and reports the differences in FILE.report.json. It needs no GUI, so it runs headless and in CI test_probe_image checks that the probe image covers the whole range of every blendif channel, also under windows the size of a drawn mask, and has the edges and texture that feathering and detail masks read: a mask rendering to zero on the probe would pass the check vacuously.
What the masks panel needs from the shared widgets, ahead of the panel: - gradientslider: marker size and bar height come from CSS, so a theme can size the sliders; markers are drawn as one shape set, a caller can keep one marker picked out while it edits that marker elsewhere, and dtgtk_gradient_slider_multivalue_set_value_pushing sets a marker the way a drag moves it, pushing a neighbor it crosses along - bauhaus: a popup can be pinned to a caller-chosen rectangle (dt_bauhaus_widget_set_popup_position), and a static popup can report the value under the pointer for a preview without changing the slider. A slider without a label centers its baseline in the height it has. A slider's popup gets room past both ends of its range, so a click there sets min or max instead of closing the popup - paint: glyphs for import, solo, solo edit, the mask lock, the masks panel, and the screen (smooth union) and product operators; the existing operator glyphs are redrawn to match them, and the union and intersection glyphs are renamed maximum and minimum, after the operations they draw
A module's mask was chosen as one of drawn, parametric, raster or drawn and parametric, each with its own controls in the blending section, and drawn shapes were combined in a flat list edited in the mask manager. Parametric channels could only be ANDed, a raster mask replaced the whole mask, and the three kinds could not be combined with each other. The mask is now a tree of groups (dev-doc/masks_data_model.md), edited in one panel: shapes, parametric channels, raster masks and AI objects are elements of groups, and every group folds its elements with one operator and has its own opacity, inversion and refinement. - the switch: dt_develop_blend_legacy_params_ext runs the migration, and DEVELOP_BLEND_VERSION goes to 15, so every older edit is converted to flexi when it is loaded, and written back as flexi. Conversion is one way: older darktable versions cannot read the result - the panel replaces the mask modes in blend_gui.c, and the mask manager (libs/masks.c) is removed. It lives in the module's blending section, in a utility module (libs/masks_flexi_host.c) or in a panel over the canvas (gui/gtk.c, masks_gui_panel_host.c), and a darkroom toolbar button shows or hides it. Built-in group layout presets come from data/masks_group_presets.json, with notes for novice users - the canvas follows the panel: hover and selection are mirrored both ways, solo and solo edit narrow what is drawn or edited, an AI object moves as one unit unless the user steps into it, and deleting a shape or changing its opacity on the canvas does what the panel does - a mask can be locked, and a locked mask survives reset, presets, styles and paste. The lock takes over a reserved field of the blend params, which every legacy conversion clears - dtgtk/expander: only a module's own expander takes the scroll target, so folding the panel's sections and groups does not scroll the module, and a focused module no longer scrolls back to its header whenever it grows - preferences for the panel under plugins/darkroom/masks/ and plugins/darkroom/blend/, the panel's CSS, and its styling contract for themes in dev-doc/flexi_masks/styling.md - tests: unit suites for the panel's model layer, migration, caching, persistence and styling; a pixel suite of classic edits in src/tests/masking/flexi rendered through the migrated load path; and --check-masks, --roundtrip-masks, --styleapply-masks, --persist-masks, --undo-masks and --lock-masks, which run a harvest through the database trip, style application, panel edits, undo and the lock
|
@masterpiga what gets exposed to shortcuts? My thought is how much control is available from Lua using dt.gui.action() calls. |
|
I posted this on I want to say again that the revamp is great work, and I'd really like to use it. But in its current state the UI still feels too far from the rest of darktable and is hard on my eyes, so I couldn't use it day to day. I sorted the list by priority: I think the critical ones should be fixed before merge, the rest can wait, and the last one is just a suggestion. Critical
Would be nice to fix
Suggestion
PS: Screenshots are from the previous build, but they are still relevant. I acknowledge that rounded corners are now fixed. Thank you for that. |
98047ea to
e1aec1a
Compare
"Carried over" means a shortcut saved under the old path is attached to the new action when shortcuts are loaded, and saved under the new path from then on.
As mentioned in Pixls, since these are all cosmetic fixes I would rather apply them after the initial merge. |
|
Is thee a user guide for this? My first test
Here's the xmp |
This is very frustrating to me. Darktable achieved very fragile but still mature visual consistency. Very few purely open source projects of similar size have it. These and couple other items break it in a core feature. Despite the great idea and huge efforts from your side, new mask panel will not be usable for me in current state. |
That seems a bit strong :) That said, darktable is not mine. I just expressed an opinion, and provided a rationale for it.
I am AFK, I will look as soon as I have chance. EDIT: @wpferguson, at step 15 you have the drawn mask: 
I can only assume that you have deleted it by mistake, because at step 16 it is no longer there. |
|
@masterpiga : Sorry, I didn't want to sound rude. I've shared only personal perspective. You are doing a great job capabilities wise with this revamp. I am sure there will be a plenty of users who will tolerate or even like your UI design approach. |
|
If I have read this carefully then the only real controversial point is 3. I have no strong opinion myselg, but I would tend to agree with @andriiryzhkov about the naming, maybe just "mask • exposure". For the buttons I agree that on the header it is ok, but then the renaming of the panel label will take too much horizontal spaces, think about "mask • color balance rgb" + all icons. So if the we rename we will also need to remove the buttons from the header. I see 2 solutions:
All in all, we need to think more about this as there is no change that will be a win only from my POV. |
4 participants
Context
In #21905, @TurboGit suggested that the set of
commits that upstream the work on Flexi masks is split into two PRs.
#22542 (merged) was the first, and this is the 2nd one.
Summary
Until now a module's mask was one of drawn, parametric, raster, or drawn
and parametric, each with its own controls. Drawn shapes were combined in
a flat list in the mask manager. Parametric channels could only be ANDed,
a raster mask replaced the whole mask, and the kinds could not be mixed.
This PR makes the mask a tree of groups, edited in a single panel. Drawn
shapes, parametric channels, raster masks from other modules and AI
objects are all elements of groups. Each group combines its elements with
one operator and has its own opacity, inversion and refinement, which
each element can also have. The panel replaces the mask modes and the
mask manager. It can sit in the module, in a utility module or docked
beside the image, and it mirrors hover and selection with the canvas.
Existing edits are converted when they are loaded. The data model is
described in
dev-doc/masks_data_model.md.Commits
the parametric and raster form types, and the state bits. Blobs are
read with a per-version stride, so v6 data is never read past its end.
and per-element refinement, the drawn-mask cache and the classic to
flexi migration. It is compiled but not yet used. It also fixes a read
before the start of the buffer in
dt_iop_copy_image_roi.--harvest-masksexports alibrary's mask configurations as anonymous JSON, and
--verify-masksreplays them headless before and after migration. This lets users hand
over reproducers.
sizing from CSS, bauhaus popup placement and preview, and new glyphs.
(
DEVELOP_BLEND_VERSION15), adds the panel, its three homes and thelayout presets, and removes the mask manager. Also adds a mask lock,
preferences, the panel's CSS (themable, see
dev-doc/flexi_masks/styling.md) and the tests.Compatibility
The conversion is one way: older darktable versions cannot read an edit
saved by this one with its masks intact. The release notes say so and
recommend a backup.
Testing
persistence and styling pass
src/tests/masking/flexi) passes 46/46 on amacOS Release build
--check-masks,--roundtrip-masks,--styleapply-masks,--persist-masks,--undo-masks,--lock-masks) were run on harvested user librariesResults of classic masks migrations
0 migration failures in 8203 distinct configuration shapes
→ the failure rate is below 0.037% (1 in 2,738) at 95% confidence.
The confidence interval is one-sided Clopper-Pearson. With zero
observed failures that degenerates to the rule of three, a bound of about 3/n.
Each "shape" is a complete, distinct mask setup:
Classic-GPU outliers are counted separately on purpose: there the CPU
renders classic and migrated identically and only the classic GPU
render disagrees, which is a pre-existing divergences between the
OpenCL/CPU paths that migration exposes rather than causes.
Checklist
src/tests/integration/where the pixelpipe is touched, ordarktable-clias a headless smoke test._(), new preferences are registered indata/darktableconfig.xml.in.RELEASE_NOTES.mdentry was added (only needed if fixing an issue in a release). Do not reference GitHub issues.AI assistance
Co-authored with Claude and Gemini.