Skip to content

feat: allow boundary strategy to be passed when updating rois programmatically#177

Merged
stropitek merged 11 commits intomainfrom
175-allow-boundary-strategy-to-be-passed-when-updating-rois-programmatically
Nov 20, 2025
Merged

feat: allow boundary strategy to be passed when updating rois programmatically#177
stropitek merged 11 commits intomainfrom
175-allow-boundary-strategy-to-be-passed-when-updating-rois-programmatically

Conversation

@stropitek
Copy link
Contributor

@stropitek stropitek commented Nov 19, 2025

Closes: #175

  • feat: add option when commiting ROI through updateRoi actions API to choose the boundary strategy
  • docs: re-organize stories
  • fix: correct implementation of rectangle intersection detection
  • refactor: return tuple from getRectanglePoints

@cloudflare-workers-and-pages
Copy link

cloudflare-workers-and-pages bot commented Nov 19, 2025

Deploying react-roi with  Cloudflare Pages  Cloudflare Pages

Latest commit: 296cd87
Status: ✅  Deploy successful!
Preview URL: https://88f74ccb.react-roi.pages.dev
Branch Preview URL: https://175-allow-boundary-strategy.react-roi.pages.dev

View logs

@stropitek
Copy link
Contributor Author

I re-organized the stories for better order (most important / relevant at the beginning).

The fixed implementation of partially_inside boundary strategy can be tested here: https://175-allow-boundary-strategy.react-roi.pages.dev/?path=/story/actions--update-position-with-commit-strategy

@stropitek stropitek marked this pull request as ready for review November 19, 2025 14:35
@stropitek stropitek requested a review from tpoisseau November 19, 2025 14:35
@stropitek
Copy link
Contributor Author

stropitek commented Nov 19, 2025

As stated a comment, inside_auto is not possible with updateRoi because it applies to different categories of operations (move, resize), but updateRoi is generic.

@tpoisseau
Copy link
Contributor

About refactor: return tuple from getRectanglePoints I think we need to agree on names rather than not using any. The point of the issue was really to name the points, not specifically to return a tuple rather than an array.

Copy link
Contributor

@tpoisseau tpoisseau left a comment

Choose a reason for hiding this comment

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

CleanShot 2025-11-20 at 09 18 28@2x

Should not the ROI snap to the commited ROI when it is moved outside ?

Copy link
Contributor

@tpoisseau tpoisseau left a comment

Choose a reason for hiding this comment

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

Otherwise, LGTM

@stropitek
Copy link
Contributor Author

stropitek commented Nov 20, 2025

Should not the ROI snap to the commited ROI when it is moved outside ?

The editable (black / transparent) ROI has no restriction on where it can be.
On commit, the orange non-editable one is updated with the position of the editable one, but with the selected boundary constraint.

It allows you to compare position sent to updateRoi with the actual position after the constraint is applied.

Can you let me know again if it LGTY?

About refactor: return tuple from getRectanglePoints I think we need to agree on names rather than not using any. The point of the issue was really to name the points, not specifically to return a tuple rather than an array.

The explanation of what each point is is kind of complex, and I don't know how to summarize it in a name without being misleading. I'm removing #176 from the scope of this PR.

@tpoisseau
Copy link
Contributor

Can you let me know again if it LGTY?

OK for me, I checked the behavior on this page instead: https://175-allow-boundary-strategy.react-roi.pages.dev/?path=%2Fstory%2Fpreferences--commit-boundary-strategy&args=allowRotate%3A!true

I suppose https://175-allow-boundary-strategy.react-roi.pages.dev/?path=/story/actions--update-position-with-commit-strategy is great for debugging, but I found it confusing.

@stropitek stropitek merged commit 863c925 into main Nov 20, 2025
9 checks passed
@stropitek stropitek deleted the 175-allow-boundary-strategy-to-be-passed-when-updating-rois-programmatically branch November 20, 2025 17:29
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.

Allow boundary strategy to be passed when updating ROIs programmatically

2 participants