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

Some late crop fixes #16976

Merged
merged 1 commit into from
Jun 15, 2024
Merged

Conversation

jenshannoschwalm
Copy link
Collaborator

  1. avoid a possible div-by-zero in _aspect_apply()
  2. restrict the width & height of crop taking care of x and y offsets
  3. the gui_expose function should apply aspect both for vertical and horizontal

@ralfbrown @MStraeten @TurboGit could you test this in depth? (landscape, portrait, ...) I think this a fix still for 4.8

See #16831 , you might see in ashift that correction via key shortcuts does sometimes produce results not-inside valid data if original ratio is used

1. avoid a possible div-by-zero in _aspect_apply()
2. restrict the width & height of crop taking care of x and y offsets
3. the gui_expose function should apply aspect both for vertical and horizontal
@jenshannoschwalm jenshannoschwalm added bugfix pull request fixing a bug priority: medium core features are degraded in a way that is still mostly usable, software stutters labels Jun 11, 2024
@jenshannoschwalm jenshannoschwalm added this to the 4.8 milestone Jun 11, 2024
@MStraeten
Copy link
Collaborator

while applying the rotation the crop frame changes the ratio temporarily but the result is ok. If that is the issue then at least the fix seems to behave like my current master build
my steps: shortcut to activate crop (21:9 ration)
shortcuts to rotate while crop module is active

16976.mp4

@TurboGit
Copy link
Member

@MStraeten : What you should in your video seems ok to me. When you change rotation the module does disable the crop temporarily (that's expected), so you see the full image with the crop guide changing (full size to crop size).

@TurboGit
Copy link
Member

At least this seems to fix the issue reported by @ralfbrown on #16831. I'll test later....

@ralfbrown
Copy link
Collaborator

I still see the occasional blip to full image for a 4:3 crop box on a 3:2 image, but I never saw any blips to other ratios as previously. I suspect that the underlying problem in this case is that all four margins are stored as 0% which then confuses things in a race condition somewhere. Since it's self-correcting after a fraction of a second, it's low priority.

I did also see the case reported in a different issue where a small triangle of black appears because R&P sets its "original format" crop beyond the rotated image. That may be a race condition of sorts too, as I also ran the keyboard-shortcut rotation at high speed with just R&P, and when it occurred I was able to fix it by switching the auto-crop to "largest area" and back to "original format"

@TurboGit
Copy link
Member

@jenshannoschwalm @ralfbrown : Ok to merge in 4.8? The code seems safe and fix ensure that values are in proper range so cannot hurt anyway.

@jenshannoschwalm
Copy link
Collaborator Author

Yes :-)

Copy link
Member

@TurboGit TurboGit left a comment

Choose a reason for hiding this comment

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

Thanks!

@TurboGit TurboGit merged commit 41b9ac8 into darktable-org:master Jun 15, 2024
6 checks passed
@jenshannoschwalm jenshannoschwalm deleted the crop_orientation branch June 16, 2024 07:36
@AxelG-DE AxelG-DE mentioned this pull request Jun 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugfix pull request fixing a bug priority: medium core features are degraded in a way that is still mostly usable, software stutters
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants