dtourolle/DarkRoom
1
0
Fork 0
Code Issues 39 Pull Requests Actions Packages Projects Releases 20 Wiki Activity

Put the grouping dials where the regrouping is

Browse Source 
The merge probability was `dr_face`'s constant and the smallest group was
a bare `< 2` in the clustering pass. Both were tuned on one library —
1,813 faces of one photographer's family — and the quantity they optimise
is a property of the population, not of the model. A household at close
family resemblance and two thousand strangers at a wedding want different
answers, and neither of them is the reference library. The doc comment
already conceded the point and pointed at `face_index --tune`; a
photographer does not have a terminal.

So they are `FaceSettings` now, saved per device beside the cache budgets
and edited from the People screen — beside the Regroup button that
applies them and the rail that shows what they did, because a value
changed three screens away from its effect is one nobody can tune.

Moving them is safe by construction, which is why nothing asks for
confirmation: a regroup writes only the suggested half, and
confirmations, names and ignores enter as anchors and come back
unchanged. The smallest-group rule is applied only to groups the system
invented — a group the user named or set aside survives it whatever its
size, because a display preference does not overrule a judgement.

**Withdrawal, without which the setting does nothing visible.** Raising
the smallest group stops the pass creating small groups; it does not
remove the ones a previous pass made, because those still hold their
suggestions, so they are not empty, so the prune leaves them. The pass
now releases every unanchored face it did not place before pruning.

And a dial you cannot see the effect of is not a dial. "What would this
do?" runs the same population through the clusterer without opening a
transaction and reports groups, faces grouped and largest group — one row
of `--tune`'s table, on the user's own library, on a worker thread. The
line leads with the group count because that is the number that says
which side of the right setting you are on: it climbs as fragments are
gathered into people and falls as separate people start being welded,
while the grouped-face count rises straight through both.

The preview parks its poll timer in a slot of its own. A preview and a
regroup are allowed to be in flight together, and sharing the sweep's
single slot would have the second to start drop the first's timer —
visible as a Regroup that finished on its worker and never said so.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
dtourolle
2026-08-29 22:32:26 +02:00
co-authored by Claude Opus 5
parent a4b9deaf96
commit 23c74d7063
10 changed files with 884 additions and 88 deletions
Download Patch File Download Diff File Expand all files Collapse all files
ui/dr-ui/src/identity_ui.rs
+137 -1
View File
@@ -81,6 +81,12 @@ pub struct IdentityController {
/// the handle, and clustering a real library is not something a Slint
/// callback may do on the UI thread.
regroup: RefCell<Option<Receiver<crate::faces::ReclusterMessage>>>,
/// The running read-only preview of the grouping dials, if any.
///
/// Separate from `regroup` because the two are allowed to be about
/// different things at once and neither blocks the other: a preview writes
/// nothing, so there is nothing for a concurrent pass to corrupt.
preview: RefCell<Option<Receiver<crate::faces::PreviewMessage>>>,
/// Whether the rail is showing the people the user has set aside.
show_ignored: std::cell::Cell<bool>,
/// Rail portraits, kept between refreshes.
@@ -363,12 +369,33 @@ pub type SweepPaths = (dr_sync::Connection, std::path::PathBuf, std::path::PathB
/// The detector and embedder files, when both are present.
pub type ModelPaths = (std::path::PathBuf, std::path::PathBuf);
/// Put the grouping dials on the screen from the settings record.
///
/// Read back out of the controller rather than echoed from the callback's
/// argument, because `Settings::sanitise` may have moved the number: a slider
/// showing 40% while the file held the clamped 50% would be a control that
/// silently disagreed with what the next Regroup was going to do.
fn push_grouping(window: &AppWindow, settings: &crate::settings_ui::SettingsController) {
let s = settings.snapshot();
window.set_identity_merge_probability(s.faces.merge_probability * 100.0);
window.set_identity_min_group_size(s.faces.min_group_size as i32);
}
/// Attach every Identity callback.
///
/// Eight arguments because the screen has eight distinct dependencies and no
/// two of them belong together: three ways of reaching the library, two
/// controllers, the window, the activity log and the settings record. Bundling
/// them into a parameter struct would name a thing that does not exist — the
/// same reason every other `wire` in this file's neighbourhood carries the
/// allow.
#[allow(clippy::too_many_arguments)]
pub fn wire<S, M, P>(
window: &AppWindow,
ctl: Rc<IdentityController>,
catalog: Rc<RefCell<Option<Catalog>>>,
activity: Rc<crate::activity::ActivityLog>,
settings: Rc<crate::settings_ui::SettingsController>,
store: S,
models: M,
paths: P,
@@ -381,6 +408,36 @@ pub fn wire<S, M, P>(
let models: Rc<dyn Fn() -> Option<ModelPaths>> = Rc::new(models);
let paths: Rc<dyn Fn() -> Option<SweepPaths>> = Rc::new(paths);
// The dials start where the settings file left them, once, rather than on
// every open: the screen writes them back through the two callbacks below,
// and re-pushing them mid-drag would fight the slider's own live value.
push_grouping(window, &settings);
{
let weak = window.as_weak();
let settings = settings.clone();
window.on_identity_merge_probability_changed(move |percent| {
let Some(w) = weak.upgrade() else { return };
settings.edit(|s| s.faces.merge_probability = percent / 100.0);
push_grouping(&w, &settings);
// The answer on screen was about the old value. Left there it would
// be read as a description of the new one, which is worse than
// having no preview at all.
w.set_identity_grouping_preview(Default::default());
});
}
{
let weak = window.as_weak();
let settings = settings.clone();
window.on_identity_min_group_size_changed(move |n| {
let Some(w) = weak.upgrade() else { return };
settings.edit(|s| s.faces.min_group_size = n.max(0) as u32);
push_grouping(&w, &settings);
w.set_identity_grouping_preview(Default::default());
});
}
// Re-read everything and redraw. Every mutating callback ends in this
// rather than patching the model in place: the operations here have
// second-order effects — a merge empties a person, a split creates one,
@@ -670,12 +727,76 @@ pub fn wire<S, M, P>(
});
}
{
let weak = window.as_weak();
let ctl = ctl.clone();
let paths = paths.clone();
let settings = settings.clone();
window.on_identity_preview_grouping(move || {
let Some(w) = weak.upgrade() else { return };
// One at a time, like every other pass here. Two previews would
// race to write the same line and the loser's answer would win.
if ctl.preview.borrow().is_some() {
return;
}
let Some((_, catalog_path, _)) = paths() else {
return;
};
w.set_identity_previewing(true);
*ctl.preview.borrow_mut() = Some(crate::faces::spawn_grouping_preview(
catalog_path,
MODEL_ID.to_string(),
settings.snapshot().faces,
));
let timer = slint::Timer::default();
let weak_tick = w.as_weak();
let ctl_tick = ctl.clone();
timer.start(
slint::TimerMode::Repeated,
Duration::from_millis(100),
move || {
let Some(w) = weak_tick.upgrade() else { return };
let mut done = false;
{
let borrow = ctl_tick.preview.borrow();
let Some(rx) = borrow.as_ref() else { return };
while let Ok(msg) = rx.try_recv() {
match msg {
crate::faces::PreviewMessage::Ready(p) => {
w.set_identity_grouping_preview(
identity::preview_label(&p).into(),
);
done = true;
}
crate::faces::PreviewMessage::Failed(e) => {
log::warn!("identity: preview: {e}");
w.set_identity_grouping_preview(
format!("could not work it out: {e}").into(),
);
done = true;
}
}
}
}
if done {
*ctl_tick.preview.borrow_mut() = None;
w.set_identity_previewing(false);
}
},
);
park_preview_timer(timer);
});
}
{
let weak = window.as_weak();
let ctl = ctl.clone();
let catalog = catalog.clone();
let store = store.clone();
let paths = paths.clone();
let settings_for_regroup = settings.clone();
window.on_identity_recluster(move || {
let Some(w) = weak.upgrade() else { return };
// One at a time. Two passes over the same faces would each create
@@ -693,7 +814,7 @@ pub fn wire<S, M, P>(
*ctl.regroup.borrow_mut() = Some(crate::faces::spawn_recluster(
catalog_path,
MODEL_ID.to_string(),
dr_face::DEFAULT_MERGE_PROBABILITY,
settings_for_regroup.snapshot().faces,
));
// Polled from the UI thread, like the indexing sweep: the worker
@@ -969,6 +1090,21 @@ fn park_timer(timer: slint::Timer) {
SWEEP_TIMER.with(|slot| *slot.borrow_mut() = Some(timer));
}
/// Keep the grouping preview's poll timer alive.
///
/// **A slot of its own, and that is the whole point.** A preview and a
/// regrouping pass are allowed to be in flight together — the preview writes
/// nothing — so parking both in [`park_timer`]'s single slot would have the
/// second to start drop the first's timer. The visible symptom would be a
/// Regroup that finished on its worker and never told the screen: the button
/// stuck on "Regrouping…" for the life of the window.
fn park_preview_timer(timer: slint::Timer) {
thread_local! {
static PREVIEW_TIMER: RefCell<Option<slint::Timer>> = const { RefCell::new(None) };
}
PREVIEW_TIMER.with(|slot| *slot.borrow_mut() = Some(timer));
}
#[cfg(test)]
mod tests {
use super::*;