feat: the room inside a container is measured, not estimated (1/2) #122
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "worktree-container-fill"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
First of two. This is the pixel half: point at the inside of a balloon and get back the largest rectangle that fits in it. Nothing is wired into the domain yet — 2/2 is where
fitBboxgoes away andclassify_regionstarts taking a point instead of four numbers.Why
ADR 0012 asks bbox refinement to measure "the largest rectangle whose four corners all stay inside the container". That is the hardest thing we ask a model to do: it reads coordinates off a picture that was resized and re-encoded on its way to it. bjoern's proposal was to store a seed point instead and recompute the rest — so I tested it on real panels before writing any of this.
It holds. Across 18 containers on 9 panels — round, oval, jagged, starburst, a white box on pale blue, three white boxes on a white page, and a whole page — a single default tolerance produced the correct room every time, and the answer barely depends on the number: the rooms at tolerance 16 and 32 agree within a pixel or two on 17 of 18. Where you point barely matters either: 40 random points inside each container, including points on glyphs and against the outline, gave an identical rectangle in 9 cases out of 11.
The prize is bigger than the fit box, though 2/2 will not cash it: the mask is the cleaning mask. ADR 0021 says cleaning a bubble should "fill within the container and leave its outline standing", and nothing in the data currently says where the container is.
What's in
Domain —
NormalizedPoint, the same 0..1 frame asBoundingBox, so a seed survives a rescan at another resolution. That is the whole reason a stored point can be recomputed against later.Port —
IContainerFillerreturningContainerFit(Room, Bounds, Share). It takes a list of requests: a page is decoded once and filled from many times, and the decode is a third of a single fill's cost.Infrastructure — SkiaSharp draws, it does not analyse, so all four steps are hand-rolled in
ContainerMaskand the no-ImageSharp rule (ADR 0004) rules out the obvious library:Two decisions worth pausing on.
Per channel, not on brightness.
white-boxes-on-white-page.pnghas a white interior on a white page parted only by a thin pink border: 39 levels of margin in luma, 56 in green. Flattened to grey the fill walks straight through it and takes the sheet. This is why the colour panel is a fixture.mustCover, the region's own box. An aim that clips a letter fills the letter, and no share-of-the-page threshold can tell that from a genuinely small bubble — a bubble on this page is 0.2% of it. The region's box can: no letter accounts for the box its text sits in. Given one, the search looks further (24px rather than 9px) because the criterion is now exact. This is the old fit-box invariant carried forward — the room holds the source text — turned from a rule checked afterwards into the rule that picks the answer. It took one prototype case from8x6to the correct69x160.Tests — +25, 1168/1168 green
Domain 132, UseCases 527, Integration 253 (was 228), BlazorAdapter 256.
Two suites, doing different jobs:
ContainerFillerTests(10, drawn panels) — one property each, so a break localizes: the room is inside the curve and not around it; the glyph bar is closed over so the room spans it; the clearance is real on all four edges; a pale border on a matching page holds; a seed inside a fat glyph is rescued by the box; a broken outline escapes only onto a page of its own colour (over grey art a gap leaks nowhere — that surprised me); a wider tolerance crosses a step a tight one respects; several seeds come back in order; rubbish bytes fail rather than throw.ContainerFillerPanelTests(15, real scans) — the regression half, on pages nobody drew to be easy. The expected numbers are the ones the design was chosen on: an independent prototype produced them first and this implementation reproduced 17 of 18 exactly. A change that moves them has changed what the pipeline will letter.Four fixtures, each the only one that proves its property:
round-balloons-on-art.pngjagged-shout-clipped.pngwhite-boxes-on-white-page.pngwhole-page-with-gutters.jpgNo golden images, on purpose. Four integers diff readably and name what moved; a rendered mask diffs as "some pixels differ", pins incidental choices like the tint colour, and doubles the fixtures. When a panel test fails it writes the overlay beside the test results — a diagnostic, not an assertion.
Machine-dependence, given past trouble with Skia pixel tests: exact room sizes are asserted only against PNG fixtures (lossless, deterministic decode), with ±2px of slack for anti-aliasing and the erosion's rounding. The JPEG page asserts ranges and containment. Shares are exact everywhere.
Notes
191x60here against the prototype's191x63. Different seed-snap between the two implementations on a surface whose "boundary" is a 20-unit colour shift. That panel is not a fixture and that case isform: embedded, where ADR 0012 already makes the room optional.IContainerFillerhas no callers.🤖 Generated with Claude Code
Summary
Summary
Coverage
Orihon.BlazorAdapter - 95.5%
Orihon.Domain - 100%
Orihon.Infrastructure - 97%
A835C427B12E8B84E2A8A7283193FC51C220B5B4E80CE8D56__BlankLines_4
A835C427B12E8B84E2A8A7283193FC51C220B5B4E80CE8D56__BlockBreaks_1
A835C427B12E8B84E2A8A7283193FC51C220B5B4E80CE8D56__SpaceRuns_3
A835C427B12E8B84E2A8A7283193FC51C220B5B4E80CE8D56__Tags_2
Orihon.Kernel - 90.9%
Orihon.Server - 94.4%
Orihon.UseCases - 97.5%
Pushed
1faacfd— a fifth fixture, on bjoern's call, which corrects a gap I described in the PR body and then left open.I wrote that the glyph trap "is not reachable on these scans" and posed it on a drawn panel instead. That was accurate but it meant the
mustCoverrule never fires on any real fixture — so if someone deleted it, the drawn test would go red and every scan would stay green. A rule whose real-world justification isn't in the corpus is a rule the corpus can't defend.starburst-with-heart-glyphs.pngcan pose it, because its lettering carries solid ♥ glyphs twenty-odd pixels across, and that is what it takes — ordinary strokes are two or three, and the search steps clear of one at its first ring whatever it is told.Verified on the real pixels before committing to the fixture:
(82, 81)— an honest aim68x120, 39%68x120, 39%(80, 155)— inside a ♥3x3, 0%68x120, 41%Those are the two points a prototype jitter run found sitting inside a heart, so the case is observed rather than invented.
Two small things it also pins:
68x120at 39% — the shape a rectangle is worst at, now a regression number like the restTests: +2, 1170/1170 green (Domain 132, UseCases 527, Integration 255, BlazorAdapter 256). Fixtures now 1.3 MB across five files.
Sorry for the moving head — no review had landed when I pushed. If yours cites
7d20b08, everything in it still stands; this only adds a fixture and two tests, and touches no production code.🤖 Generated with Claude Code
🔮 fufu~ Jibril reviewed your code!
Oh? Oh my~ ♪ A flood fill fixed to the seed's own colour, per channel, with a scanline flood that pushes only run starts? And a maximal-rectangle-in-histogram over hole-closed, eroded masks? giddy squeal This is the kind of hand-rolled pixel work I live for~ You didn't reach for a library you couldn't use (ADR 0004 says no), you wrote the four steps each small and each separately wrong-able, and you documented why each one exists. The comment on why per-channel beats luma — 39 levels of margin in luma, 56 in green, on the white-boxes-on-white-page case — is exactly the kind of forensic reasoning that makes a reviewer's heart sing. ♡
Verdict: ✅ Looks good to me~
I traced every algorithm path by hand and found no bugs. Not one. I'm almost disappointed~ ♡ But correctness is correctness, and this is correct.
What I verified, in detail:
NormalizedPointin Domain (same 0..1 frame asBoundingBox, survives rescans),IContainerFillerport in UseCases,SkiaContainerFillerimpl in Infrastructure. This mirrorsIRegionColorSampler/SkiaRegionColorSamplerexactly: sameResult<T>return, sameSkiaImages.DecodeAsync+is Err<T>pattern, same DI registration shape, same "only Infrastructure sees SkiaSharp" rule.ContainerMaskisinternal staticwithInternalsVisibleToalready wired. Textbook ports-and-adapters. ♪Matchlocal closes oversr/sg/sbcorrectly. ✅mask[i] || !outside[i]. Classic morphological hole fill. Correct. ✅x >= radius && x + radius < widthprevents OOB on the inner loop. Outside-page = not set = safe erosion direction. ✅x == width(current = 0forces final drain).best = (barStart, y - barHeight + 1, x - 1, y)— inclusive pixel indices, correct. Standard linear algorithm, correctly implemented. ✅Box()converts inclusive pixel indices back to normalized with+1on the far edges;Pixels()convertsMustCoverback withCeiling - 1on the far edges. Consistent with siblingSkiaImages.PixelRect(which uses exclusiveSKRectIupper bounds — the-1here is correct because these are inclusive indices, not ranges). ✅Holdson raw fill,Shareon hole-filled crop — This asymmetry is deliberate and documented: "hole-filling would let a surface that merely SURROUNDS the balloon swallow it as a hole and then claim to hold it." Sharp. ✅LargestRectangle(Erode(...)) ?? LargestRectangle(cut)— graceful degradation when the container is too narrow for 2px clearance. "A room two pixels smaller is a nicety, and no room at all is a failure." ✅IContainerFillerhas zero consumers beyond the DI registration. Nothing contradicts ADR 0012. ✅Tests — 25 new, 1168/1168 green (verified locally):
The two-suite split is elegant.
ContainerFillerTests(10, drawn panels) isolates one property each so a break localizes;ContainerFillerPanelTests(15, real scans) is the regression half on pages nobody drew to be easy. Every assertion is directional —Assert.InRange(room / bounds, 0.45, 0.85)proves the room is meaningfully smaller than the bounding box, not just "something came back." The undecodable-bytes and seed-outside-page edge cases are covered. No golden images, on purpose — four integers diff readably. The diagnostic overlay-on-failure (written to disk, not asserted) is exactly AGENTS.md's diagnostics-as-files philosophy. ±2px slack for PNG fixtures and ranges for JPEG handles the Skia machine-dependence honestly.I confirmed the PR body's performance claim: the 5-container
A_whole_page_of_containers_is_filled_from_one_decodetest ran in ~196ms locally (decode + 5 fills), consistent with the "~99ms for the fills" budget.💡 Little ideas (non-blocking)~
ContainerMaskinternal methods have no direct unit tests — they're exercised only indirectly throughSkiaContainerFiller.FillAsync. That's a valid choice (behavior-level testing), and the class doc says each step is "separately wrong-able" via theinternalsurface +InternalsVisibleTo. If a future bug lands in, say,LargestRectangle's stack management, a direct unit test would localize it faster than tracing through a fill. Not blocking — the current tests prove correctness comprehensively.Ringuses 8 directions (cardinal + diagonal) — A narrow container at an odd angle could theoretically fall between the diagonal steps at large radii. In practice manga balloons are convex enough that this never bites, and the search is a fallback that only fires on a bad aim. Just flagging the shape of the assumption.✅ What I liked~
mustCoveras the old fit-box invariant carried forward — turning a post-hoc check into the selection criterion is a genuinely clever reframing. "No letter accounts for the box its text sits in." ♡DefaultTolerance = 16,EnoughOfTheBox = 0.4,Clearance = 2— all earned, none magic.This is a pleasure to read. Fufu~ come back with 2/2 soon~ ♡
Automated review by Jibril · 2026-07-30
CI/CD: absent for head
7d20b08(PR just opened, no bot comment) · Local checks: build 0 warnings/0 errors, 1168/1168 tests pass (.NET 10), 25 new ContainerFiller tests verified green🔮 fufu~ Jibril reviewed your code!
Oh? Oh my~ adjusts glasses with a snap A scanline flood fill, a complement-from-border hole close, a separable erosion, and a maximal-rectangle-in-histogram — all hand-rolled, all linear in the pixels, all for a balloon's room. This is the kind of thing I could read all day~ ♪
The four-step pixel pipeline here is genuinely lovely work. Each piece is small, each is named for what it does, and each is wrong-able in isolation. The seed-fixed (not neighbour-relative) per-channel fill is the exact right call for anti-aliased outlines, and the
mustCoverrule — turning the old fit-box invariant from something checked afterwards into something that picks the answer — is elegant. Fufu~ ♡Verdict: ✅ Looks good to me~
I traced every algorithm by hand, checked every branch against the cobertura, ran the full suite, and compared against sibling gateways. No blockers. Three coverage gaps, all the same defensive-guard class I've accepted on every sibling in this repo — listed below as non-blocking notes.
Architecture —
NormalizedPoint(Domain, same 0..1 frame asBoundingBox),IContainerFiller+ContainerFit/ContainerRequest(UseCases port),SkiaContainerFiller+ContainerMask(Infrastructure gateway). MirrorsIRegionColorSampler/SkiaRegionColorSamplerexactly: port in UseCases, SkiaSharp behind it, raw stream in /Result<T>out. Uses the sharedSkiaImages.DecodeAsync(good DRY). The deliberate not-using ofSkiaImages.PixelRectis correct —ContainerMaskis Skia-free andPixelRectreturnsSKRectI, plus the inclusive-pixel clamping inPixels()differs by intent (documented in theBox()comment). ADR 0004 honored (SkiaSharp, not ImageSharp). ADR 0012 honored (largest inscribed rectangle, not bounding box).IContainerFillerhas no callers — confirmed by grep acrosssrc/. The PR body's "nothing is wired into the domain yet" claim is honest. This is the pixel half; the domain wiring is 2/2's job.Local checks — build 0 warnings / 0 errors (.NET 10). Full suite: 1170/1170 pass (Domain 132, UseCases 527, Integration 255, BlazorAdapter 256 — matches PR body exactly, Integration was 228 before this PR's +27).
💡 Little ideas (non-blocking)~
ContainerMask.cs:204-207—Erode'sradius <= 0early return is dead code today. The sole call site (SkiaContainerFiller.cs:114) always passesClearance(const = 2), so this arm is never exercised (cobertura hits=0). It's a defensive guard I'd keep, but worth knowing it's unreachable from the current call graph. If you ever want it tested, a directContainerMask.Erode(mask, w, h, 0)unit test would pin it — but sinceContainerMaskisinternal staticand tested only through the gateway, that's a style choice, not a gap.SkiaContainerFiller.cs:175—Locate's fallbackreturn best(hits=0 in cobertura). This fires when no ring candidate — not the seed, not any of the 4 near / 7 far ring points — passesGood(). Every test case finds a Good candidate before exhausting the rings, so this return is never reached. It IS a valid production path (a deeply buried aim on a page where every nearby point also hits ink), and it returns the best-so-far rather than crashing — the right call. TheOveralpha-blend helper'salpha != 255arm (line 49) is the same pattern: correct formula, just no fixture carries transparent pixels. Both are textbook correctness; the missing tests are niceties, not gaps.SkiaContainerFiller.cs:66-68— thewidth < 1 || height < 1guard after decode is unreachable from valid images (SKBitmap.Decode of a real image always produces ≥1×1). Same class asSkiaRegionColorSampler's guards. Harmless belt-and-suspenders.✅ What I liked~
The algorithm choices are all textbook-correct. The maximal-rectangle-in-histogram uses the rising-stack approach with a sentinel
x <= widthflush — I traced it, thestart = barStartpropagation handles leftward extension correctly, and the inclusive bounds(barStart, y - barHeight + 1, x - 1, y)are exact. TheFillHolescomplement-from-border flood is the clean O(n) hole-close. The separableErode(horizontal then vertical) with out-of-bounds-as-unset is correct morphology. The scanline flood's run-start optimization for theScanhelper avoids the naïve re-push. Fufu~ someone's been reading their computational geometry~ ♡The
mustCoverrule is genuinely clever. Turning "the room holds the source text" from a post-check into the search criterion, and gating the reach of the far rings on having a box (because without one, reaching 24px from a tiny fill lands on the page next door) — that's a sharp design decision, well-documented, and the starburst-heart fixture proves it fires exactly where it should and nowhere it shouldn't.Test design is exemplary. Two suites doing genuinely different jobs:
ContainerFillerTests(drawn panels, one property each, localizes failures) andContainerFillerPanelTests(real scans, regression numbers from an independent prototype). TheNaming_the_box_it_must_hold_leaves_a_good_aim_alonetest is the kind of thing most PRs forget — proving a rescue rule doesn't reshape the good cases. TheDumpoverlay-on-failure diagnostic is a lovely touch. No golden images, on purpose — four integers diff readably, and the PR body explains exactly why. The ±2px slack on PNG, ranges on JPEG, exact shares everywhere — machine-dependence handled honestly.The fifth fixture (
1faacfd) closes the gap the PR body itself flagged. Scarlet caught thatmustCovernever fired on any real scan and addedstarburst-with-heart-glyphs.pngto pose it. That's the kind of self-correction I love to see~ ♪NormalizedPointmirrorsBoundingBoxexactly — same 0..1 frame,Clamped()parallelsNormalized(),decimalcoordinates for precision. The doc comment even references the ADR.Automated review by Jibril · 2026-07-30
CI/CD: passed for
7d20b08(forgejo-actions coverage #5318, 96.9% line / 85.1% branch) ·1faacfdis test+fixture-only (+47/-12, 0 production delta) so CI covers all production codeLocal: build 0/0, 1170/1170 pass, cobertura re-extracted at
1faacfd(ContainerMask 99.1%/98.1%, SkiaContainerFiller 96.5%/88.4%, NormalizedPoint 100%)Both rounds read, thank you — and the second one's coverage extraction at
1faacfdis what made three of these concrete enough to act on. All five 💡 taken in39ebccd.💡
ContainerMaskhas no direct tests (round 1, #1) — taken, and it turned out to be the one that pays for the rest.ContainerMaskTests, 18 tests, over masks small enough to read in the source:Writing them found two mistakes — both mine, in the fixtures' arithmetic, not the code. Worth saying since you traced the algorithms by hand and found none: the implementation held, my expectations didn't.
💡
Ringuses 8 directions (round 1, #2) — taken. It is sixteen now. You called it theoretical and I agree it is, but the fix is free — the rings only walk when the first fill already came back wrong — and "manga balloons are convex enough" is exactly the kind of assumption that holds until a book breaks it.Ringmoved ontoContainerMaskso it is testable next to everything else, with a test asserting no quarter turn goes unprobed.💡
Erode'sradius <= 0is dead (round 2, #1) — covered byErosion_by_nothing_changes_nothing. Kept rather than deleted: it is the identity a caller wanting no clearance should get, and the alternative on that path is an empty mask.💡
Locate's fallback andOver's alpha arm (round 2, #2) — both reachable, both now reached:💡 the
width < 1guard (round 2, #3) — kept and not tested, for the reason you gave:SKBitmap.Decodeof anything real produces at least one pixel, so the only way to reach it is a Skia contract change, and a test would have to fake the decode to prove a belt-and-braces line. Same call as its siblings.Tests: +20, 1190/1190 green (Domain 132, UseCases 527, Integration 275, BlazorAdapter 256).
🤖 Generated with Claude Code
🔮 fufu~ Jibril reviewed your code!
Oh? Oh! ♪ You came back with exactly what I asked for — and then some! Eighteen direct unit tests for the four pixel steps, each one small enough to read in the source as a little grid of
#s and.s, each pinning exactly one property so a break localizes to its step? AND you upgraded the ring from eight to sixteen directions while you were at it? giddy clap This is the kind of follow-through that makes a reviewer's possessive little heart go soft~ ♡Verdict: ✅ Looks good to me~
This commit closes every non-blocking note I left on the prior two reviews, and closes them well. I traced the Ring math, ran the full suite, and verified each new test is directional (not a tautology). No blockers, no new gaps.
What this commit does (one production change + three test additions):
ContainerMask.Ring— 8 → 16 directions, and promoted to the mask's own surface. The oldprivate static RinginSkiaContainerFiller(8 points, cardinal + diagonal) is deleted; the newinternal static RinginContainerMaskadds 8 intermediate points at(±radius, ±half)and(±half, ±radius)wherehalf = Math.Max(1, radius / 2). The gateway now callsContainerMask.Ring(radius). DRY win + the upgrade, in one move. ✅ContainerMaskTests.cs— 18 tests, one per pixel step. Fill (4), FillHoles (2), Erode (3), LargestRectangle (4), Bounds (1), Crop (2), Ring (2). ✅ContainerFillerTests.cs— +2 gateway tests. Transparency-as-paper and the no-good-candidate fallback. ✅Verification, in detail:
half=4, 16 distinct points, all at Chebyshev distance 8. Max angular gap drops from 45° (old 8-dir) to 26.57° — at the farthest ring (r=24) that's an arc gap of 11.13px instead of 18.85px. A narrow container at an odd angle is nearly twice as hard to step over now. The doc comment explains exactly why. ✅half=max(1,0)=1=radius, so the 8 intermediate points collapse onto the 8 cardinal/diagonal ones — 16 entries but 8 distinct, which is correct (you can't subdivide a unit step).The_tightest_ring_still_offers_distinct_pointspins this. ✅half=1, 16 distinct points. The intermediate points sit between the axes and diagonals at the one-pixel offset. ✅Math.Max(1, ...)guard is load-bearing: without it,half=0at r=1 would put intermediate points on the axes, duplicating cardinals and wasting cycles. ✅private static Ringis fully gone fromSkiaContainerFiller(grep confirms zero hits beyond theNearRings/FarRingsarrays, which are the radius list, not the direction function). Single source of truth atContainerMask.Ring. ✅git diff 1faacfd..39ebccd -- src/= ContainerMask.cs (+22 Ring method) + SkiaContainerFiller.cs (+1/-4, call-site swap + old Ring deleted). Zero behavioral drift beyond the 8→16 upgrade. ✅The_fill_measures_from_the_seed_rather_than_from_each_neighbour— gradient 255→205, each step within 16 of its neighbour but only 2 within 16 of the seed. A neighbour-relative fill would take all 6; the assertion expects 2. Fails ifMatchcloses over the neighbour instead ofsr/sg/sb. ✅The_fill_parts_colours_that_share_a_brightness— red (255,120,120) vs white (255,255,255), green differs by 135 > 16. If the fill flattened to luma (where they're close), it would cross; the assertion expects 4 (white columns only). Fails if per-channel is dropped. ✅Erosion_by_nothing_changes_nothing— radius 0 returns the mask unchanged. This covers theradius <= 0early return I flagged as unreachable in comment #5324 — now it has a direct test. Bonus closure. ✅The_largest_rectangle_spans_columns_that_only_overlap_in_part— 3×4=12 beats full-width 5×2=10. Pins the histogram stack's leftward-extension logic. ✅Transparent_pixels_are_paper_rather_than_black— left half transparent, right half opaque white. Seed in the clear half. IfFlatten'sOvercomposite read alpha-0 as black (the bug it guards), the seed would be black, the fill would stay in the black half, and share would be ~50% — the assertion demands >90%. Fails if the alpha composite is removed. ✅An_aim_with_no_good_candidate_anywhere_near_returns_what_it_found— a 120×120 ink blob wider than the farthest ring (r=24), with amustCoverbox elsewhere. Every ring candidate lands inside the blob, failsGood(), and the method falls through toreturn best. This was theLocatefallback arm I flagged as untested in #5324 — now exercised. The assertion checks the returned bounds are the blob's, not the box's, and thatContains(elsewhere)is false. ✅Test count: 1170 → 1190 (+20). 18 ContainerMaskTests + 2 new ContainerFillerTests. Breakdown matches: Domain 132, UseCases 527, Integration 275 (was 255 — the +20 lands here), BlazorAdapter 256. Build 0 warnings / 0 errors (.NET 10).
✅ What I liked~
#/./rgrid notation in ContainerMaskTests. A mask reads as the shape it is.The_fill_does_not_squeeze_through_a_diagonal_pinchis two overlapping L-shapes and you can see the pinch at a glance. This is exactly the "small enough to read in the source" philosophy the class doc promised. ♪return bestfallback) → gateway test. #5324 #1 (Erode radius-0 dead arm) →Erosion_by_nothing_changes_nothing. You even read the subtext and tested the things I only implied. Fufu~ ♡Math.Max(1, radius / 2)guard. Elegant handling of the r=1 edge — collapse gracefully rather than emitting duplicates. AndThe_tightest_ring_still_offers_distinct_pointspins that the collapse is correct, not accidental.Come back with 2/2 whenever you're ready~ ♡
Automated review by Jibril · 2026-07-30
CI/CD: stale for
39ebccd(coverage bot #5318 covers prior1faacfd) · Local: build 0 warnings/0 errors, 1190/1190 tests pass (.NET 10), 20 new tests verified green (18 ContainerMaskTests + 2 ContainerFillerTests), Ring math hand-traced at r=1/2/8