feat: add CropOnLMarkers processor for L-shaped corner markers#294
Open
Praddy2006 wants to merge 1 commit into
Open
feat: add CropOnLMarkers processor for L-shaped corner markers#294Praddy2006 wants to merge 1 commit into
Praddy2006 wants to merge 1 commit into
Conversation
PR Summary by QodoAdd CropOnLMarkers preprocessor for L-corner marker based sheet alignment Description
Diagram
High-Level Assessment
Files changed (4)
|
Code Review by Qodo
Context used 1. No L-marker anchor regions
|
![qodo-free-for-open-source-projects[bot]](https://avatars.githubusercontent.com/in/1068000?s=60&v=4){kind=small}
Comment on lines
70
to
76
| "name": { | ||
| "type": "string", | ||
| "enum": [ | ||
| "CropOnLMarkers", | ||
| "CropOnMarkers", | ||
| "CropPage", | ||
| "FeatureBasedAlignment", |
There was a problem hiding this comment.
1. No l-marker anchor regions 📎 Requirement gap ≡ Correctness
The template schema and sample configuration add CropOnLMarkers but provide no way to define anchor-point regions/ROIs for L-marker detection. This prevents templates from explicitly expressing where L-markers should be found and increases the risk of unreliable detection across varied scans.
Agent Prompt
## Issue description
`CropOnLMarkers` is introduced, but the template schema does not support defining anchor-point regions (ROIs) for L-shaped corner markers, and the processor options currently expose only global detection knobs (e.g., `min_contour_area`). This violates the requirement to support anchor point definitions in the template without breaking existing templates.
## Issue Context
The schema currently enumerates `CropOnLMarkers` but does not define an `if name == CropOnLMarkers` options schema, nor any structure for anchor/ROI definitions. The sample template also lacks anchor definitions, and the processor does not read them.
## Fix Focus Areas
- src/schemas/template_schema.py[64-183]
- src/processors/CropOnLMarkers.py[13-35]
- samples/sample_lmarkers/template.json[179-198]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+44
to
+58
| # Step 1: Blur + threshold to get clean black/white image | ||
| gray = cv2.GaussianBlur(image, (5, 5), 0) | ||
| _, binary = cv2.threshold(gray, 0, 255, cv2.THRESH_BINARY_INV + cv2.THRESH_OTSU) | ||
|
|
||
| # Step 2: Morphological close - connects broken edges of L-shapes | ||
| kernel = np.ones((3, 3), np.uint8) | ||
| binary = cv2.morphologyEx(binary, cv2.MORPH_CLOSE, kernel, iterations=2) | ||
|
|
||
| if config.outputs.show_image_level >= 2: | ||
| InteractionUtils.show("CropOnLMarkers: binary", binary, config=config) | ||
|
|
||
| # Step 3: Find all contours | ||
| contours, _ = cv2.findContours( | ||
| binary, cv2.RETR_EXTERNAL, cv2.CHAIN_APPROX_SIMPLE | ||
| ) |
There was a problem hiding this comment.
2. No canny edge detection 📎 Requirement gap ☼ Reliability
CropOnLMarkers.apply_filter() performs thresholding and then directly extracts contours, but never runs cv2.Canny() edge detection as required. This can reduce detection robustness for L-markers under varying lighting and print artifacts.
Agent Prompt
## Issue description
The L-marker detection pipeline does not use Canny edge detection before contour extraction, despite the compliance requirement calling for a Canny + contour approach to locate L-shaped markers and their corner/turning points.
## Issue Context
Current flow: blur -> Otsu threshold -> morphology close -> `findContours(binary, ...)`. There is no `cv2.Canny(...)` stage.
## Fix Focus Areas
- src/processors/CropOnLMarkers.py[44-58]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+67
to
+116
| if len(candidates) < 4: | ||
| logger.warning( | ||
| f"CropOnLMarkers: Found only {len(candidates)} candidates," # noqa: E231 | ||
| " need 4. Check min/max contour area settings." | ||
| ) | ||
| if config.outputs.show_image_level >= 1: | ||
| InteractionUtils.show( | ||
| "CropOnLMarkers: not enough markers", | ||
| image, | ||
| config=config, | ||
| ) | ||
| return None | ||
|
|
||
| # Step 5: Find the inner corner (turning point) of each L candidate | ||
| # We get the bounding box and pick the corner closest to image center | ||
| h, w = image.shape[:2] | ||
| cx, cy = w / 2, h / 2 # image center | ||
|
|
||
| inner_corners = [] | ||
| for cnt in candidates: | ||
| x, y, bw, bh = cv2.boundingRect(cnt) | ||
|
|
||
| # The 4 possible corners of the bounding box | ||
| box_corners = [ | ||
| [x, y], # top-left | ||
| [x + bw, y], # top-right | ||
| [x, y + bh], # bottom-left | ||
| [x + bw, y + bh], # bottom-right | ||
| ] | ||
|
|
||
| # Inner corner = corner of bounding box closest to image center | ||
| inner = min( | ||
| box_corners, | ||
| key=lambda p: (p[0] - cx) ** 2 + (p[1] - cy) ** 2, | ||
| ) | ||
| inner_corners.append(inner) | ||
|
|
||
| # Step 6: Pick one marker per quadrant (TL, TR, BL, BR) | ||
| quadrant_corners = self._pick_one_per_quadrant(inner_corners, cx, cy) | ||
|
|
||
| if quadrant_corners is None or len(quadrant_corners) != 4: | ||
| logger.warning("CropOnLMarkers: Could not find one marker per quadrant.") | ||
| return None | ||
|
|
||
| logger.info(f"CropOnLMarkers: Detected inner corners: {quadrant_corners}") | ||
|
|
||
| # Step 7: Crop and warp using the 4 inner corners | ||
| centres = np.array(quadrant_corners, dtype="float32") | ||
| image = ImageUtils.four_point_transform(image, centres) | ||
|
|
There was a problem hiding this comment.
3. Hard-coded 4 marker points 📎 Requirement gap ☼ Reliability
The processor requires at least 4 candidates and then selects exactly one per quadrant, enforcing a 4-point warp instead of aggregating all detected points and computing an outer bounding rectangle. This violates the requirement to avoid a fixed 4-point assumption and can fail when detections are more/less than four.
Agent Prompt
## Issue description
The algorithm is constrained to exactly 4 points (one per quadrant) and warps using those points, instead of aggregating all detected L-marker points and computing an outer bounding rectangle/bounding box to drive alignment/cropping.
## Issue Context
Current behavior:
- returns `None` when `len(candidates) < 4`
- computes `inner_corners` per candidate
- forces one-per-quadrant selection and requires `len(...) == 4`
- warps via `ImageUtils.four_point_transform(image, centres)`
## Fix Focus Areas
- src/processors/CropOnLMarkers.py[67-116]
- src/processors/CropOnLMarkers.py[119-152]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+55
to
+58
| # Step 3: Find all contours | ||
| contours, _ = cv2.findContours( | ||
| binary, cv2.RETR_EXTERNAL, cv2.CHAIN_APPROX_SIMPLE | ||
| ) |
There was a problem hiding this comment.
4. Findcontours unpacking crash 🐞 Bug ☼ Reliability
CropOnLMarkers unpacks cv2.findContours(...) into two values, which raises a runtime unpacking error on OpenCV versions that return a 3-tuple (e.g., OpenCV 3). This can crash preprocessing before any cropping is attempted.
Agent Prompt
## Issue description
`CropOnLMarkers.apply_filter()` assumes `cv2.findContours()` returns exactly `(contours, hierarchy)`, but the repo already accounts for OpenCV’s signature differences elsewhere. On OpenCV builds returning 3 values, preprocessing will fail immediately.
## Issue Context
The codebase includes `ImageUtils.grab_contours()` specifically to normalize the OpenCV v2/v3/v4 `findContours` return shape, and other processors (e.g., `CropPage`) use it.
## Fix Focus Areas
- src/processors/CropOnLMarkers.py[55-58]
## Implementation notes
- Replace the 2-value unpacking with:
- `contours = ImageUtils.grab_contours(cv2.findContours(binary, cv2.RETR_EXTERNAL, cv2.CHAIN_APPROX_SIMPLE))`
- Keep the rest of the loop unchanged.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+85
to
+103
| inner_corners = [] | ||
| for cnt in candidates: | ||
| x, y, bw, bh = cv2.boundingRect(cnt) | ||
|
|
||
| # The 4 possible corners of the bounding box | ||
| box_corners = [ | ||
| [x, y], # top-left | ||
| [x + bw, y], # top-right | ||
| [x, y + bh], # bottom-left | ||
| [x + bw, y + bh], # bottom-right | ||
| ] | ||
|
|
||
| # Inner corner = corner of bounding box closest to image center | ||
| inner = min( | ||
| box_corners, | ||
| key=lambda p: (p[0] - cx) ** 2 + (p[1] - cy) ** 2, | ||
| ) | ||
| inner_corners.append(inner) | ||
|
|
There was a problem hiding this comment.
5. Warp anchored to bbox 🐞 Bug ≡ Correctness
CropOnLMarkers derives the 'inner corner' from cv2.boundingRect corners, but bounding-box corners (notably [x + bw, y + bh]) are not guaranteed to be on the L-marker contour, so the warp can be anchored to points in whitespace and mis-crop/mis-align the sheet. This is especially risky because those derived points are used directly as inputs to four_point_transform.
Agent Prompt
## Issue description
The processor currently picks the corner closest to the image center from the *bounding rectangle’s* four corners. For an L-shaped contour, a bounding-rect corner can be outside the drawn marker (inside the empty area of the bounding box), which makes the perspective transform anchor inaccurate.
## Issue Context
The output of this step is fed into `ImageUtils.four_point_transform()`, so corner inaccuracies directly affect cropping/alignment.
## Fix Focus Areas
- src/processors/CropOnLMarkers.py[85-116]
## Implementation notes
Implement corner extraction using contour geometry rather than bbox corners. Options (any one is acceptable if robust):
- Use the actual contour points: `pts = cnt.reshape(-1,2)` and choose an anchor that is guaranteed to be on the contour (e.g., the contour point minimizing distance to `(cx, cy)` as the 'inner' point), then optionally refine.
- Or approximate polygon (`cv2.approxPolyDP`) / fit two dominant perpendicular lines and compute their intersection.
- Ensure the chosen anchor point lies on/near the contour (or within a small distance threshold) before accepting it as a marker corner.
- Keep quadrant selection, but operate on these contour-derived anchor points.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Owner
|
@Praddy2006 I'll be able to take a look at this PR in 2-3 weeks. Until then can you address the AI comments? Update some tests and add a working screenshot/recording? |
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
2 participants
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.
Summary
Adds a new
CropOnLMarkerspre-processor that detects L-shaped cornermarkers and uses them to crop and align OMR sheets.
Approach
ImageUtils.four_point_transform—reusing existing crop utilities
Changes
src/processors/CropOnLMarkers.py— new processorsrc/schemas/template_schema.py— added CropOnLMarkers to validatorsamples/sample_lmarkers/— sample template for testingUsage in template.json
{
"name": "CropOnLMarkers",
"options": {
"marker_size": 40,
"thickness": 10,
"min_contour_area": 200,
"max_contour_area": 6000
}
}
Closes #270