Add more fill modes to ProgressBar

[floppyhammer]

Add more fill modes to ProgressBar, as TextureProgressBar already has many. Currently, only FILL_RIGHT_TO_LEFT, FILL_TOP_TO_BOTTOM, and FILL_BOTTOM_TO_TOP were implemented besides the default FILL_LEFT_TO_RIGHT mode.

Bugsquad edit: This closes godotengine/godot-proposals#4514.

[floppyhammer]

I have a question though. What does is_layout_rtl() do in progress_bar.cpp? Since I don't know when the function returns true, I haven't tested the fill modes in those situations yet.

[EricEzaM]

I have a question though. What does is_layout_rtl() do in progress_bar.cpp? Since I don't know when the function returns true, I haven't tested the fill modes in those situations yet.

It's to do with right-to-left UI layouts, typically used for languages which read R-T-L. It checks if the setting is set which makes it RLT... that's about all I know, @bruvzg could explain more.

[floppyhammer]

I changed the editor language to Arabic and now is_layout_rtl() returns true. But I don't see the reason for ProgressBar to behave differently for different language layouts. Also TextureProgressBar doesn't do this check at all.

[groud]

I changed the editor language to Arabic and now is_layout_rtl() returns true. But I don't see the reason for ProgressBar to behave differently for different language layouts. Also TextureProgressBar doesn't do this check at all.

Well, it makes sense that Right-to-Left languages have progress bars filling from the right to the left. If @bruvzg implemented it, it means that likely how things happen with right-to-left languages. Please keep this behavior.

However it is not needed for top-to-bottom and bottom-to-top, so your PR needs an edit there.

[floppyhammer]

@groud But considering it's now possible to manually select from left-to-right and right-to-left fill modes, isn't it more reasonable to discard this check. Otherwise, when the left-to-right is selected by default, the progress actually goes from right to left. Instead, we could do this check when setting the initial fill mode, i.e. left-to-right for usual languages and right-to-left for rtl languages.

[groud]

@groud But considering it's now possible to manually select from left-to-right and right-to-left fill modes, isn't it more reasonable to discard this check. Otherwise, when the left-to-right is selected by default, the progress actually goes from right to left. Instead, we could do this check when setting the initial fill mode, i.e. left-to-right for usual languages and right-to-left for rtl languages.

No, this has to be automatic, that's the point of what @bruvzg implemented.
I don't know much about the details, but if I record correctly, you can override this behavior at the Control node level.

Worst case, it could eventually be renamed to "Begin to end" or something like that. But for now, it's better to keep compatibility.

[floppyhammer]

@groud Alright. But it might need some fix as it behaves like this with original code:

In addition, it seems it goes from left to right by default in the first place (I moved original code inside left_to_right case block, so it should behave exactly the same way as before when left_to_right fill mode is selected).

[groud]

@groud Alright. But it might need some fix as it behaves like this with original code

Ah, It might indeed need a bugfix I don't know.

[Calinou]

Feature looks good to me.

[Calinou]

Should be good to merge after rebasing.

[floppyhammer]

@Calinou Rebasing done.

[akien-mga]

I think this might not respect the UI mirroring convention for RTL. I see there's a switch in the constructor but that's only valid for the default mode.

It would likely be better to use BEGIN_TO_END instead of LEFT_TO_RIGHT and do the mirroring there.

[floppyhammer]

@akien-mga Makes sense.

[floppyhammer]

@akien-mga Might be good to go now.

[akien-mga]

Could maybe be simplified like this (untested):

Suggested change

	case FILL_BEGIN_TO_END: {
	int mp = fg->get_minimum_size().width;
	int p = round(r * (get_size().width - mp));
	if (p > 0) {
	if (is_layout_rtl()) {
	int p_remaining = round((1.0 - r) * (get_size().width - mp));
	draw_style_box(fg, Rect2(Point2(p_remaining, 0), Size2(p + fg->get_minimum_size().width, get_size().height)));
	} else {
	draw_style_box(fg, Rect2(Point2(0, 0), Size2(p + fg->get_minimum_size().width, get_size().height)));
	}
	}
	} break;
	case FILL_END_TO_BEGIN: {
	int mp = fg->get_minimum_size().width;
	int p = round(r * (get_size().width - mp));
	if (p > 0) {
	if (is_layout_rtl()) {
	draw_style_box(fg, Rect2(Point2(0, 0), Size2(p + fg->get_minimum_size().width, get_size().height)));
	} else {
	int p_remaining = round((1.0 - r) * (get_size().width - mp));
	draw_style_box(fg, Rect2(Point2(p_remaining, 0), Size2(p + fg->get_minimum_size().width, get_size().height)));
	}
	}
	} break;
	case FILL_BEGIN_TO_END:
	case FILL_END_TO_BEGIN: {
	int mp = fg->get_minimum_size().width;
	int p = round(r * (get_size().width - mp));
	// We want FILL_BEGIN_TO_END to map to right to left when UI layout is RTL,
	// and left to right otherwise. And likewise for FILL_END_TO_BEGIN.
	bool right_to_left = is_layout_rtl() ? (mode == FILL_BEGIN_TO_END) : (mode == FILL_END_TO_BEGIN);
	if (p > 0) {
	if (right_to_left) {
	int p_remaining = round((1.0 - r) * (get_size().width - mp));
	draw_style_box(fg, Rect2(Point2(p_remaining, 0), Size2(p + fg->get_minimum_size().width, get_size().height)));
	} else {
	draw_style_box(fg, Rect2(Point2(0, 0), Size2(p + fg->get_minimum_size().width, get_size().height)));
	}
	}
	} break;

[akien-mga]

Looks good to me. Left a suggestion to reduce code duplication but it's also fine as is.
Would be good to get review from @bruvzg who's more familiar than me with UI mirroring requirements.

[groud]

Maybe we could precise what being and end means? Or at least mention it's dependent on the language direction? Something like:

The progress bar fills from begin to end horizontally, according to the language direction.

[akien-mga]

Maybe:

The progress bar fills from begin to end horizontally, according to the layout direction. If [method Control.is_layout_rtl] is [code]false[/code], it fills from left to right, and if it is [code]true[/code], it fills from right to left.

[kleonc]

Is "<method> is <return value>" correct though? I think it should be "<method> returns <return value>".
(yeah, nitpicking)

The progress bar fills from begin to end horizontally, according to the layout direction. If [method Control.is_layout_rtl] returns [code]false[/code], it fills from left to right, and if it returns [code]true[/code], it fills from right to left.

[akien-mga]

Yeah that's better. I thought initially that it's a property but it's indeed a method.

[bruvzg]

Seems fine. But I guess it should be documented what's begin-to-end means for different layout directions.

Changes done.

[akien-mga]

Thanks!

[MaaaxiKing]

What was the problem with just rotating the whole Node?

[kleonc]

What was the problem with just rotating the whole Node?

You mean why different modes instead of just rotating the Node? Rotating the Node will rotate all of its contents, including percent text or children of that Node. And this might be not wanted.

[KoBeWi]

Also rotating won't work in a container.