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

Add scrollbaroptions FormSpec element #8530

Open
wants to merge 19 commits into
base: master
from

Conversation

@v-rob
Copy link
Contributor

v-rob commented May 14, 2019

This PR adds a scrollbaroptions element for formspecs. Similar to tableoptions, it must be placed before the scrollbars it modifies. Old scrollbars are not affected. Closes #8575.

Valid options are:

  • min - Minimum value of the scrollbar (default 0)
  • max - Maximum value of the scrollbar (default 1000)
  • smallstep - Scroll amount when using arrows or scroll wheel (default 10)
  • largestep - Scroll amount when clicking the bar itself (default 100)
  • thumbsize - Size of the thumb determined by the number of unit the thumb spans out of the range of values (max - min).
  • arrows - Whether to make the arrows hidden, shown, or automatic.

Some potential examples:

scrollbaroptions[min=0;max=255;smallstep=8;largestep=16;thumbsize=8;arrows=hide]
scrollbaroptions[max=10000;largestep=1000]

If the step value is negative, it will use Irrlicht's defaults. If the max is less than or equal to the min, the scrollbar element will have no bar and do nothing, i.e. disabled. If thumbsize is zero or less, it defaults to 1 (which usually will appear as a square).

This PR is Ready for Review.

@DS-Minetest

This comment has been minimized.

Copy link
Contributor

DS-Minetest commented May 14, 2019

Imo it would be good if #8507 is merged before this, so that here the option sliderlength can be added.
Edit: However, this option could be added later without problems.
Ergo just related: #8507

@v-rob

This comment has been minimized.

Copy link
Contributor Author

v-rob commented Jun 6, 2019

I'm trying to incorporate the new guiScrollBar class in this PR, and I can't figure out what's going incorrectly. The error code is rather undecipherable, and I can't figure anything out from it. I think the error lies in line 516, but I'm not certain. I based this guiScrollBar on the one in guiEditBoxWithScrollbar.cpp

src/gui/guiFormSpecMenu.cpp Outdated Show resolved Hide resolved
@DS-Minetest

This comment has been minimized.

Copy link
Contributor

DS-Minetest commented Jun 6, 2019

Have you rebased already? A not existing class might maybe explain the error.

src/gui/guiFormSpecMenu.cpp Outdated Show resolved Hide resolved
@v-rob

This comment has been minimized.

Copy link
Contributor Author

v-rob commented Jun 6, 2019

I have rebased as evidenced by this file: https://github.com/v-rob/minetest/blob/scrollbaroptions/src/gui/guiScrollBar.cpp. I believe the problem lies in the instantiation of guiScrollBar, but I don't have much to work off of. Maybe @stujones11 can shed some light on this, since he made it.

@DS-Minetest

This comment has been minimized.

Copy link
Contributor

DS-Minetest commented Jun 7, 2019

@v-rob "guiScrollBar.h" is not yet included.

@v-rob

This comment has been minimized.

Copy link
Contributor Author

v-rob commented Jun 8, 2019

Well, it works now (almost). I just needed to include guiScrollBar.h (duh!) as well as change a few IGUIScrollBar things to the new guiScrollBar. Now I just need to make the bar change size properly -- see the screenshot :) Shouldn't be too hard.

large_scrollbar

@paramat paramat added the Formspec label Jun 8, 2019
@v-rob

This comment has been minimized.

Copy link
Contributor Author

v-rob commented Jun 10, 2019

I still have two bugs to iron out before this PR can be merged: First, I still don't think that I have pagesize quite right. The other bug is that trying to get the value from the scrollbar always returns INV (which is failure). I need to do some more testing.

guiScrollBar* e = new guiScrollBar(Environment, this, spec.fid, rect,
is_horizontal, true);

int max = data->scrollBarOptions.max;

This comment has been minimized.

Copy link
@DS-Minetest

DS-Minetest Jun 10, 2019

Contributor

As far as I have seen, minetest seems to use s32, not int.

@SmallJoker SmallJoker added the WIP label Jun 10, 2019
@DS-Minetest

This comment has been minimized.

Copy link
Contributor

DS-Minetest commented Jun 10, 2019

If you want the default quadratic slider rectangle, you have to set page_size as large as possible, this results in a thumb_size of (nearly) 0, which is smaller than thumb_min and will ergo be set to thumb_min.

If the length of the scrollbar (including borders) is the length of the visible part of the whole page that you scroll over, page_size is the length of the whole page.
(All this lengths are in pixels.)

The length of the scrollbar over page_size is kept equal to thumb_size over thumb_area.

Imo, this is very confusing and not really useful for modders.
There should be something else that you can set.

Maybe just give the thumb size in scrollbar units.

Example 1: The whole page is 1000 scrollbar units long (max-min). (In pixels it's 1000*some_factor.) The visible part is 10*same_factor_as_before pixels long. => The thumb has to be 10 long (in scrollbar units).

Example 2: You can choose one number out of 10 with a scrollbar (from 0 to 9). Ergo min is 0 and max is 9. And thumb_size in scrollbar units is 1 because it's always one number at a time.

Now, the question is: How to convert thumb_size in scrollbar units into page_size in pixels?

Wanted value:
page_size (in pixels)

Given are:
ts (thumb size in scrollbar units)
scrollbar_size (length of the scrollbar in pixels)
min (in scrollbar units)
max (in scrollbar units)
thumb_area (in pixels (can be calculated))

Other variables:
thumb_size (in pixels)
sutopi (scrollbar unit to pixel (= pixels per scrollbar unit))
pitosu (1/sutopi)


thumb_area = (max-min+1) * sutopi
=> sutopi = thumb_area / (max-min+1)

thumb_size = ts*sutopi

thumb_size = thumb_area / (page_size / scrollbar_size)

=> ts*sutopi = thumb_area / (page_size / scrollbar_size)

page_size / scrollbar_size * ts * sutopi = thumb_area

page_size = thumb_area * scrollbar_size / ts / sutopi
          = thumb_area * scrollbar_size / ts / (thumb_area / (max-min+1))
          = thumb_area * scrollbar_size / ts / thumb_area * (max-min+1)
          = scrollbar_size * (max-min+1) / ts
s32 scrollbar_size;
if (is_horizontal)
	scrollbar_size = dim.X;
else
	scrollbar_size = dim.Y;

e->setPageSize(scrollbar_size * (max-min+1) / data->scrollBarOptions.thumbSize);

Edit: This is done now.

src/gui/guiFormSpecMenu.cpp Outdated Show resolved Hide resolved
@rubenwardy rubenwardy self-requested a review Jun 21, 2019
@rubenwardy rubenwardy dismissed their stale review Jun 21, 2019

wip

@rubenwardy rubenwardy force-pushed the minetest:master branch from bbc5aa0 to 0e3b135 Jun 21, 2019
src/gui/guiFormSpecMenu.h Outdated Show resolved Hide resolved
src/gui/guiFormSpecMenu.h Outdated Show resolved Hide resolved
src/gui/guiFormSpecMenu.cpp Outdated Show resolved Hide resolved
src/gui/guiFormSpecMenu.cpp Outdated Show resolved Hide resolved
@v-rob v-rob force-pushed the v-rob:scrollbaroptions branch from 902dbde to c4a59d8 Oct 15, 2019
src/gui/guiScrollBar.h Outdated Show resolved Hide resolved
@v-rob v-rob force-pushed the v-rob:scrollbaroptions branch 2 times, most recently from 849dc4c to aa12766 Oct 16, 2019
@SmallJoker

This comment has been minimized.

Copy link
Member

SmallJoker commented Nov 10, 2019

@v-rob Due to @DS-Minetest's formspec changes you might need to move some code.

I lost track of this PR, but it apparently only needs re-reviewing after rebase and some quick tests (which I actually wanted to do now).

@v-rob v-rob force-pushed the v-rob:scrollbaroptions branch from aa12766 to deea41c Nov 17, 2019
@v-rob

This comment has been minimized.

Copy link
Contributor Author

v-rob commented Nov 17, 2019

Sorry it took so long to rebase. I forget how to rebase properly every time I try it, so I wrote it down this time. Hopefully it works.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
6 participants
You can’t perform that action at this time.