ADFA-3290 | Implement smart boundary detection for dynamic margins#1170
Closed
ADFA-3290 | Implement smart boundary detection for dynamic margins#1170
Conversation
Replaces hardcoded bounds with a vertical projection edge-detection algorithm.
jatezzz
requested review from
a team,
Daniel-ADFA,
avestaadfa,
dara-abijo-adfa,
hal-eisen-adfa,
itsaky-adfa and
jomen-adfa
Contributor
📝 WalkthroughRelease Notes: Smart Boundary Detection for Dynamic MarginsFeatures
Technical Changes
|
| Cohort / File(s) | Summary |
|---|---|
ViewModel Integration ComputerVisionViewModel.kt |
Refactored image loading to use withContext(Dispatchers.Default) for bitmap processing, rotate the bitmap, invoke SmartBoundaryDetector.detectSmartBoundaries() to compute left/right boundaries, and dynamically initialize guide percentages from detected boundaries instead of fixed values (0.2f/0.8f). Early return on null bitmap with toast message. |
Boundary Detection Utilities BitmapUtils.kt, SmartBoundaryDetector.kt |
Added calculateVerticalProjection() utility function to analyze red-channel intensity transitions across bitmap columns. Introduced new SmartBoundaryDetector singleton with detectSmartBoundaries() that uses vertical projection analysis, gap detection, signal normalization, and fallback thresholding to robustly identify left and right content boundaries with configurable edge-ignore behavior. |
Sequence Diagram(s)
sequenceDiagram
actor User
participant ViewModel as ComputerVisionViewModel
participant Decoder as Bitmap Decoder
participant Rotator as Bitmap Rotation
participant BitmapUtils as BitmapUtils
participant Detector as SmartBoundaryDetector
User->>ViewModel: Select Image URI
ViewModel->>Decoder: uriToBitmap(uri)
Decoder-->>ViewModel: bitmap (or null)
alt Bitmap Decoded Successfully
ViewModel->>Rotator: Rotate bitmap to correct orientation
Rotator-->>ViewModel: rotatedBitmap
ViewModel->>BitmapUtils: calculateVerticalProjection(rotatedBitmap)
BitmapUtils-->>ViewModel: FloatArray projection
ViewModel->>Detector: detectSmartBoundaries(rotatedBitmap)
Detector->>Detector: Analyze projection, detect gaps
Detector->>Detector: Attempt primary detection
Detector->>Detector: Apply fallbacks if needed
Detector-->>ViewModel: Pair<leftBound, rightBound>
ViewModel->>ViewModel: Convert bounds to guide percentages
ViewModel->>ViewModel: Update guide state
else Bitmap Decoding Failed
ViewModel->>ViewModel: Show error toast
ViewModel->>ViewModel: Return early
end
Estimated code review effort
🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
- Implement ROI (Region of Interest) Filtering to Eliminate Margin Fals… #902: Modifies ComputerVisionViewModel's guide boundary initialization—this PR replaces fixed guide defaults (0.2/0.8) with SmartBoundaryDetector-based computation while the related PR focuses on ROI filtering and guide reset logic.
- fix(cv): correct image rotation in cv-image-to-xml - ADFA-2641 #881: Updates image rotation handling in ComputerVisionViewModel—both PRs work with bitmap rotation in the image-loading flow, though this one adds projection analysis and boundary detection on top.
- Feature/adfa 2665 cv zoom ability margin lines #887: Implements UI guideline components and guide percentage events—this PR adds automatic boundary detection to compute guide percentages while the related PR consumes those values in the UI layer.
Suggested reviewers
- Daniel-ADFA
- itsaky-adfa
Poem
🐰 A smart boundary detector hops through pixels bright,
Analyzing projections with algorithmic might,
From blurry edges it finds the path so true,
Dynamic guides now replace the static few!
The vision sharpens—no more guesswork here. ✨
🚥 Pre-merge checks | ✅ 2 | ❌ 1
❌ Failed checks (1 warning)
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage | Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. | Write docstrings for the functions missing them to satisfy the coverage threshold. |
✅ Passed checks (2 passed)
| Check name | Status | Explanation |
|---|---|---|
| Title check | ✅ Passed | The title directly aligns with the main change: replacing hardcoded margin boundaries with smart boundary detection for dynamic margins. |
| Description check | ✅ Passed | The description clearly relates to the changeset, explaining the addition of SmartBoundaryDetector, calculateVerticalProjection utility, and updates to ComputerVisionViewModel. |
✏️ Tip: You can configure your own custom pre-merge checks in the settings.
✨ Finishing Touches
📝 Generate docstrings
- Create stacked PR
- Commit on current branch
🧪 Generate unit tests (beta)
- Create PR with unit tests
- Commit unit tests in branch
feat/ADFA-3290-smart-boundary-detection
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
Comment @coderabbitai help to get the list of available commands and usage tips.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cv-image-to-xml/src/main/java/org/appdevforall/codeonthego/computervision/ui/viewmodel/ComputerVisionViewModel.kt (1)
143-146:⚠️ Potential issue | 🟠 MajorRethrow
CancellationExceptioninloadImageFromUri.The broad
catch (e: Exception)at lines 143–146 swallowsCancellationExceptionfromviewModelScope.launch, breaking structured cancellation and preventing proper coroutine cancellation propagation.🔧 Proposed fix
+import kotlinx.coroutines.CancellationException ... - } catch (e: Exception) { + } catch (e: CancellationException) { + throw e + } catch (e: Exception) { Log.e(TAG, "Error loading image from URI", e) _uiEffect.send(ComputerVisionEffect.ShowError("Failed to load image: ${e.message}")) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cv-image-to-xml/src/main/java/org/appdevforall/codeonthego/computervision/ui/viewmodel/ComputerVisionViewModel.kt` around lines 143 - 146, In loadImageFromUri (the coroutine launched via viewModelScope.launch) the catch (e: Exception) block swallows CancellationException; change the error handling so CancellationException is rethrown immediately (or check e is CancellationException and throw it) before handling other exceptions so coroutine cancellation propagates correctly; leave the existing Log.e(TAG, ...) and _uiEffect.send(ComputerVisionEffect.ShowError(...)) for non-cancellation errors.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@cv-image-to-xml/src/main/java/org/appdevforall/codeonthego/computervision/utils/SmartBoundaryDetector.kt`:
- Around line 17-44: The detectSmartBoundaries function can pass invalid
start/end indices into projection.copyOfRange when edgeIgnorePercent is negative
or too large; before slicing the projection (and before using
leftZoneEnd/rightZoneStart/rightZoneEnd) clamp and validate computed indices:
compute ignoredEdgePixels = clamp((width * edgeIgnorePercent).toInt(), 0,
width), ensure leftZoneEnd = min((width * LEFT_ZONE_END_PERCENT).toInt(), width)
and rightZoneStart = max((width * RIGHT_ZONE_START_PERCENT).toInt(), 0), and
ensure rightZoneEnd = max(width - ignoredEdgePixels, rightZoneStart); if any
computed slice would be empty or inverted, skip findBestGapMidpoint calls and
fall back to LEFT_FALLBACK_BOUND_PERCENT/RIGHT_FALLBACK_BOUND_PERCENT (or call
findBestGapMidpoint with an empty-safe path like normalizeSignal=true) so
copyOfRange is never called with invalid ranges in detectSmartBoundaries while
keeping references to calculateVerticalProjection and findBestGapMidpoint.
---
Outside diff comments:
In
`@cv-image-to-xml/src/main/java/org/appdevforall/codeonthego/computervision/ui/viewmodel/ComputerVisionViewModel.kt`:
- Around line 143-146: In loadImageFromUri (the coroutine launched via
viewModelScope.launch) the catch (e: Exception) block swallows
CancellationException; change the error handling so CancellationException is
rethrown immediately (or check e is CancellationException and throw it) before
handling other exceptions so coroutine cancellation propagates correctly; leave
the existing Log.e(TAG, ...) and
_uiEffect.send(ComputerVisionEffect.ShowError(...)) for non-cancellation errors.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 25d76446-2ce0-433c-b217-232f2dc3d6f7
📒 Files selected for processing (3)
cv-image-to-xml/src/main/java/org/appdevforall/codeonthego/computervision/ui/viewmodel/ComputerVisionViewModel.ktcv-image-to-xml/src/main/java/org/appdevforall/codeonthego/computervision/utils/BitmapUtils.ktcv-image-to-xml/src/main/java/org/appdevforall/codeonthego/computervision/utils/SmartBoundaryDetector.kt
Comment on lines
+17
to
+44
| fun detectSmartBoundaries( | ||
| bitmap: Bitmap, | ||
| edgeIgnorePercent: Float = DEFAULT_EDGE_IGNORE_PERCENT | ||
| ): Pair<Int, Int> { | ||
| val width = bitmap.width | ||
| val projection = calculateVerticalProjection(bitmap) | ||
| val minimumGapWidth = (width * MIN_GAP_WIDTH_PERCENT).toInt() | ||
|
|
||
| val ignoredEdgePixels = (width * edgeIgnorePercent).toInt() | ||
| val leftZoneEnd = (width * LEFT_ZONE_END_PERCENT).toInt() | ||
| val rightZoneStart = (width * RIGHT_ZONE_START_PERCENT).toInt() | ||
| val rightZoneEnd = width - ignoredEdgePixels | ||
|
|
||
| val leftSignal = projection.copyOfRange(ignoredEdgePixels, leftZoneEnd) | ||
| var (leftBound, leftGapLength) = findBestGapMidpoint(leftSignal, offset = ignoredEdgePixels) | ||
| if (leftBound == null || leftGapLength < minimumGapWidth) { | ||
| leftBound = findBestGapMidpoint(leftSignal, offset = ignoredEdgePixels, normalizeSignal = true).first | ||
| } | ||
|
|
||
| val rightSignal = projection.copyOfRange(rightZoneStart, rightZoneEnd) | ||
| var (rightBound, rightGapLength) = findBestGapMidpoint(rightSignal, offset = rightZoneStart) | ||
| if (rightBound == null || rightGapLength < minimumGapWidth) { | ||
| rightBound = findBestGapMidpoint(rightSignal, offset = rightZoneStart, normalizeSignal = true).first | ||
| } | ||
|
|
||
| val finalLeftBound = leftBound ?: (width * LEFT_FALLBACK_BOUND_PERCENT).toInt() | ||
| val finalRightBound = rightBound ?: (width * RIGHT_FALLBACK_BOUND_PERCENT).toInt() | ||
| return Pair(finalLeftBound, finalRightBound) |
Contributor
There was a problem hiding this comment.
Validate edgeIgnorePercent and range bounds before copyOfRange.
Current logic can throw IllegalArgumentException when callers pass invalid edgeIgnorePercent (e.g., negative or too large), because computed start/end indices may invert.
🛡️ Proposed fix
fun detectSmartBoundaries(
bitmap: Bitmap,
edgeIgnorePercent: Float = DEFAULT_EDGE_IGNORE_PERCENT
): Pair<Int, Int> {
val width = bitmap.width
+ if (width <= 0) {
+ return Pair(0, 0)
+ }
+
+ val safeEdgeIgnorePercent = edgeIgnorePercent.coerceIn(0f, 0.49f)
val projection = calculateVerticalProjection(bitmap)
val minimumGapWidth = (width * MIN_GAP_WIDTH_PERCENT).toInt()
- val ignoredEdgePixels = (width * edgeIgnorePercent).toInt()
- val leftZoneEnd = (width * LEFT_ZONE_END_PERCENT).toInt()
- val rightZoneStart = (width * RIGHT_ZONE_START_PERCENT).toInt()
- val rightZoneEnd = width - ignoredEdgePixels
+ val ignoredEdgePixels = (width * safeEdgeIgnorePercent).toInt()
+ val leftZoneEnd = (width * LEFT_ZONE_END_PERCENT).toInt().coerceAtLeast(ignoredEdgePixels)
+ val rightZoneStart = (width * RIGHT_ZONE_START_PERCENT).toInt().coerceIn(0, width)
+ val rightZoneEnd = (width - ignoredEdgePixels).coerceAtLeast(rightZoneStart)
val leftSignal = projection.copyOfRange(ignoredEdgePixels, leftZoneEnd)
var (leftBound, leftGapLength) = findBestGapMidpoint(leftSignal, offset = ignoredEdgePixels)
if (leftBound == null || leftGapLength < minimumGapWidth) {
leftBound = findBestGapMidpoint(leftSignal, offset = ignoredEdgePixels, normalizeSignal = true).first
}
val rightSignal = projection.copyOfRange(rightZoneStart, rightZoneEnd)
var (rightBound, rightGapLength) = findBestGapMidpoint(rightSignal, offset = rightZoneStart)
if (rightBound == null || rightGapLength < minimumGapWidth) {
rightBound = findBestGapMidpoint(rightSignal, offset = rightZoneStart, normalizeSignal = true).first
}
- val finalLeftBound = leftBound ?: (width * LEFT_FALLBACK_BOUND_PERCENT).toInt()
- val finalRightBound = rightBound ?: (width * RIGHT_FALLBACK_BOUND_PERCENT).toInt()
+ val fallbackLeft = (width * LEFT_FALLBACK_BOUND_PERCENT).toInt()
+ val fallbackRight = (width * RIGHT_FALLBACK_BOUND_PERCENT).toInt()
+ val finalLeftBound = (leftBound ?: fallbackLeft).coerceIn(0, width - 1)
+ val finalRightBound = (rightBound ?: fallbackRight).coerceIn(finalLeftBound, width - 1)
return Pair(finalLeftBound, finalRightBound)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| fun detectSmartBoundaries( | |
| bitmap: Bitmap, | |
| edgeIgnorePercent: Float = DEFAULT_EDGE_IGNORE_PERCENT | |
| ): Pair<Int, Int> { | |
| val width = bitmap.width | |
| val projection = calculateVerticalProjection(bitmap) | |
| val minimumGapWidth = (width * MIN_GAP_WIDTH_PERCENT).toInt() | |
| val ignoredEdgePixels = (width * edgeIgnorePercent).toInt() | |
| val leftZoneEnd = (width * LEFT_ZONE_END_PERCENT).toInt() | |
| val rightZoneStart = (width * RIGHT_ZONE_START_PERCENT).toInt() | |
| val rightZoneEnd = width - ignoredEdgePixels | |
| val leftSignal = projection.copyOfRange(ignoredEdgePixels, leftZoneEnd) | |
| var (leftBound, leftGapLength) = findBestGapMidpoint(leftSignal, offset = ignoredEdgePixels) | |
| if (leftBound == null || leftGapLength < minimumGapWidth) { | |
| leftBound = findBestGapMidpoint(leftSignal, offset = ignoredEdgePixels, normalizeSignal = true).first | |
| } | |
| val rightSignal = projection.copyOfRange(rightZoneStart, rightZoneEnd) | |
| var (rightBound, rightGapLength) = findBestGapMidpoint(rightSignal, offset = rightZoneStart) | |
| if (rightBound == null || rightGapLength < minimumGapWidth) { | |
| rightBound = findBestGapMidpoint(rightSignal, offset = rightZoneStart, normalizeSignal = true).first | |
| } | |
| val finalLeftBound = leftBound ?: (width * LEFT_FALLBACK_BOUND_PERCENT).toInt() | |
| val finalRightBound = rightBound ?: (width * RIGHT_FALLBACK_BOUND_PERCENT).toInt() | |
| return Pair(finalLeftBound, finalRightBound) | |
| fun detectSmartBoundaries( | |
| bitmap: Bitmap, | |
| edgeIgnorePercent: Float = DEFAULT_EDGE_IGNORE_PERCENT | |
| ): Pair<Int, Int> { | |
| val width = bitmap.width | |
| if (width <= 0) { | |
| return Pair(0, 0) | |
| } | |
| val safeEdgeIgnorePercent = edgeIgnorePercent.coerceIn(0f, 0.49f) | |
| val projection = calculateVerticalProjection(bitmap) | |
| val minimumGapWidth = (width * MIN_GAP_WIDTH_PERCENT).toInt() | |
| val ignoredEdgePixels = (width * safeEdgeIgnorePercent).toInt() | |
| val leftZoneEnd = (width * LEFT_ZONE_END_PERCENT).toInt().coerceAtLeast(ignoredEdgePixels) | |
| val rightZoneStart = (width * RIGHT_ZONE_START_PERCENT).toInt().coerceIn(0, width) | |
| val rightZoneEnd = (width - ignoredEdgePixels).coerceAtLeast(rightZoneStart) | |
| val leftSignal = projection.copyOfRange(ignoredEdgePixels, leftZoneEnd) | |
| var (leftBound, leftGapLength) = findBestGapMidpoint(leftSignal, offset = ignoredEdgePixels) | |
| if (leftBound == null || leftGapLength < minimumGapWidth) { | |
| leftBound = findBestGapMidpoint(leftSignal, offset = ignoredEdgePixels, normalizeSignal = true).first | |
| } | |
| val rightSignal = projection.copyOfRange(rightZoneStart, rightZoneEnd) | |
| var (rightBound, rightGapLength) = findBestGapMidpoint(rightSignal, offset = rightZoneStart) | |
| if (rightBound == null || rightGapLength < minimumGapWidth) { | |
| rightBound = findBestGapMidpoint(rightSignal, offset = rightZoneStart, normalizeSignal = true).first | |
| } | |
| val fallbackLeft = (width * LEFT_FALLBACK_BOUND_PERCENT).toInt() | |
| val fallbackRight = (width * RIGHT_FALLBACK_BOUND_PERCENT).toInt() | |
| val finalLeftBound = (leftBound ?: fallbackLeft).coerceIn(0, width - 1) | |
| val finalRightBound = (rightBound ?: fallbackRight).coerceIn(finalLeftBound, width - 1) | |
| return Pair(finalLeftBound, finalRightBound) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@cv-image-to-xml/src/main/java/org/appdevforall/codeonthego/computervision/utils/SmartBoundaryDetector.kt`
around lines 17 - 44, The detectSmartBoundaries function can pass invalid
start/end indices into projection.copyOfRange when edgeIgnorePercent is negative
or too large; before slicing the projection (and before using
leftZoneEnd/rightZoneStart/rightZoneEnd) clamp and validate computed indices:
compute ignoredEdgePixels = clamp((width * edgeIgnorePercent).toInt(), 0,
width), ensure leftZoneEnd = min((width * LEFT_ZONE_END_PERCENT).toInt(), width)
and rightZoneStart = max((width * RIGHT_ZONE_START_PERCENT).toInt(), 0), and
ensure rightZoneEnd = max(width - ignoredEdgePixels, rightZoneStart); if any
computed slice would be empty or inverted, skip findBestGapMidpoint calls and
fall back to LEFT_FALLBACK_BOUND_PERCENT/RIGHT_FALLBACK_BOUND_PERCENT (or call
findBestGapMidpoint with an empty-safe path like normalizeSignal=true) so
copyOfRange is never called with invalid ranges in detectSmartBoundaries while
keeping references to calculateVerticalProjection and findBestGapMidpoint.
Collaborator
Author
|
Closed due to error on branch name |
jatezzz
removed request for
a team,
Daniel-ADFA,
avestaadfa,
dara-abijo-adfa,
hal-eisen-adfa,
itsaky-adfa and
jomen-adfa
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
This PR replaces the hardcoded margin boundaries (0.2f and 0.8f) with a smart algorithm that calculates their optimal positions dynamically. It introduces the
SmartBoundaryDetectorand a newcalculateVerticalProjectionutility inBitmapUtilsto analyze image content and snap guidelines to the best available vertical gaps.Details
BitmapUtils.calculateVerticalProjection.SmartBoundaryDetectorto find gap midpoints using the projection data.ComputerVisionViewModel.loadImageFromUrito apply the dynamically calculated left and right percentage bounds instead of resetting to default hardcoded values.document_5098099422705747202.mp4
Ticket
ADFA-3290
Observation
The edge detection approach in
calculateVerticalProjectionuses a simplified, highly performant Sobel-like technique by directly calculating pixel variations on the red channel, avoiding heavy matrix operations on the main thread.