Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Crop improvements #145

Merged
merged 6 commits into from
Dec 30, 2024
Merged

Crop improvements #145

merged 6 commits into from
Dec 30, 2024

Conversation

pol-rivero
Copy link
Contributor

Improvements:

  • NEW: Dragging outside the area will create a new crop (previously did nothing)
    • Useful for starting again after a misclick
1.mp4
  • Dragging outside the area (but close to the region) will search the closest handle and act as if that handle had been clicked
    • Prevents accidentally losing the current selection (accidentally starting a new crop)
2.mp4
  • Dragging inside the area moves it (unchanged behavior)

  • NEW: Dragging inside the area while very close to a handle will act as if that handle had been clicked

    • Prevents losing the area placement when misclicking a handle
3.mp4
  • By clicking the area and moving it, it is possible to have a part of the area outside the bounds of the image. This previously resulted in a black background in the out-of-bounds area, while now it is excluded.
4.mp4
Old New
n6 n5
  • Due to the use of floats, using the crop tool could result in the image looking blurry. The crop is now rounded to the closest integer to avoid that issue.
Old New
n8 n7
image image
  • The PR also has some minor code cleanup in crop.rs, mostly to move some methods that made more sense in Crop than CropTool and to make Crop.size not an Option (didn't seem necessary and also caused some minor flickering).

When moving the crop area outside the bounds, output only the in-bounds part of the image instead of a black background
@gabm
Copy link
Owner

gabm commented Dec 29, 2024

Great improvements and fixes! 👍

I don't remember, why size was an option back then, but now it's not anymore.. any idea why this is okay that way?

@pol-rivero
Copy link
Contributor Author

I also don't understand why it was an option. The only behavior change that I could find is that, in the first frame after clicking (before the first drag update), the crop area would be None and therefore be treated as the entire screen, while now that area has a size of 0.
However, this doesn't matter in practice, since it's practically impossible to save the screenshot in such a small period of time, so avoiding the flickering and having simpler code is worth it in my opinion.

@gabm
Copy link
Owner

gabm commented Dec 30, 2024

aaah yes, that was a real problem. it was provoked by starting a crop but not moving the mouse. Easiest achieved with a trackball, clicking on one place without moving.

We should account for that. We can do that by checking against an epsilon, like the handle size - or with an option or similar.

@pol-rivero
Copy link
Contributor Author

pol-rivero commented Dec 30, 2024

Aah, ok, I've added it back (checked against epsilon).

@gabm gabm merged commit 478113f into gabm:main Dec 30, 2024
3 checks passed
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