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

Fliping orientation does not also flip the current crop #11614

Open
emko opened this issue Apr 23, 2022 · 11 comments · Fixed by #11643
Open

Fliping orientation does not also flip the current crop #11614

emko opened this issue Apr 23, 2022 · 11 comments · Fixed by #11643
Assignees
Labels
difficulty: hard big changes across different parts of the code base feature: redesign current features to rewrite priority: medium core features are degraded in a way that is still mostly usable, software stutters release notes: pending scope: UI user interface and interactions
Milestone

Comments

@emko
Copy link
Contributor

emko commented Apr 23, 2022

*Updated
issue with moving crop under orientation was fixed but main issue of crop not taking into account what happens with the orientation module changes is still a issue

To Reproduce
add a crop
add orientation and flip the image
move crop module below orientation

Orientation should default above crop because anytime you want to flip you then have to adjust the crop, but in this case its not above crop. When you move crop below orientation it causes many bugs, you can't crop your image again as the red controls are no longer there, image gets glitchy and sometimes dt will crash.

Screencast
(https://streamable.com/ei16tr)

  • darktable version : 3.8.1
  • OS : Fedora 35
  • Memory : 32gb
  • Graphics card : 970
  • Graphics driver : Nvidia
  • Are the steps above reproducible with a fresh edit (i.e. after discarding history)? yes
@elstoc
Copy link
Contributor

elstoc commented Apr 23, 2022

I think the issue here is a desire to use "flip" as a tool for testing composition while cropping so having flip affect the crop means it can't be used that way. Apart from feature duplication, are there any good reasons to avoid adding flip "back" into the crop module? (it used to be in crop & rotate)

Another question for @AlicVB

@elstoc elstoc added feature: redesign current features to rewrite scope: UI user interface and interactions labels Apr 23, 2022
@AlicVB
Copy link
Contributor

AlicVB commented Apr 24, 2022

good news : I can reproduce the issue :)

@elstoc : to answer your request, I would say that we should keep the crop module as simple as possible. even with simple code it's not so easy to avoid bugs and strange situations here, so better avoid to reproduce a new "clipping" nightmare !

@elstoc
Copy link
Contributor

elstoc commented Apr 24, 2022

Ok. Moving crop below orientation was an attempt to mitigate the original issue, and I'm not sure was advisable. So presumably the fix needs to allow flip to be used in its current pipe position without impacting the crop (so that crop continues to select the same area of the image regardless of the flip)

@AlicVB
Copy link
Contributor

AlicVB commented Apr 24, 2022

After some more tests and code review, that's quite tricky : in fact the crop zone is defined in % of the image at the crop module entry, so all distortion that happen before the crop module will change the crop area :( If we want to avoid that we have no other choices than to define the crop with the initial image as reference.
imho that would be better, but the question as usual is what will be broken by such a change...

oh and currently moving crop before other distort module produce lot of issues (like the one mentioned here).

So what I would propose :

  1. forbid crop to be moved before distort modules (@TurboGit : I've never done that. do you think it's easy to do ? for 4.0 ?)
  2. for 4.2 : evaluate a move to initial image reference

What do you think ?

@elstoc
Copy link
Contributor

elstoc commented Apr 24, 2022

forbid crop to be moved before distort modules

there is already some code to prevent movements the other way: you can't move orientation above crop but you can move crop below orientation, which seems inconsistent.

@jenshannoschwalm
Copy link
Collaborator

Don't we just require a rule for the crop and rotate&perspective modules added to

GList *dt_ioppr_get_iop_order_rules()

@TurboGit
Copy link
Member

  1. forbid crop to be moved before distort modules (@TurboGit : I've never done that. do you think it's easy to do ? for 4.0 ?)

I'll assign this to me and will try to do that for 4.0.

@TurboGit
Copy link
Member

Don't we just require a rule for the crop and rotate&perspective modules added to

Yes probably. Will do that.

@TurboGit TurboGit self-assigned this Apr 26, 2022
@TurboGit TurboGit added this to the 4.0 milestone Apr 26, 2022
@TurboGit TurboGit added the priority: medium core features are degraded in a way that is still mostly usable, software stutters label Apr 26, 2022
TurboGit added a commit to TurboGit/darktable that referenced this issue Apr 26, 2022
@TurboGit
Copy link
Member

See #11643.

@TurboGit
Copy link
Member

Reopened as a partial fix only.

@TurboGit TurboGit reopened this Apr 26, 2022
@TurboGit TurboGit modified the milestones: 4.0, 4.2 May 7, 2022
@dterrahe
Copy link
Member

Possibly this issue could be renamed to better reflect the missing functionality, because the action initially causing problems can no longer be performed?

@emko emko changed the title Moving crop below orientation causes issues Fliping orientation does not also flip the current crop Oct 29, 2022
@TurboGit TurboGit added difficulty: hard big changes across different parts of the code base release notes: pending labels Nov 14, 2022
@TurboGit TurboGit modified the milestones: 4.2, 4.4 Nov 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
difficulty: hard big changes across different parts of the code base feature: redesign current features to rewrite priority: medium core features are degraded in a way that is still mostly usable, software stutters release notes: pending scope: UI user interface and interactions
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants