focus: judge a filter by how sharply it peaks, not by how big it reads - #32
Conversation
The designer could change a filter but not tell you whether the change was an improvement, so the only feedback was the live number -- and that number is a trap. On the 85H50AI, turning section 3 on took the peak from 25,698 to 55,642 while making the filter worse: that section reads HIGHER as the picture blurs (r = -0.49 against a Laplacian ground truth), so it inflates every reading including the out-of-focus ones. "Make the number go up" is backwards. What actually matters is contrast along a defocus sweep: how far the reading falls off peak as the lens walks away from focus. So "Measure it" walks the lens out eight steps, reads the grid at each, walks it back, and reports the ratio -- "falls to 1/12 of its peak" -- never the peak itself. The lens is walked back in a finally, unconditionally. Stopping, leaving the tab, a camera that stops answering and a thrown error all arrive there, and abandoning someone's focus where an abort happened to leave it is worse than never measuring: the focus is gone and nothing said so. Offered only where focus.move exists; a camera focused by hand cannot be asked to sweep.
PR Summary by QodoMeasure focus filters by defocus peak sharpness
AI Description
Diagram
High-Level Assessment
Files changed (3)
|
Code Review by Qodo
1.
|
![qodo-free-for-open-source-projects[bot]](https://avatars.githubusercontent.com/in/1068000?s=60&v=4){kind=small}
Four findings from review, all real, three of them one root: the sweep drives the lens for several seconds and nothing else in the module knew it. A move the sweep can observe. It called focus.move() raw, so a host that reports failure by REJECTING -- which is what a fetch-backed host does -- left the rejection unhandled and the step counted anyway. A counted step that never happened is a step handed back that was never taken, which walks the lens PAST where the operator left it. moveOnce() reports the outcome; a refused step out ends the sweep, and a refused step back is counted and said out loud rather than papered over with "the lens is back where it started". Sweeps are serialised. Aborting one bumps the generation, which stops it measuring but cannot stop it walking back -- that part must finish. So for a few seconds after leaving Focus there is still a sweep sending `near`, and a new one starting then sends `far` against it. A new sweep now waits for the old one's walk home, and is dropped if it was cancelled while it waited. A destroyed editor does not resume polling. destroy() stops the poll and empties the root without touching `mode`, so a finally asking only "still in Focus?" answered yes for a panel that no longer existed and read the camera for as long as the page stayed open. Near and Far are held down for the duration, both halves load-bearing: the buttons are disabled, and the handler refuses, for anything that reaches one another way. Uncounted travel mid-measurement invalidates the ratio and the claim about where the lens ended up. Nine more checks. Each was mutation-tested -- breaking the guard it covers turns exactly that check red, and no other.
|
All four findings were real and are fixed in e2197e3. Three share one root: the sweep drives the lens for several seconds and nothing else in the module knew it. 1 — Move failures can strand the lens. Correct, and it is the bug I had already fixed once for the motor buttons ( 2 — A closed editor resumes camera polling. Correct. 3 — Restarted sweeps fight over the lens. Correct. Aborting bumps the generation, which stops the sweep measuring but cannot stop it walking back. A new sweep now waits on 4 — Manual input corrupts a filter sweep. Correct. On the alternatives in the summary: agreed, and for the reason given. A bidirectional sweep accumulates relative-position error on a lens with no position feedback, and every extra step is another chance the return falls short of where the operator left it. TestsNine more checks, and every guard mutation-tested — breaking it turns exactly the check written for it red, and no other:
One of them needed rewriting before it meant anything: sampling the lens position mid-walk proves nothing, since 2 on the way home and 2 on the way out look identical. It asserts on order instead — once a walk back starts, no
|
1 participant
The filter designer could change a filter but gave no way to tell whether the change was an improvement. The only feedback was the live focus number — and that number is a trap.
Measured on the 85H50AI: turning section 3 on took the peak from 25,698 to 55,642 while making the filter worse. That section reads higher as the picture blurs (r = −0.49 against an independent Laplacian ground truth), so it inflates every reading, the out-of-focus ones included. "Make the number go up" is exactly backwards.
What this adds
Measure it walks the lens out eight steps, reads the zone grid at each, walks it back, and reports the ratio — "Falls to 1/12 of its peak across the sweep" — never the peak. Contrast along a defocus sweep is the thing a focus filter is for.
The lens always comes back
The walk-back lives in a
finallyand is not conditional on the sweep still being current. Stopping, leaving the tab, a camera that stops answering and a thrown error all arrive there. Abandoning the operator's focus where an abort happened to leave it is worse than never measuring: the focus is gone and nothing said so.The button exists only where
focus.movedoes — a camera focused by hand cannot be asked to sweep.Tests
Nine new checks in
tests/ui-check.html: no button without a lens; the ratio is reported rather than the peak; the out and back step counts match; it ends at position 0; stopping returns the lens; leaving the tab returns it; a camera that stops answering is reported and the button comes back.Each was mutation-tested — breaking the guard it covers turns exactly that check red:
node tools/smoke.mjsandnode tools/ui-check.mjsboth pass.