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

vectorscope: fix visibility of primary sample #9906

Conversation

Mark-64
Copy link
Contributor

@Mark-64 Mark-64 commented Aug 30, 2021

With merge of #9866, in one of the last commits (fb08c34) I realized I broke the connection between the gui "restrict scope to selection" and the visualization of the primary sample dot in the vectorscope.
The result is that the primary sample is always visualized.
This is because d->vectorscope_pt[0] was used not only to store the x coordinate of the primary sample, but also to trigger the visualization on/off.
When I changed the calculation of primary sample coordinates, starting from the color conversion, this mechanism was overriden.
With this PR I replace the old mechanism with usage of darktable.lib->proxy.colorpicker.restrict_histogram and darktable.lib->proxy.colorpicker.primary_sample->size for triggering the visualization, which is more clear.

@Mark-64 Mark-64 marked this pull request as draft August 30, 2021 21:01
@Nilvus Nilvus added the bugfix pull request fixing a bug label Aug 30, 2021
@Nilvus Nilvus added this to the 3.8 milestone Aug 30, 2021
@Nilvus Nilvus added the scope: UI user interface and interactions label Aug 30, 2021
@Mark-64 Mark-64 force-pushed the fix_primary_sample_visibility_in_vectorscope branch from da73f32 to 54ee79e Compare August 30, 2021 22:03
@Mark-64 Mark-64 marked this pull request as ready for review August 30, 2021 22:05
@TurboGit
Copy link
Member

With merge of #9866, in one of the last commits (fb08c34) I realized I broke the connection between the gui "restrict scope to selection" and the visualization of the primary sample dot in the vectorscope.

On my side I thought it was a feature. Indeed the samples are the ones that have been explicitly added. Having the current color picker point being displayed seemed ok to me. No ?

@TurboGit TurboGit self-requested a review August 31, 2021 09:23
@Mark-64
Copy link
Contributor Author

Mark-64 commented Aug 31, 2021

On my side I thought it was a feature. Indeed the samples are the ones that have been explicitly added. Having the current color picker point being displayed seemed ok to me. No ?

Well, that was not the intention of the original designer. In #9835, @dtorop set the primary sample visualized in vectorscope only when "restrict scope to selection" is enabled.
We can always rediscuss that, having it visualized at all times, or (as I would prefer) connect it to "display samples on image / vectorscope", as long as it is a conscious decision and not a bug...
So I suggest to fix the bug first and then we can always change the behavior.
Also consider that visualization of the primary sample is a bit more computationally expensive as it requires dt_dev_invalidate() to reprocess the vectorscope.

@TurboGit
Copy link
Member

@Mark-64 : Fine by me but now even with the option selected to display the area on the scope there is nothing displayed.

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.

Something broken the other way around now.

@Mark-64
Copy link
Contributor Author

Mark-64 commented Aug 31, 2021

Strange, it works perfectly for me in both Windows and Ubuntu.
Are you sure you checked "restrict scope to selection" ?
I known it could sound counterintuitive, but "display samples on image/vectorscope" is only to show the live samples, not the primary one.

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.

Ok, quite non-intuitive indeed ! Let's go with that now, but we will probably want to rework this part!

@TurboGit TurboGit merged commit 95da62b into darktable-org:master Aug 31, 2021
@Mark-64
Copy link
Contributor Author

Mark-64 commented Aug 31, 2021

Ok, thanks.
I can open an issue to rework this, so we can discuss

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 scope: UI user interface and interactions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants