Skip to content

Added an basic implementation of floodfill#7

Open
dheer-prog wants to merge 3 commits into
ad-si:mainfrom
dheer-prog:ff
Open

Added an basic implementation of floodfill#7
dheer-prog wants to merge 3 commits into
ad-si:mainfrom
dheer-prog:ff

Conversation

@dheer-prog

Copy link
Copy Markdown

Hello @ad-si , This is an basic implementation of floodfill that I have created for this library I'll soon add this to cli.c I also added test's for this in test.c, I'll soon add support for mulitple seed points and will also add 8-way directionality

@dheer-prog

Copy link
Copy Markdown
Author

Hello @ad-si, have you had a chance to review the pr?

- Remove the target_value == new_value early return: it skipped filling
  neighbors that are within threshold but have a different grayscale value
- Rename floodfill.{c,h} to flood_fill.{c,h} to match fcv_flood_fill and
  the watershed_segmentation naming convention
- Add regression test for the new_value == seed case
- Make flood fill test failure messages use the ❌ prefix like the rest of
  the suite; drop the stray header comment and trailing whitespace
@ad-si

ad-si commented Jun 15, 2026

Copy link
Copy Markdown
Owner

Thanks for the PR @dheer-prog — solid first pass. The overflow-checked allocations, NULL guards, isfinite marker check, and bounds checking are all good practice, and the BFS core is correct. I pushed a commit (024e2b9) onto your ff branch with fixes for a few issues. Here's the full rundown so nothing is a surprise.

1. Bug: the target_value == new_value early return (the important one)

uint8_t target_value = grayscale_data[start_idx];
if (target_value == new_value) {
  destroy_floodfill_queue(queue);
  free(visited);
  return output_data;   // unmodified copy
}

The assumption here is "filling with the seed's own value is a no-op, so bail early." That's true for the classic exact-match flood fill, but not for this threshold-based one. The fill expands to every connected pixel within ±threshold of the seed value — and those pixels can have different grayscale values that still need to be overwritten with new_value.

Concrete failure case (2×2, seed at (0,0), threshold 3, new_value 10):

input:  10  8        expected: 10 10        old code:  10  8   ← idx1 left unchanged
       200 200                 200 200                 200 200

Pixel 8 is within threshold 3 of the seed 10, so it should become new_value (10). The early return skipped the entire fill and returned an untouched copy, leaving it at 8.

This guard exists in the recursive textbook flood fill specifically to prevent infinite recursion when there's no bookkeeping. Your implementation already has a visited array that prevents re-processing, so the guard isn't needed for termination — it was only causing the wrong output. I removed it.

One thing worth knowing: your existing no-op test (the all-7s grid with new_value == 7) passed because it happened to exercise this buggy early-return path, so the test suite didn't catch the bug. I added a regression test for the new_value == seed case (the 2×2 example above) that fails against the old code and passes now.

2. File naming: floodfill.{c,h}flood_fill.{c,h}

Small consistency thing. The function is fcv_flood_fill (two words, snake_case), and the closest existing module is watershed_segmentation.{c,h} / fcv_watershed_segmentation. The one-word floodfill filename was the odd one out, so I renamed the files to flood_fill.{c,h} and updated the #includes in the source and in tests/test.c. The makefile picks these up automatically via its src/*.c / include/*.h wildcards, so no makefile change was needed and the amalgamation (flatcv.c/flatcv.h) still builds.

3. tests/test.c cleanup

  • Removed the stray header block at the top of the file:
    //cc -Wall -Wextra -Wpedantic -Iinclude tests/test.c src/{feature_to_test}.c
    //How to use test.c
    It had an unfilled {feature_to_test} placeholder and trailing whitespace, and build instructions live in the makefile anyway.
  • Made the flood-fill test failure messages use the prefix, matching every other test in the file (most of yours were plain text, one had the emoji — now they're uniform).
  • Removed the trailing whitespace after uint8_t threshold, in the function signature.

What I deliberately left alone

  • The circular-buffer queue. Since visited is set before every enqueue, each pixel is enqueued at most once and the queue (capacity = pixel count) can never overflow — which makes the if (!enqueue_floodfill(...)) return NULL; branches technically unreachable. But they're correct, harmless defensive code, and rewriting the queue into a plain stack would add risk for zero behavior change. Not worth touching.
  • The makefile change (test: flatcv_mactest: $(FLATCV_NATIVE)). Unrelated to flood fill, but it's a reasonable portability fix (lets make test work on Linux too), so I kept it. Maybe mention it in the PR description so reviewers know it's intentional.

Verification

make test passes in full on my end: unit tests (including the new regression case), all 66 CLI integration testcases, the amalgamation build (which now compiles the renamed flood_fill.c), and the corner-detection suite.

Still open (your stated follow-ups)

No action needed now — just listing what's outstanding from your description:

  • Wiring fcv_flood_fill into cli.c
  • Multiple seed points
  • 8-way connectivity

Nice work overall — the bug was a subtle one. Let me know if you have questions about any of the changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants