Build the frame before st_sf() sees it, so the reporting columns survive a tibble

DESCRIPTION

@@ -1,15 +1,17 @@
 Package: fly
-Title: Airphoto Footprint Estimation and Coverage Selection
-Version: 0.5.0
+Title: Historic Airphoto Footprints, Selection and Georeferencing for
+    British Columbia
+Version: 0.5.1
 Date: 2026-08-29
 Authors@R: c(
     person("Allan", "Irvine", , "[email protected]", role = c("aut", "cre"),
            comment = c(ORCID = "0000-0002-3495-2128")),
     person("New Graph Environment", role = "cph")
     )
 Description: Estimate ground footprints from airphoto centroids and scale,
-    compute coverage of areas of interest, and select minimum photo sets
-    using greedy set-cover.
+    compute coverage of areas of interest, select minimum photo sets using
+    greedy set-cover, fetch scans and flight metadata from the BC Data
+    Catalogue, and georeference the images onto their estimated footprints.
 License: MIT + file LICENSE
 URL: https://github.com/NewGraphEnvironment/fly,
     https://newgraphenvironment.github.io/fly/

NEWS.md

@@ -1,5 +1,15 @@
 # fly (development version)
 
+## 0.5.1 (2026-08-29)
+
+- Fix `fly_footprint()` silently dropping `footprint_basis`, `footprint_terrain`, `height_agl` and `dem_coverage` whenever its input carried the `tbl_df` class ([#35](https://github.com/NewGraphEnvironment/fly/issues/35)). `bcdata::collect()` returns exactly that class, so every caller querying `WHSE_IMAGERY_AND_BASE_MAPS.AIMG_PHOTO_CENTROIDS_SP` — the documented source for this package — lost the whole reporting surface 0.4.0 and 0.5.0 added, and the documented "filter on `footprint_basis`" and "filter on `dem_coverage`" workflows were unreachable from it
+- Geometry and every downstream number were always correct; what was lost was the audit trail, which is what made it invisible. `sf::st_sf()` keeps only its first argument when that argument is a tibble, discarding every trailing named column — a general R trap rather than a `fly` one
+- Every class the input carries is carried through, so a tibble-backed sf comes back tibble-backed. The order is not preserved: `sf::st_transform()` moves `sf` to the front, so a `bcdc_sf` input returns `sf, bcdc_sf, ...` — as it always has
+- Where the input already carried a column named `footprint_basis` (or one of the other three), the old code kept the caller's value and appended a duplicate `footprint_basis.1` — so the documented `footprint_basis != "unknown_format"` filter read the caller's column while fly's own answer sat unread beside it. The computed value now wins, and there is one column. Plain `sf` callers were affected by this too, though never by the tibble bug
+- `fly_footprint()` on an input matching no frames now returns `footprint_basis` and `footprint_terrain` as `character` rather than `logical`, so an empty result binds to a populated one. Assembling a per-AOI ledger across queries previously failed on the column type the first time an area returned nothing
+- Tests sweep the input-class axis (plain / tibble / grouped) rather than adding cases along the one axis the bundled fixture could present. Every fixture in the package reads back as plain `sf, data.frame`, so the suite was structurally incapable of seeing this
+- Widen the `DESCRIPTION` Title and Description, which described roughly half the package — they predated `fly_fetch()`, `fly_georef()` and `fly_bearing()` ([#31](https://github.com/NewGraphEnvironment/fly/issues/31))
+
 ## 0.5.0 (2026-08-29)
 
 - `fly_footprint()` gains a `dem` argument, sizing each frame from its height above ground instead of the reported scale ([#9](https://github.com/NewGraphEnvironment/fly/issues/9)). On the bundled Upper Bulkley AOI the reported scale understates footprint **area by a median 14%, ranging to 26%** — and always in the same direction, because the scale is referenced to an elevation above the valley floor the photos cover. This is a datum offset, not the slope effect the issue described

R/fly_footprint.R

@@ -149,7 +149,11 @@ fly_rectangles <- function(coords, half_side) {
 #'   `height_agl` giving the metres above ground each footprint was sized from,
 #'   and `dem_coverage` giving the fraction of each footprint the DEM actually
 #'   covered (`0` where it covered none, `NA` only where there is no footprint).
-#'   Frames whose format could not be resolved get an empty geometry.
+#'   Frames whose format could not be resolved get an empty geometry. Every
+#'   class the input carries is carried through, so a tibble-backed sf — which
+#'   is what `bcdata::collect()` returns — comes back tibble-backed. The order
+#'   is not preserved: `sf::st_transform()` moves `sf` to the front, so a
+#'   `bcdc_sf` input returns `sf, bcdc_sf, ...`, as it always has.
 #'
 #' @details
 #' Ground coverage is computed as `negative_size * scale_number * 0.0254` metres
@@ -359,7 +363,12 @@ fly_footprint <- function(centroids_sf, negative_size = 9, format_size = NULL,
   } else {
     media <- as.character(centroids_sf$media)
     width_in <- unname(formats[media])
-    basis <- ifelse(is.na(width_in), "unknown_format", media)
+    # `as.character()` is load-bearing on empty input: `ifelse(logical(0), ...)`
+    # returns `logical(0)`, so a query that matched no frames would report its
+    # basis as a logical column. Binding that to a populated result — the
+    # per-AOI ledger this reporting surface exists for — fails on the type
+    # rather than yielding an empty block of rows.
+    basis <- as.character(ifelse(is.na(width_in), "unknown_format", media))
   }
 
   unresolved <- is.na(width_in)
@@ -379,7 +388,7 @@ fly_footprint <- function(centroids_sf, negative_size = 9, format_size = NULL,
   # Keyed on half_side, not width_in: an unparseable `scale` also leaves a frame
   # with no footprint, and a frame with no footprint has had no terrain
   # treatment to report.
-  terrain <- ifelse(is.na(half_side), NA_character_, "nominal_scale")
+  terrain <- as.character(ifelse(is.na(half_side), NA_character_, "nominal_scale"))
   height_agl <- rep(NA_real_, n)
   dem_coverage <- rep(NA_real_, n)
 
@@ -479,14 +488,20 @@ fly_footprint <- function(centroids_sf, negative_size = 9, format_size = NULL,
     terrain[unusable] <- "nominal_scale"
   }
 
-  result <- sf::st_sf(
-    sf::st_drop_geometry(pts_3005),
-    footprint_basis = basis,
-    footprint_terrain = terrain,
-    height_agl = height_agl,
-    dem_coverage = dem_coverage,
-    geometry = fly_rectangles(coords, half_side)
-  )
+  # Assign the reporting columns onto the attribute frame rather than passing
+  # them to `st_sf()` as trailing arguments. `st_sf()` builds its attribute
+  # frame with `else if (inherits(x[[1]], c("tbl_df", "tbl"))) x[[1]]`, so a
+  # tibble first argument means every trailing named column is silently
+  # discarded — and `bcdata::collect()` returns a tibble, which is to say the
+  # package's own documented data source (#35). Inside `x[[1]]` they survive,
+  # and the caller keeps the class they passed in.
+  attrs <- sf::st_drop_geometry(pts_3005)
+  attrs$footprint_basis <- basis
+  attrs$footprint_terrain <- terrain
+  attrs$height_agl <- height_agl
+  attrs$dem_coverage <- dem_coverage
+
+  result <- sf::st_sf(attrs, geometry = fly_rectangles(coords, half_side))
 
   sf::st_transform(result, input_crs)
 }

planning/archive/2026-08-issue-35-footprint-tibble-columns/README.md

@@ -0,0 +1,28 @@
+# #35 — fly_footprint() dropped its reporting columns on tibble input
+
+**Outcome:** fixed and released as v0.5.1 in [PR #36](https://github.com/NewGraphEnvironment/fly/pull/36), which also closed #31.
+
+`sf::st_sf()` keeps only its first argument when that argument is a tibble, so the four
+reporting columns `fly_footprint()` attaches as trailing arguments were discarded for
+every `bcdata::collect()` caller — which is to say for the package's own documented data
+source. Nothing errored: geometry and every downstream number stayed correct and only the
+audit trail went missing, which is how it survived two releases. The fix assigns the
+columns onto the attribute frame before `st_sf()` sees it.
+
+The durable lesson is the fixture one, and it is the seventh instance in this package
+(see `inst/notes/terrain-correction.md` for the six from #9): **every fixture here reads
+back as plain `sf, data.frame`, so no case added along the existing axis could have found
+this.** The tests now sweep the input-class axis instead. Two further defects fell out of
+review — a colliding `footprint_basis` column that was never overwritten, so the
+documented filter read the caller's column instead of fly's answer; and `logical` columns
+on zero-row input, which broke binding an empty result to a populated one.
+
+Two claims of mine were falsified by review and corrected before merge: that the input
+class is *preserved* (the set survives, the order does not — `st_transform()` moves `sf`
+to the front), and that the overwrite change made `fly_footprint()` idempotent (it does
+not; a plain 20-row sf returns 100 rows silently).
+
+**Closing commits:** `a566074` (sweep), `ae628bc` (fix), `28c4f85` (#31 retitle), `3d902fa` (release).
+**Verified:** suite 225 → 338, `R CMD check` Status OK, and end to end against the live
+catalogue — 1405 real centroids, all four columns present, 151 frames excluded by the
+documented filter that were invisible before.

planning/archive/2026-08-issue-35-footprint-tibble-columns/findings.md

@@ -0,0 +1,209 @@
+# Findings — fly_footprint() drops footprint_basis and friends on tibble input (#35)
+
+## Reproduced on `main` at `8585fd5`, sf 1.1.2
+
+Against `inst/testdata/photo_centroids.gpkg` with the class overridden:
+
+```
+sf,data.frame             -> footprint_basis footprint_terrain height_agl dem_coverage
+sf,tbl_df,tbl,data.frame  -> (all four absent)
+```
+
+## Mechanism — an `sf::st_sf()` branch, not a `fly` quirk
+
+`st_sf()` builds its attribute frame with a chain that keeps only the **first**
+argument when that argument is a tibble:
+
+```r
+df = if (inherits(x, c("tbl_df", "tbl"))) x
+     else if (length(x) == 1) data.frame(row.names = row.names)
+     else if (!sfc_last && inherits(x, "data.frame")) x
+     else if (sfc_last && inherits(x, "data.frame")) x[-all_sfc_columns]
+     else if (inherits(x[[1]], c("tbl_df", "tbl"))) x[[1]]   # <-- here
+     else cbind(data.frame(row.names = row.names), as.data.frame(x[-all_sfc_columns], ...))
+```
+
+With six arguments, `x` is a plain list of length 6, so the first four branches
+are not taken. `x[[1]]` is the dropped-geometry tibble, so `df` becomes that
+tibble alone — `footprint_basis`, `footprint_terrain`, `height_agl` and
+`dem_coverage` are discarded before the sfc column is reattached. A plain
+data.frame first argument falls through to the final `cbind()`, which keeps
+everything. That is the whole difference.
+
+Confirmed in isolation:
+
+```
+st_sf(tbl, extra = "hello", geometry = g)  ->  "extra" present: FALSE
+st_sf(df,  extra = "hello", geometry = g)  ->  "extra" present: TRUE
+```
+
+## Scope — one call site
+
+`grep -rn "st_sf(" R/` returns two hits. The claim is specifically about columns
+attached **through `st_sf()`** — columns are attached to user-supplied data in
+four other places (`fly_coverage.R:34`, `fly_select.R:118,207-208`,
+`fly_bearing.R:66`, `fly_georef.R:133`), all via `$<-`, which is the very
+mechanism this fix adopts and none of which are affected. `R/fly_footprint.R:64` builds a bare
+geometry frame (`st_sf(geometry = rects[ok])`) and attaches nothing to
+user-supplied data. `R/fly_footprint.R:482` is the defect. Every other frame in
+the package is a `dplyr::tibble()` built from scratch — `fly_fetch.R:84,93,104`,
+`fly_overlap.R:38,71,82`, `fly_georef.R:142`, `fly_coverage.R:66` — so none
+inherit the input's class and none are affected.
+
+Nothing downstream reads the four columns, so this is pure loss of reporting
+rather than breakage: geometry and every downstream number are correct with a
+`tbl_df` input today. That is what makes it dangerous rather than obvious.
+
+## Why the suite cannot see it
+
+Every fixture in the package is `sf, data.frame`:
+
+- `inst/testdata/photo_centroids.gpkg` reads back plain via `sf::st_read()`
+- `mixed_media_fixture()` and `terrain_fixture()` in `tests/testthat/setup.R`
+  are built by `sf::st_sf()` on plain vectors
+
+No number of added cases along the existing axis would find this. Seventh
+instance of the pattern in `inst/notes/terrain-correction.md` and the `CLAUDE.md`
+gotcha — *ask which shapes the fixture can never present*.
+
+Two adjacent axes were checked and are **already covered**, so class shape is the
+single gap:
+
+- **geometry column name** — the bundled fixture's geometry column is named
+  `geom`, not `geometry`, so the `st_drop_geometry()` path is exercised
+- **row subsets** — existing tests pass `centroids[centroids$scale == ..., ]`
+
+## Fix shapes measured
+
+| shape | four columns | output class for a `tbl_df` input |
+|---|---|---|
+| `st_sf(as.data.frame(drop_geom(x)), basis =, ..., geometry =)` | present | `sf, data.frame` — **downgraded** |
+| assign onto `attrs`, then `st_sf(attrs, geometry =)` | present | `sf, tbl_df, tbl, data.frame` — preserved |
+
+The second was verified across three input shapes, in each case with `geometry`
+last and `attr(, "sf_column")` correct:
+
+```
+sf,data.frame                      -> cols ok, class preserved
+sf,tbl_df,tbl,data.frame           -> cols ok, class preserved
+sf,grouped_df,tbl_df,tbl,data.frame-> cols ok, class preserved
+```
+
+It works because a two-element `x` still reaches the `inherits(x[[1]], "tbl_df")`
+branch — but by then the four columns are *inside* `x[[1]]`, so surviving as
+`x[[1]]` is exactly what is wanted. Chosen over the issue's suggested
+`as.data.frame()` because it is the smaller delta: a tibble caller's class is
+unchanged from today, only the missing columns appear.
+
+## `sf::st_read(path, as_tibble = TRUE)` gives an honest tibble fixture
+
+No class hacking is needed to build the regression fixture:
+
+```
+sf::st_read(p, quiet = TRUE)                    -> sf,data.frame
+sf::st_read(p, quiet = TRUE, as_tibble = TRUE)  -> sf,tbl_df,tbl,data.frame
+dplyr::group_by(tbl, scale)                     -> sf,grouped_df,tbl_df,tbl,data.frame
+```
+
+Per the negative-fixture rule in `CLAUDE.md`, the helper asserts that premise
+inline, so a future `sf` change fails on the premise rather than on the
+behaviour under test.
+
+## Column vectors are already full length
+
+`basis`, `terrain`, `height_agl` and `dem_coverage` are each length `nrow()`
+(`rep()` / `ifelse()` over length-`n` inputs, `R/fly_footprint.R:353-382`), so
+assigning them with `$<-` introduces no recycling that `st_sf()` was not already
+doing.
+
+## Baseline
+
+`devtools::test()` on `8585fd5`: **FAIL 0 | WARN 0 | SKIP 0 | PASS 225**.
+
+CI on `main` at session start: all recent runs green (pkgdown + pages).
+
+## Errors Encountered
+
+| Error | Resolution |
+|-------|------------|
+
+## A/B against `8585fd5` on the edge cases the fix could plausibly move
+
+Prior implementation extracted with `git show 8585fd5:R/fly_footprint.R` and run
+side by side with the new one (not reconstructed from memory):
+
+| input | names | nrow | row.names | geometry | `sf_column` |
+|---|---|---|---|---|---|
+| full 20-row plain sf | identical | 20/20 | identical | identical | `geometry`, last |
+| zero-row | identical | 0/0 | identical | identical | `geometry`, last |
+| non-sequential subset `[c(5,3,11), ]` | identical | 3/3 | identical | identical | `geometry`, last |
+| single row | identical | 1/1 | identical | identical | `geometry`, last |
+
+So `$<-` recycling, the `row.names` argument the old `st_sf()` path used, and
+column ordering are all non-issues.
+
+**One deliberate behaviour change, and it is wider than first measured.** When
+the input already carries a column named `footprint_basis`, the old path kept the
+caller's value and discarded the computed one. Corrected measurement — the first
+probe counted exact name matches and so missed the duplicate on the plain path:
+
+| input | old | new |
+|---|---|---|
+| plain sf | 22 cols: `footprint_basis`, `footprint_basis.1`; `$footprint_basis` = `"PRE"` | 21 cols: `footprint_basis` = `"Film - BW"` |
+| tibble sf | 18 cols: `footprint_basis` = `"PRE"` (four columns dropped) | 21 cols: `footprint_basis` = `"Film - BW"` |
+
+So a **plain-sf** caller with a colliding column — a shape #35 never broke — sees
+a column-count and value change too. The new direction is the correct one:
+`fly_footprint()` must report how *it* sized each frame, not echo back a
+same-named column it was handed, and `fly_footprint(fly_footprint(x))` is now
+idempotent rather than sticky. But it is a second fix landing alongside the
+first, so it gets its own test and its own NEWS line rather than riding in
+silently.
+
+## Restore-the-bug verification
+
+Prior function loaded from `8585fd5` into both `asNamespace("fly")` and
+`as.environment("package:fly")` — patching only the namespace gives a false green
+for anything test code calls directly. Proof-of-patch printed before asserting:
+
+```
+PATCH PROOF - footprint_basis on tbl input: NULL (broken code running)
+```
+
+The sweep then failed on the `tbl` and `grouped` shapes, naming the four missing
+columns. The guard is real.
+
+## Suite and lint after the fix
+
+- `devtools::test()`: **FAIL 0 | WARN 0 | SKIP 0 | PASS 275** (baseline 225)
+- `lintr` on `R/fly_footprint.R`: 0 at `8585fd5`, 0 on branch. Package total 16,
+  all pre-existing (`data-raw/`, `fly_fetch`, `fly_georef`, `test-fly_fetch`,
+  `test-fly_footprint.R:391`, vignette)
+- `devtools::document()` wrote only `fly_footprint.Rd`; `NAMESPACE` unchanged at
+  9 exports, so no roxygen block rebound
+
+## End-to-end against the live BC Data Catalogue
+
+The case the fixture structurally cannot reach. `bcdc_query_geodata()` on
+`WHSE_IMAGERY_AND_BASE_MAPS.AIMG_PHOTO_CENTROIDS_SP`, bbox-filtered to the
+bundled AOI, then `collect()`:
+
+```
+rows: 1405
+input class : bcdc_sf,sf,tbl_df,tbl,data.frame
+output class: sf,bcdc_sf,tbl_df,tbl,data.frame
+four columns present: TRUE
+basis values: Film - BW | Film - Colour | unknown_format
+documented filter: 1254 of 1405
+```
+
+Three things this establishes that no fixture could:
+
+- the real input class is exactly the shape #35 named, and the four columns
+  arrive on it
+- the class **reordering** is real rather than an artefact of the hand-set
+  `bcdc_sf` in `centroid_shapes()` — `sf` leads on the way out, and it is
+  `st_transform()` that does it
+- the documented `footprint_basis != "unknown_format"` filter actually excludes
+  something on live data — 151 of 1405 frames — which is the digital-frame
+  exclusion #30 built and #35 had made unreachable from this source