resolve #21014 by limiting number of ticks by mjmdavis · Pull Request #21147 · opencv/opencv

mjmdavis · 2021-11-29T04:18:48Z

resolves #21014

Pull Request Readiness Checklist

See details at https://github.com/opencv/opencv/wiki/How_to_contribute#making-a-good-pull-request

I agree to contribute to the project under Apache 2 License.
To the best of my knowledge, the proposed patch is not based on a code under GPL or other license that is incompatible with OpenCV
The PR is proposed to proper branch
There is reference to original bug report and related work
There is accuracy test, performance test and test data in opencv_extra repository, if applicable
Patch to opencv_extra has the same branch name.
The feature is well documented and sample code can be built with the project CMake

force_builders=Mac,iOS

mjmdavis · 2021-11-29T04:19:51Z

modules/highgui/src/window_cocoa.mm

    [[slider slider] setMinValue:0];
-    [[slider slider] setNumberOfTickMarks:(max+1)];
-    [[slider slider] setAllowsTickMarkValuesOnly:YES];
+    [[slider slider] setNumberOfTickMarks:(max > 100 ? 101 : max+1)];


Set a limit on the number of TickMarks shown.

mjmdavis · 2021-11-29T04:21:11Z

modules/highgui/src/window_cocoa.mm

-    [[slider slider] setNumberOfTickMarks:(max+1)];
-    [[slider slider] setAllowsTickMarkValuesOnly:YES];
+    [[slider slider] setNumberOfTickMarks:(max > 100 ? 101 : max+1)];
+    [[slider slider] setAllowsTickMarkValuesOnly:NO];


Allow selection of values without TickMarks. This is important for situations where there is a large range of values.

mjmdavis · 2021-11-29T04:24:43Z

While it might be nicer to subclass NSSlider and override setNumberOfTickMarks, I'm not sure it's worth the extra complexity in this case. I could add a comment explaining why the limit is there.

alalek

Need to fix cvSetTrackbarMax() too.

(use max - min instead of max there)

alalek · 2021-11-29T12:33:56Z

modules/highgui/src/window_cocoa.mm

    [[slider slider] setMinValue:0];
-    [[slider slider] setNumberOfTickMarks:(max+1)];
-    [[slider slider] setAllowsTickMarkValuesOnly:YES];
+    [[slider slider] setNumberOfTickMarks:(max > 100 ? 101 : max+1)];
+    [[slider slider] setAllowsTickMarkValuesOnly:NO];
    if(value)


Can we just hide all ticks? (use 0 instead of 101)

[[slider slider] setNumberOfTickMarks:(max > 100 ? 0 : max+1)];

Yes! I think this is a better solution. You'd mentioned limiting the number of ticks as a preferred solution previously.

No ticks is simpler and solves the problem in a way that's less likely be reverted.

Actually I mean hiding for case with more than 100 values:

-[[slider slider] setNumberOfTickMarks:(max > 100 ? 101 : max+1)]; +[[slider slider] setNumberOfTickMarks:(max > 100 ? 0 : max+1)];

Or

if (max <= 100) { [[slider slider] setNumberOfTickMarks:(max+1)]; }

Please take a look on cvSetTrackbarMax() too. We have setNumberOfTickMarks call there too.

[[slider slider] setNumberOfTickMarks:((max - min) > 100 ? 0 : (max - min +1))];

Note: we need to call setNumberOfTickMarks:0 here if we want to hide ticks.

It makes sense to define constant in this file somewhere to avoid using of "magic" number 100 in multiple places (OPENCV_NSSLIDER_MAX_TICKS=100)

When I search the code, there are no more mentions of setNumberOfTickMarks. Are you looking at a branch other than 4.x?

I will prepare another PR that does this. The behaviour will be a little inconsistent but ok I guess.

mjmdavis · 2021-11-29T15:29:13Z

modules/highgui/src/window_cocoa.mm

    [[slider slider] setMaxValue:max];
    [[slider slider] setMinValue:0];
-    [[slider slider] setNumberOfTickMarks:(max+1)];
-    [[slider slider] setAllowsTickMarkValuesOnly:YES];


Now no TickMarks will be shown.

mjmdavis · 2021-11-29T15:29:32Z

I believe this will work but I've somehow just run into "ERROR: recursion is detected during loading of "cv2" binary extensions. Check OpenCV installation.". I might need to test it later.

mjmdavis · 2021-11-29T15:45:31Z

modules/highgui/src/window_cocoa.mm

@@ -944,8 +944,6 @@ - (void)createSliderWithName:(const char *)name maxValue:(int)max value:(int *)v
    [[slider name] setStringValue:cvname];
    [[slider slider] setMaxValue:max];
    [[slider slider] setMinValue:0];


Should this be min?

I guess this should be a different issue.

In general yes, it should be min.
In "createSliderWithName", we always have min = 0

Ah, I see that now.

mjmdavis · 2021-11-29T15:46:35Z

It works but now the area of the track bar to the left of the slider is blue and the dragger is round. :)

Changing this might require some custom assets or the subclassing of NSSlider.

I'd say accept this for now as it makes the system functional and then create a new ticket to look at the rest.

mjmdavis · 2021-11-30T15:31:28Z

Thinking more about it, what should the behaviour be if someone:

Creates a trackbar with max 80.
Updates the trackbar to have max 120.

Would then expect the trackbar to change in appearance? I believe it's better to offer a consistent UI in this situation. What do you think @alalek?

Also not that on this branch I did not find any other references to setNumberOfTickMarks so that must have been an existing oversight. Given that this PR removes tick marks completely, that is ok to leave uncorrected.

Options:

Merge this now to resolve the 30s wait when trying to create a slider with large range. Figure out desired behaviour and make a ticket for it.
Only allow 101 ticks. When there are more than 101 ticks, show 101 ticks and set setAllowsTickMarkValuesOnly: to NO. This is not preferred since it could create confusion for users who expect the ticks to line up with a number.

Right now, highgui trackbar with ticks around 10k is unusable on OSX so would love to get something in to restore functionality.

alalek · 2021-12-01T00:34:31Z

Sometime, on UI creation stage application don't know exact number of required ticks.
Ticks information may be available later (e.g., after choosing/loading of video file).
This is why we have cvSetTrackbarMax() in OpenCV API (which should be updated too).

Would then expect the trackbar to change in appearance? I believe it's better to offer a consistent UI in this situation.

I agree with that. It makes sense.

As a complete and consistent solution, I would vote for the option 2:

Only allow 101 ticks. When there are more than 101 ticks, show 101 ticks and set setAllowsTickMarkValuesOnly: to NO

I'm OK with the current PR state which removes ticks at all.

* remove tickmarks on NSSlider

mjmdavis added 2 commits November 28, 2021 20:13

opencv#21014 limit TickMarks on NSSlider for perf.

e17c19e

opencv#21014 disable slider allowsTickMarkValuesOnly

75a6f24

mjmdavis commented Nov 29, 2021

View reviewed changes

alalek reviewed Nov 29, 2021

View reviewed changes

remove tickmarks on NSSlider

e5bea4b

mjmdavis commented Nov 29, 2021

View reviewed changes

alalek approved these changes Dec 3, 2021

View reviewed changes

alalek merged commit d2f87ca into opencv:4.x Dec 3, 2021

alalek mentioned this pull request Dec 30, 2021

(5.x) Merge 4.x #21371

Merged

alalek mentioned this pull request Feb 22, 2022

(5.x) Merge 4.x #21651

Merged

a-sajjad72 pushed a commit to a-sajjad72/opencv that referenced this pull request Mar 30, 2023

Merge pull request opencv#21147 from mjmdavis:4.x

6ce71d2

* remove tickmarks on NSSlider

Uh oh!

Conversation

mjmdavis commented Nov 29, 2021 • edited by alalek Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Readiness Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mjmdavis commented Nov 29, 2021

Uh oh!

alalek left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mjmdavis commented Nov 29, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mjmdavis commented Nov 29, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mjmdavis commented Nov 30, 2021

Uh oh!

alalek commented Dec 1, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

mjmdavis commented Nov 29, 2021 •

edited by alalek

Loading

mjmdavis commented Nov 29, 2021 •

edited

Loading