Skip to content

Add field scanner fallback for coarse Giants boundaries#1292

Open
m0thm wants to merge 1 commit into
Courseplay:mainfrom
m0thm:fix/field-boundary-scanner-fallback
Open

Add field scanner fallback for coarse Giants boundaries#1292
m0thm wants to merge 1 commit into
Courseplay:mainfrom
m0thm:fix/field-boundary-scanner-fallback

Conversation

@m0thm

@m0thm m0thm commented Jul 9, 2026

Copy link
Copy Markdown

Fixes #1291.

On Kinlaig, Giants field boundary detection can return a very coarse boundary:

Field boundary detection successful after 42 updates, 26 boundary points and 0 islands

With this patch, Courseplay falls back to its own field scanner when the Giants result has too few points:

Giants boundary only has 25 points, using Courseplay field scanner result with 143 points instead

The fallback also tries the Courseplay scanner when Giants detection fails outright and no custom field is available, while keeping custom fields preferred when configured.

@pvaiko

pvaiko commented Jul 15, 2026

Copy link
Copy Markdown
Contributor

Using CP scanner if Giants fails is a good idea. Not sure however about using the number of vertices as a quality metrics. We started using the Giants one due to a lot of complaints about our scanner, especially around corners and very narrow / weird shapes, I'm almost sure our results had more vertices there, but weren't better.

@KeilerHirsch KeilerHirsch left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Traced how the result flows to callers, not just the two new methods. In the "Giants detection failed, no custom field" branch, self.fieldPolygon previously stayed nil forever (caller sees "no boundary found"); now _tryGiantsFailedBoundaryFallback() gets a chance to populate it before the function returns — that's the actual fix, not just cosmetic logging. The 2x-improvement threshold in _tryLowResolutionBoundaryFallback is a sensible guard against replacing a decent boundary with scanner noise. Both new methods nil-guard g_fieldScanner/g_fieldScanner.findContour correctly for setups where the scanner isn't available.

One thing worth a note, not blocking: this class's own docstring says detection is deliberately asynchronous ("update() must be called until it returns false") specifically so it doesn't block a frame — but FieldScanner:findContour (FieldScanner.lua:157) is a synchronous loop, not chunked like FieldCourseField:update() is. Calling it from inside the completion callback means a one-off synchronous scan on the (hopefully rare) fallback path. The loop looks bounded (i < 11) so probably not a real hitch in practice, but flagging it since it's a bit of an architectural mismatch with the class's own async design intent.

luac -p passes. LGTM.

@KeilerHirsch KeilerHirsch left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Follow-up after seeing @pvaiko's comment, which I hadn't read before approving — should have checked the discussion first, not just the diff.

My approval was based on the code being internally safe (nil-guarded, bounded fallback that only fires when the primary detection is coarse/missing), not on the vertex-count metric being a proven-good quality signal — that's a domain judgment call I'm not positioned to make from the code alone. @pvaiko's point is a real one: if the project already tried and moved away from the CP scanner specifically because "more vertices" didn't correlate with "better" on corners/narrow shapes, then #scannerPolygon > #self.fieldPolygon * 2 is reusing exactly the metric that was already found unreliable, just as a fallback trigger instead of a primary source this time.

Worth resolving before merge — maybe a metric that looks at something more geometrically meaningful (e.g. corner angles, self-intersection, or bounding-box coverage) rather than raw point count, or at minimum some field-testing on the corner/narrow-shape cases that motivated the original switch to Giants. Not walking back the rest of the review (the fallback wiring itself, nil-guards, and the failed-detection path are still solid), just flagging that I shouldn't have approved without weighing this first.

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.

Field boundary doing very odd things!

3 participants