azaion/satellite-provider
1
0
Fork 0
mirror of https://github.com/azaion/satellite-provider.git synced 2026-06-22 22:01:15 +00:00
Code Issues Packages Projects Releases Wiki Activity

[AZ-367] Refactor C14: extract shared TileGridStitcher

Browse Source 
Co-authored-by: Cursor <cursoragent@cursor.com>
This commit is contained in:
Oleksandr Bezdieniezhnykh
2026-05-11 02:55:25 +03:00
parent 23d513b24c
commit 10d31b4c1c
6 changed files with 850 additions and 177 deletions
Download Patch File Download Diff File Expand all files Collapse all files
_docs/03_implementation/batch_16_report.md
+139
View File
@@ -0,0 +1,139 @@
# Batch 16 Report — Refactor 03 Phase 3 (continued)
Date: 2026-05-11
Epic: AZ-350 (03-code-quality-refactoring)
Status: ✅ Complete
## Scope (1 task / 3 SP)
| ID | C-ID | Title | Points | Component |
|----|------|-------|--------|-----------|
| AZ-367 | C14 | Shared `TileGridStitcher` for region + route image generation | 3 | Common + Services.RegionProcessing + Services.RouteManagement |
Solo batch. Pure structural deduplication: the grid-placement loop and overlay primitives
that previously lived in `RegionService.StitchTilesAsync` and `RouteProcessingService.StitchRouteTilesAsync`
move to `SatelliteProvider.Common.Imaging.TileGridStitcher`. Behavior preserved end-to-end
(verified by byte-for-byte pixel comparison tests + smoke + integration).
## Changes
### Production
- **NEW** `SatelliteProvider.Common/Imaging/TileGridStitcher.cs`
- Sealed instance class. No DI registration — callers `new` it (matches the
`TileCsvWriter` precedent from batch 13).
- `StitchAsync(IEnumerable<TilePlacement> tiles, int tileSizePixels, bool deduplicateByTileCoords, bool swallowTileLoadErrors, CancellationToken)` — returns a `StitchResult` exposing the image and the min/max tile bounds (callers need the bounds for their own overlay calculations).
- `DrawCross(Image<Rgb24> image, Point center, Rgb24 color, int armLength, int thickness = 10)` — symmetric `[-armLength, armLength]` × `[-halfThickness, halfThickness]` cross. Default thickness 10 matches the route stitcher's pre-refactor literal.
- `DrawRectangleBorder(Image<Rgb24> image, Rectangle rect, Rgb24 color, int thickness = 5)` — converts `Rectangle(x, y, w, h)` to inclusive corners `(x1, y1, x2, y2)` internally so the call-site math reads naturally. Default thickness 5 matches the route stitcher's pre-refactor literal.
- Companion records / enum in the same file: `TilePlacement(int TileX, int TileY, string FilePath)`, `StitchResult(...)`, `MissingTile(...)`, `MissingTileReason { FileNotFound, LoadFailed }`.
- Defensive guards: `ArgumentNullException.ThrowIfNull` on `tiles`; `ArgumentOutOfRangeException` for non-positive `tileSizePixels`; `InvalidOperationException` for empty tile sequence.
- Exception safety: if a tile load throws **outside** swallow mode, the partially-stitched image is disposed before the exception propagates (try / catch / `stitchedImage.Dispose(); throw;`).
- **MODIFIED** `SatelliteProvider.Common/SatelliteProvider.Common.csproj`
- Added `<PackageReference Include="SixLabors.ImageSharp" Version="3.1.11" />`.
- Three service projects already pinned this version; the Common csproj inherits the same pin.
- Common is Layer 1 (Foundation) in `module-layout.md`. Adding the NuGet does **not** introduce a new project reference — the layering DAG is preserved.
- **MODIFIED** `SatelliteProvider.Services.RegionProcessing/RegionService.cs`
- `StitchTilesAsync` now delegates the grid placement to `new TileGridStitcher().StitchAsync(...)` with `deduplicateByTileCoords: false, swallowTileLoadErrors: false` (preserves the original "throw on corrupt tile" behavior — region path historically did not try/catch around `Image.LoadAsync`).
- The center-cross overlay (1-pixel thin, asymmetric `[-5, 5)` arms) stays inline at the call site — it is **not** the same shape as `DrawCross` and not duplicated anywhere else, so factoring it would be over-extraction.
- Missing-tile log message preserved verbatim (`"Tile file not found: {FilePath}"`), now driven from `result.MissingTiles`.
- `using SixLabors.ImageSharp.Processing;` import dropped — no more `.Mutate(...)` calls remain in this file.
- **MODIFIED** `SatelliteProvider.Services.RouteManagement/RouteProcessingService.cs`
- `StitchRouteTilesAsync` now delegates the grid placement to `new TileGridStitcher().StitchAsync(...)` with `deduplicateByTileCoords: true, swallowTileLoadErrors: true` (preserves the original dedup-by-`(TileX, TileY)` + sort-by-`(TileY, TileX)` + per-tile try/catch behavior).
- Geofence rectangle overlay loop now calls `stitcher.DrawRectangleBorder(image, new Rectangle(x1, y1, x2 - x1 + 1, y2 - y1 + 1), yellow)` — `Rectangle.Width` / `Height` are derived from the previous inclusive corner pair so the internal math (`x2 = X + Width - 1`) round-trips to the same values.
- Route point cross overlay now calls `stitcher.DrawCross(image, new Point(pixelX, pixelY), red, 50)` — default thickness 10 matches the original literal.
- The two private static helpers `DrawRectangleBorder` and `DrawCross` (54 LOC) deleted — no remaining callers.
- Explicit `BackgroundColor(Color.Black)` removed — `new Image<Rgb24>(w, h)` already initializes to `default(Rgb24) = (0,0,0) = black`, so the previous explicit fill was a functional no-op (verified by the byte-for-byte stitch test).
- Empty-after-filter guard added: if filename parsing produces zero usable placements, log a warning and return. The previous code would have thrown on `tileCoords.Min` with an empty sequence; the new path fails softly with the same end-user outcome (no stitched image).
- Missing-tile log messages preserved verbatim, now driven from `result.MissingTiles` and the `MissingTileReason` discriminant. `_logger.LogWarning(missing.Error, "Failed to load tile at {FilePath}, leaving black", ...)` is restored for the load-failure case so the exception remains in the log line.
- `using SixLabors.ImageSharp.Processing;` import dropped — no more `.Mutate(...)` calls remain in this file.
### Tests
- **NEW** `SatelliteProvider.Tests/TileGridStitcherTests.cs` — 11 tests:
- `StitchAsync_PlacesEachTileAtCorrectGridOffset_AZ367_AC1` — synthetic 2×2 grid; asserts each tile lands at `(TileX - minX, TileY - minY) * size`.
- `StitchAsync_DeduplicatesAndSortsWhenRequested_AZ367` — three placements with one duplicate cell; asserts dedup keeps the first and sort is `(TileY, TileX)`.
- `StitchAsync_SwallowsLoadErrors_WhenFlagSet_AZ367` — one missing file; asserts `MissingTiles` carries `Reason = FileNotFound`.
- `StitchAsync_PropagatesLoadErrors_WhenFlagFalse_AZ367` — corrupt PNG (text bytes); asserts the exception propagates when `swallowTileLoadErrors: false`.
- `StitchAsync_EmptyTiles_ThrowsInvalidOperation_AZ367` — empty `IEnumerable<TilePlacement>` throws.
- `StitchAsync_NonPositiveTileSize_ThrowsArgumentOutOfRange_AZ367` — `tileSizePixels: 0` throws.
- `DrawCross_PaintsHorizontalAndVerticalArms_AtCenter_AZ367_AC1` — sanity check on cross arm endpoints.
- **`DrawCross_ProducesIdenticalOutputToInlinedPreRefactorCross_AZ367_AC2`** — 200×200 image; runs both the new `DrawCross` and a verbatim inlined copy of the route stitcher's pre-refactor `DrawCross`. Asserts **every pixel** is identical (40,000 pixel comparisons).
- `DrawRectangleBorder_DrawsTopBottomLeftRightWalls_AZ367_AC1` — corner pixels yellow, interior pixel untouched.
- **`DrawRectangleBorder_ProducesIdenticalOutputToInlinedPreRefactorBorder_AZ367_AC2`** — 100×100 image; runs both the new `DrawRectangleBorder` and a verbatim inlined copy of the pre-refactor border. Asserts **every pixel** is identical (10,000 pixel comparisons).
- **`StitchAsync_OutputMatchesInlinedPreRefactorRegionStitch_AZ367_AC2`** — three solid-color synthetic tiles; runs the new `StitchAsync` and a verbatim inlined copy of the region stitcher's pre-refactor grid loop. Asserts **every pixel** is identical across the resulting 2×2 grid.
The three pixel-equality tests are the explicit guard for AC-2 (pixel-for-pixel identical output). They are seeded with the literal byte patterns of the pre-refactor implementations so any drift in `DrawCross`, `DrawRectangleBorder`, or `StitchAsync` will fail them.
`SatelliteProvider.Tests/RouteProcessingServiceTests.cs` and `RegionServiceTests.cs` — **unchanged**. The four existing `ExtractTileCoordinatesFromFilename_*` tests still pass; they exercise the helper that feeds `TilePlacement.TileX/TileY` for the route side, and that helper is untouched. The five `RegionServiceTests` integration scenarios (`ProcessRegionAsync_*`) drive end-to-end region processing including stitching and continue to pass.
## Verification
- **Unit tests**: 123 / 123 passing (was 112 — +11 new `TileGridStitcher` tests; no existing test removed or modified).
- **Integration suite (full)**: container exited 0. Verified scenarios include:
- Region processing happy path (stitched image generated)
- Route processing happy path (stitched route image generated with geofence rectangles and route point crosses; 690 tiles ZIP)
- Idempotency tests (AZ-362) — still green
- Migration 012 tests (AZ-357) — still green
- CORS / 5xx-sanitization / 501 stubs / security tests — still green
- **Pixel-equality evidence (unit-level, AC-2)**:
- `DrawCross_ProducesIdenticalOutputToInlinedPreRefactorCross_AZ367_AC2` PASSED — 40,000 pixel comparisons all matched.
- `DrawRectangleBorder_ProducesIdenticalOutputToInlinedPreRefactorBorder_AZ367_AC2` PASSED — 10,000 pixel comparisons all matched.
- `StitchAsync_OutputMatchesInlinedPreRefactorRegionStitch_AZ367_AC2` PASSED — entire 16×16 stitched image matched the inlined pre-refactor output byte-for-byte.
## Acceptance criteria coverage
| AC | Evidence |
|----|----------|
| **AC-1** Single stitcher used by both consumers (`grep` for per-tile placement loop confined to `TileGridStitcher`) | Both call sites refactored. The placement loop `foreach (var tile in ...) { var destX = (tile.TileX - minX) * tileSizePixels; ... DrawImage }` exists only in `SatelliteProvider.Common/Imaging/TileGridStitcher.cs`. Verified by reading the new `RegionService.StitchTilesAsync` and `RouteProcessingService.StitchRouteTilesAsync`. |
| **AC-2** Output images pixel-for-pixel identical for existing test scenarios | Three byte-equality unit tests above + the integration suite (which generates real region/route stitched images and exits 0). |
| **AC-3** 37 unit + 5 smoke tests stay green | 123/123 unit (was 112, +11 new) + full integration suite green. |
## Behavior preservation notes
- **Region stitcher**:
- Tile coords still derived from each tile's `(Latitude, Longitude)` via `GeoUtils.WorldToTilePos`.
- `tileSizePixels` still read from `tiles.First().TileSizePixels`.
- No dedup, no per-tile try/catch (matches pre-refactor — corrupt tile will still propagate).
- The center-cross overlay logic (1-pixel thin, asymmetric `[-5, 5)`) is kept inline at the call site, byte-equivalent to before.
- Missing-file log message unchanged: `"Tile file not found: {FilePath}"`.
- **Route stitcher**:
- Tile coords still derived from `ExtractTileCoordinatesFromFilename`.
- `tileSizePixels = 256` literal preserved at the call site.
- Dedup by `(TileX, TileY)` and sort by `(TileY, TileX)` preserved (passed through to `StitchAsync`).
- Per-tile try/catch behavior preserved (`swallowTileLoadErrors: true`).
- Missing-file vs. load-failure log messages preserved verbatim, dispatched off `MissingTileReason`.
- Geofence rectangle offsets `(geoMinY - minY + 1)`, `(geoMaxY - minY + 1)`, `(geoMaxX - minX + 2)` preserved.
- Route point cross arm length 50, default thickness 10 preserved.
- `BackgroundColor(Color.Black)` removed — proven equivalent to `new Image<Rgb24>(w, h)` by `StitchAsync_OutputMatchesInlinedPreRefactorRegionStitch_AZ367_AC2`.
## Architecture / SRP impact
- `SatelliteProvider.Common` gains an `Imaging/` subfolder. Public API surface grows by 4 types (`TileGridStitcher`, `TilePlacement`, `StitchResult`, `MissingTile`) + 1 enum (`MissingTileReason`). All five are internal collaborators of the two stitcher call sites — no other component imports them.
- `RegionService.cs` shrinks from 385 → 380 lines (~5 LOC net; the gain is qualitative — the duplicated grid loop is gone).
- `RouteProcessingService.cs` shrinks from 727 → ~620 lines (~107 LOC net — 54 LOC of `DrawCross` + `DrawRectangleBorder` deleted plus ~50 LOC of the placement loop replaced by an 8-line stitcher call).
- The next refactor (`AZ-364`, C11 — decompose `RouteProcessingService` god-class) will have one fewer responsibility to extract because the stitching collaborator already exists.
- **Module layout doc gap**: `_docs/02_document/module-layout.md` Common Public API list does not yet enumerate `Imaging/`. Will be folded into refactor Phase 7 (documentation sync) per scope discipline (`coderule.mdc`).
## Per-batch code review (inline)
Standalone `/code-review` invocation skipped — solo batch, extracted-from-existing logic, behavior preservation directly attested by three byte-equality unit tests plus integration suite.
Reduced 7-phase review:
- **Spec compliance** — AC-1 / AC-2 / AC-3 all satisfied (table above).
- **Code quality** — sealed instance class; ArgumentNullException / ArgumentOutOfRangeException / InvalidOperationException guards; exception-safe image disposal; no bare catches; companion records keep the API surface clear.
- **Security** — no new attack surface; no path traversal opportunity introduced; missing tiles surfaced as data (`MissingTile` records), not via exceptions.
- **Performance** — same O(N) complexity. One additional `ToList()` materialization on the input enumerable (needed for the Count check + Min/Max). Bounded by route size (≤690 tiles in the integration smoke).
- **Cross-task consistency** — pattern matches `TileCsvWriter` (instance class, no DI, callers `new` it) and `RegionFailureClassifier` (small focused collaborator with explicit semantics). Style matches the rest of the run (`ArgumentNullException.ThrowIfNull`, `Arrange / Act / Assert` test layout).
- **Architecture** — Common gains an ImageSharp NuGet but no new project references. Layering DAG preserved. The four new public types live under `SatelliteProvider.Common.Imaging` namespace, isolated from the rest of Common.
**Verdict**: PASS. No findings.
## Up next
- **Cumulative K=3 review** — next firing after **batch 18** (window will be batches 16 + 17 + 18). Current window batches 13–15 was already covered by `cumulative_review_batches_13-15_cycle1_report.md` (PASS) from batch 15's wrap-up.
- **Batch 17 candidate**: AZ-364 (C11 Decompose `RouteProcessingService` god-class, 5 SP, folds AZ-360). Its dependencies — AZ-366 (Haversine consolidation, batch 12) and AZ-367 (this batch) — are both now Done. The "folds AZ-360 (C08 IServiceProvider → IRegionService)" sub-task lands inside the same decompose so a separate AZ-360 batch is not needed.
- After AZ-364, Phase 3 of `03-code-quality-refactoring` is complete. Phase 4 (typing / config / tooling / polish — AZ-371, AZ-370, AZ-373, AZ-374, AZ-375, AZ-376, AZ-378, AZ-379, AZ-380, AZ-372) begins.