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

[NO SQUASH] Create new UI API (Formspec/HUD replacement) #14263

Draft
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

v-rob
Copy link
Contributor

@v-rob v-rob commented Jan 15, 2024

Closes #6527. Continued from #12926, but reworked stuff like serialization, added full styling support, and made a Lua API for the UI.

This PR is officially Ready for Review undergoing some changes right now.

DO NOT SQUASH. The code is already in nice logical commits.

image

@v-rob v-rob mentioned this pull request Jan 15, 2024
14 tasks
@v-rob v-rob added @ Script API @ Client / Audiovisuals Roadmap The change matches an item on the current roadmap Feature ✨ PRs that add or enhance a feature Formspec labels Jan 15, 2024
@v-rob v-rob force-pushed the what-fun branch 7 times, most recently from 7968d0b to e3a4f26 Compare January 15, 2024 03:49
doc/lua_api.md Outdated Show resolved Hide resolved
@v-rob
Copy link
Contributor Author

v-rob commented Jan 17, 2024

Sizers were easier than I'd hoped, so I added them in. There's a flex and grid sizer, bearing no little resemblance to CSS grid and flexbox, albeit somewhat simplified. The documentation also has pretty ASCII images to document how layout works in more detail.

There's also a few other minor changes based on preliminary feedback.

Simple wrapping flex:
image

Simple grid:
image

Code
ui.set_default_theme(ui.Style{
	ui.Style{
		sel = "#root",
	},
	ui.Style{
		sel = "#base",

		rel_pos = {0.5, 0.5},
		rel_anchor = {0.5, 0.5},
		rel_size = {0, 0},

		bg_fill = "black#8C",
	},
})

local function flex_builder(id, player, cx)
	return ui.Window{
		type = "gui",

		style = ui.Style{
			noclip = true,

			ui.Style{
				sel = ".green",

				size = {100, 100},
				bg_fill = "green#8C",
			},
			ui.Style{
				sel = ".blue",

				size = {150, 100},
				bg_fill = "blue#8C",
			},
			ui.Style{
				sel = ".purple",

				size = {100, 120},
				bg_fill = "purple#8C",
			},
			ui.Style{
				sel = ".red",

				size = {100, 100},
				bg_fill = "red#8C",
				weight = 1,
			},
			ui.Style{
				sel = ".cyan",

				size = {100, 100},
				bg_fill = "cyan#8C",
				weight = 2,
			},
			ui.Style{
				sel = ".white",

				size = {700, 100},
				bg_fill = "white#8C",
			},
			ui.Style{
				sel = ".black",

				size = {100, 300},
				bg_fill = "black#8C",
			},
			ui.Style{
				sel = ".sneaky",

				size = {0, 0},
			},
		},

		root = ui.Elem{
			id = "root",

			ui.Flex{
				id = "base",
				dir = "right",
				wrap = "forward",

				--hspacing = "around",
				--vspacing = "remove",

				rel_size = {0.5, 0.9},

				padding = {5, 5, 5, 5},
				gap = {5, 10},

				ui.Elem{groups = {"green"}},
				ui.Elem{groups = {"red"}},
				ui.Elem{groups = {"sneaky"}},
				ui.Elem{groups = {"cyan"}},
				ui.Elem{groups = {"purple"}},
				ui.Elem{groups = {"white"}},
				ui.Elem{groups = {"green"}},
				ui.Elem{groups = {"red"}},
				ui.Elem{groups = {"blue"}},
				ui.Elem{groups = {"red"}},
				ui.Elem{groups = {"cyan"}},
				ui.Elem{groups = {"black"}},
				ui.Elem{groups = {"purple"}},

				--[[
				id = "base",
				dir = "down",
				wrap = "forward",

				hspacing = "remove",
				vspacing = "evenly",

				rel_size = {0.5, 0.9},

				padding = {5, 5, 5, 5},
				gap = {10, 5},

				ui.Elem{groups = {"black"}},
				ui.Elem{groups = {"green"}},
				ui.Elem{groups = {"blue"}},
				ui.Elem{groups = {"blue"}},
				ui.Elem{groups = {"purple"}},
				]]
			},
		},
	}
end

local function grid_builder(id, player, cx)
	return ui.Window{
		type = "gui",

		style = ui.Style{
			noclip = true,

			ui.Style{
				sel = ".green",

				size = {100, 100},
				bg_fill = "green#8C",
			},
			ui.Style{
				sel = ".blue",

				size = {150, 100},
				bg_fill = "blue#8C",
			},
			ui.Style{
				sel = ".purple",

				size = {100, 120},
				bg_fill = "purple#8C",
			},
			ui.Style{
				sel = ".white",

				size = {600, 100},
				bg_fill = "white#8C",
			},
			ui.Style{
				sel = ".black",

				size = {100, 300},
				bg_fill = "black#8C",
			},
			ui.Style{
				sel = ".sneaky",

				size = {0, 0},
			},
		},

		root = ui.Elem{
			id = "root",

			ui.Grid{
				id = "base",

				hsizes = {100, 0, 150},
				vsizes = {100, 120},
				hweights = {0, 1, 0},
				--vweights = {1, 5},

				--hspacing = "around",
				vspacing = "remove",

				rel_size = {0.5, 0.9},

				padding = {5, 5, 5, 5},
				gap = {5, 10},

				ui.Elem{groups = {"white"},  pos = {0, 0}, span = {3, 1}},
				ui.Elem{groups = {"green"},  pos = {1, 1}, span = {1, 1}},
				ui.Elem{groups = {"purple"}, pos = {0, 1}, span = {1, 1}},
				ui.Elem{groups = {"blue"},   pos = {2, 1}, span = {1, 1}},
				ui.Elem{groups = {"black"},  pos = {1, 2}, span = {1, 1}},
				ui.Elem{groups = {"sneaky"}, pos = {0, 0}, span = {3, 3}},
			},
		},
	}
end

core.register_on_joinplayer(function(player)
	local id = ui.open(flex_builder, player:get_player_name())
end)

@v-rob v-rob force-pushed the what-fun branch 3 times, most recently from babe696 to b2f28c9 Compare January 19, 2024 07:31
@v-rob
Copy link
Contributor Author

v-rob commented Jan 19, 2024

I beefed up style selectors in power, so they have things similar to child/parent selectors and stuff now. In some respects, they have a few features that are more convenient than CSS itself, such as the ability to group things like (button, checkbox)($hovered, $pressed) instead of the more verbose button$hovered, button$pressed, checkbox$hovered, checkbox$pressed). It still beats CSS for simplicity, though, which is definitely desirable.

As usual, here's some example code and a screenshot of the output (nothing amazing to look at, but shows the expected output):

Code
ui.set_default_theme(ui.Style{
	ui.Style{
		sel = "?root",
	},
	ui.Style{
		sel = "?<(?root)",

		rel_pos = {0.5, 0.5},
		rel_anchor = {0.5, 0.5},
		rel_size = {0, 0},

		bg_fill = "black#8C",
	},
})

local function style_builder(id, player, cx)
	return ui.Window{
		type = "gui",

		style = ui.Style{
			ui.Style{
				sel = "?<(flex)",

				size = {100, 100},
				bg_fill = "green#8C",
			},
			ui.Style{
				sel = "flex",

				fg_image = "cdb_queued.png",
				fg_scale = 1,

				ui.Style{
					sel = "?horizontal",

					fg_valign = "bottom",
				},
				ui.Style{
					sel = "?vertical",

					fg_halign = "right",
				},
			},
			ui.Style{
				sel = ".blue?<(flex)",

				bg_fill = "blue#8C",
			},
			ui.Style{
				sel = "flex!(/hud/, elem, #that)",

				bg_fill = "white#8C",
			},
			ui.Style{
				sel = "?nth_child(3)?nth_last_child(6)",

				bg_fill = "purple#8C",
			},
			ui.Style{
				sel = "flex?>(.blue)",

				rel_anchor = {0.4, 0.4},
			},
			ui.Style{
				sel = "?root?>>(.blue)",

				fg_image = "air.png",
				fg_scale = 1,
			},
			ui.Style{
				sel = ".what?<<(#bob)",

				bg_fill = "red#8C",
			},
			ui.Style{
				sel = ".eh?<>(.what)",

				bg_fill = "black#8C",
			},
			ui.Style{
				sel = ".maybe_empty?empty",

				bg_fill = "yellow#8C",
			},
			ui.Style{
				sel = "(?root, ?<(.maybe_empty))?only_child",

				fg_fill = "blue#3",
				fg_halign = "right",
			},
			ui.Style{
				sel = "(?first_child, ?last_child)?<(flex)",

				fg_image = "cdb_add.png",
			},
		},

		root = ui.Elem{
			id = "bob",
			groups = {"blue"},

			ui.Flex{
				id = "base",

				dir = "down",
				wrap = "forward",

				rel_size = {0.5, 0.5},

				padding = {5, 5, 5, 5},
				gap = {5, 5},

				ui.Elem{},
				ui.Elem{groups = {"blue"}},
				ui.Elem{},
				ui.Elem{groups = {"what", "eh"}},
				ui.Elem{groups = {"eh"}},
				ui.Elem{groups = {"maybe_empty"}},
				ui.Elem{
					groups = {"maybe_empty"},
					ui.Elem{
						fg_image = "cdb_update.png",
						fg_scale = 1,
					},
				},
				ui.Elem{},
			}
		},
	}
end

core.register_on_joinplayer(function(player)
	local id = ui.open(style_builder, player:get_player_name())
	core.after(2, function() ui.update(id) end)
end)

image

@v-rob v-rob force-pushed the what-fun branch 10 times, most recently from 6e94043 to 40c1f65 Compare January 24, 2024 08:35
@mark-wiemer
Copy link
Contributor

This looks like excellent work, from my reading of the conversation. I'm new to both Luanti and C++ but I have a ton of experience at Microsoft with UX design. Not much to say yet, but just want to congratulate and thank @v-rob and the team for their persistent efforts here, this new UI API will unlock a LOT of technological doors for Luanti game designers ❤️

Thanks everyone, merry Christmas, and I look forward to contributing to this PR however and whenever I can :)

@mruncreative
Copy link

mruncreative commented Dec 27, 2024

Ok, seems like everyone disagrees with me on clien't side. I still want texturpack support though. It's important for texturepacks to be able to provide a consistent look.
To me it's the biggest issue with formspec.

@mark-wiemer
Copy link
Contributor

mark-wiemer commented Dec 27, 2024

@mruncreative I appreciate your concern, but the focus of this PR is a bit different. Maybe you can find or file a better issue for "texture packs + formspecs" specifically to focus the discussion?

@grorp grorp added the Action / change needed Code still needs changes (PR) / more information requested (Issues) label Dec 29, 2024
@Zughy
Copy link
Contributor

Zughy commented Jan 3, 2025

@v-rob reminder

@rubenwardy rubenwardy removed this from the 5.11.0 milestone Jan 5, 2025
@Zughy Zughy added this to the 5.12.0 milestone Jan 5, 2025
```

Lastly, state precedences combine. A style for pressed and hovered buttons will
override styles for only pressed or only hovered buttons. The highest state
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So basically lexicographical ordering based on state precedences?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Basically, yes, where higher precedence means listed later in lexicographical ordering.

@ExeVirus
Copy link
Contributor

ExeVirus commented Jan 6, 2025

Very Minor Nitpick

I see ipairs used extensively. In general, since this is falling to the main-thread lua, we should aggressively optimize our limited compute here. A small, easy optimization is to instead use #indexing, typically a 10-15% perf boost for iterations for minimal effort:

Demonstration lua script
#!/usr/bin/env luajit

--[[
Adapted from https://gist.github.com/dpino/647b9d2e12aa8ff859b4

Environment: Lenovo Legion  / 16GB RAM /  i7-9750H CPU @ 2.60GHz

         |  pairs   |  ipairs  | indexing |
pairs    | 1.000000 | 3.007634 | 3.549550 |
ipairs   | 0.332487 | 1.000000 | 1.180180 |
indexing | 0.281726 | 0.847328 | 1.000000 |
]]--

local ITERATIONS = 1000000

local x

local function array(n)
    local result = {}
    for i=1, n do
        table.insert(result, tostring(i))
    end
    return result
end

local function execute_times(f, times)
    local a = array(100)
    local begin = os.clock()
    for i=1,times do
        f(a)
    end
    local finish = os.clock()
    return finish - begin
end

local times = {}

-- Pairs
local elapse_time1 = execute_times(function(a)
    for j,v in pairs(a) do
        x=v
    end
end, ITERATIONS)
times["pairs"] = elapse_time1

-- iPairs
local elapse_time2 = execute_times(function(a)
    for _,v in ipairs(a) do
        x=v
    end
end, ITERATIONS)
times["ipairs"] = elapse_time2

-- Indexing
local elapse_time3 = execute_times(function(a)
    local len = #a
    for j=1,len do
        x=a[j]
    end
end, ITERATIONS)
times["indexing"] = elapse_time3

local results = {
    { times["pairs"]/times["pairs"], times["pairs"]/times["ipairs"], times["pairs"]/times["indexing"] },
    { times["ipairs"]/times["pairs"], times["ipairs"]/times["ipairs"], times["ipairs"]/times["indexing"] },
    { times["indexing"]/times["pairs"], times["indexing"]/times["ipairs"], times["indexing"]/times["indexing"] },
}
print("|  pairs   |  ipairs  | indexing |")
for _, row in ipairs(results) do
    io.write(string.format("| %.6f ", row[1]))
    io.write(string.format("| %.6f |", row[2]))
    io.write(string.format(" %.6f |", row[3]))
    io.write("\n")
end

@@ -36,6 +36,7 @@ set(BUILD_SERVER FALSE CACHE BOOL "Build server")
set(BUILD_UNITTESTS TRUE CACHE BOOL "Build unittests")
set(BUILD_BENCHMARKS FALSE CACHE BOOL "Build benchmarks")
set(BUILD_DOCUMENTATION TRUE CACHE BOOL "Build documentation")
set(BUILD_UI FALSE CACHE BOOL "Build experimental UI API")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please specify that Build Client and USE_SDL2 are required here, rather than just fatal erroring when they try it.

return rect[1], rect[2], rect[1], rect[2]
elseif #rect == 1 then
return rect[1], rect[1], rect[1], rect[1]
end
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might want to make #rect a local len variable, rather than calling #rect 3 times.

return default_theme
end

function ui.set_default_theme(theme)
Copy link
Contributor

@ExeVirus ExeVirus Jan 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Error check here to ensure theme is of type ui.Style {}

Also does this use table.copy or direct table pass through?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Style objects are immutable, so no need to copy.

There's a lot of places I could do type assertions, but I currently don't. Arguably that's not the best, and I could add it everywhere.

@@ -14,6 +14,7 @@ General options and their default values:
BUILD_UNITTESTS=TRUE - Build unittest sources
BUILD_BENCHMARKS=FALSE - Build benchmark sources
BUILD_DOCUMENTATION=TRUE - Build doxygen documentation
BUILD_UI=TRUE - Build experimental UI API
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mention Requiring SDL2 and BUILD_CLIENT

end)
```

Note that, since the UI API has no theme bundled by default at this point, a
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You have the prelude theme bundled at this point, no?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Technically, yeah. Here, I meant there's no theme that gives default visuals.

checkbox, which has a dynamic `main` box, the checkbox will have the `hovered`
state whereas the parent button will not. However, if the mouse hovers over the
label, which has a static `main` box, the event will pass through the label and
the parent button will have the `hovered` state instead.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You cover it later, but at this point in the readme, it's not clear that a touch/click event will propagate downward to each underneath dynamic box until handled. After reading this section, it sounds like they only hit the top-most dynamic box.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They do only hit the topmost dynamic box, which is the intended behavior. Does it say somewhere that it hits all of them?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://github.com/minetest/minetest/blob/93ec0443ea10e677c23059f0e2cd5ab9c0b59ac8/doc/experimental_ui_api.md?plain=1#L732

When mouse or touch input is received by the window, it first allows the
focused element and its parents to peek at the event, such as to let buttons
become unpressed. Then, it sends the input to the hovered element. If it
ignores the input, the input passes to the element directly below it (which is
not necessarily its parent element), and so on. If none of them used the input,
the window again gets to use the input, possibly sending it to the server.

Here you imply that, if an event goes unhandled by that topmost dynamic box, it will be distributed downward. Or is this only the case for hover? I would expect it to also apply in the case of clicks

pressed, the window sets the focused element to the topmost focusable element
under the cursor. Lastly, if the mouse is double clicked outside of any box
except the backdrop of the root element, then the window will be closed if it
is not uncloseable.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we make double click/double touch a disable-able option/ I mean, I guess we could put an invisible box there, but seems like a hack

Copy link
Contributor Author

@v-rob v-rob Jan 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why would you want to disable it? Formspecs always do it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just because it's available for formspecs to be always there, in this rewrite I would hope that would be dehardcoded. Had my fair share of mistaken double clicks or double taps, personally. I'm sure others have too.

Copy link
Contributor Author

@v-rob v-rob Jan 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think it should be an option, since that's just going to end up with inconsistencies between mods. I'd prefer to either require the behavior or do away with it entirely. Either way is fine for me.

The tab key is used for changing which element has user focus via the keyboard.
Pressing tab will cause the next focusable element in the element tree to
become focused, whereas pressing shift+tab will transfer focus to the previous
focusable element.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Next adjacent element? or next child element?

What if I'm on layer 3, and the next element is at layer 2?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To be precise, repeatedly hitting tab will do a preorder traversal of focusable elements in the element tree. I will clarify that.

no other purpose than being a base for other themes. The prelude theme sets
style properties that are essential to the functionality of the element, or are
at least a basic part of the element's intended design. It can be accessed with
the `ui.get_prelude_theme()` function.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Core theme vs prelude theme, will they be one in the same or no?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They are separate. The prelude is designed to be the base for every theme (including the core theme), and doesn't give any visuals. Imagine the prelude theme as a web browser's default stylesheet after a CSS reset has been used--things like <head> tags are still hidden, but other things like buttons don't have a distinct look from any other element.

on_scroll = function(ev)
state.scroll = ev.value
context:update()
end,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, scrolling is server side? or will it be both?

I'm thinking that both would allow lazy loading, I guess, can't think of many other use cases yet.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Scrolling will be client-side; I'm just storing the scroll value on the server for the sole purpose of displaying a label.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

or will it be both?

but the server does have an on_scroll callback, then?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It will when scrollbars exist :) They don't yet, as mentioned in the paragraph above the example.

will automatically generate IDs when none is required.
* The format of this ID is not precisely specified, but it will have the
format of an engine reserved ID and will not conflict with any other IDs
generated during this session.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ui.new_id() gives a number of 0->large. What If I also give an id that is just a number "1"?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, it gives an ID of _0, which is an engine-reserved ID. Mods are not allowed to use IDs with leading underscores or dashes.

Defaults to the theme provided by `ui.get_default_theme()`.
* `style` (`ui.Style`): Specifies a style to apply across the entire element
tree. Defaults to an empty style.
* `uncloseable` (boolean): Indicates whether the user is able to close the
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Love that this is an option, what if there's a bug and a modder/game maker gives them no buttons to close with? How do they exit to the main menu?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the future, ESC should likely open the pause menu, but for now you'd have to Alt+F4.

@ExeVirus
Copy link
Contributor

ExeVirus commented Jan 6, 2025

A key feature I expected from unifying HUD and formspecs into this new UI interface was control of when the mouse is captured vs not, and keyboard is captured vs not.

In my head, I expected the ability to create "hud" or "gui" windows/contexts where I specify if that window/context captures the mouse, or keyboard explicitly.

For example, say I want to leave the keyboard for walking/interacting with the game (not the window), but have the mouse available for button/clicking? Right now, that keyboard event could only be passed through if I do so on the server and replicate the action on the client, which will definitely have lag and be an issue.

Some example things we can do by having these two captures (keyboard, mouse, heck individual keys) separated and controllable:

  1. Capture any keyboard button, while mouse can still look around.
  2. Capture new keyboard buttons, while leaving mouse and normal keyboard buttons alone. I.e. new server-side only buttons allowed
  3. Capture just the mouse for clicking HUD elements, while leaving the keyboard for movement across the screen. Think a 2D game/city builder game where the user is locked to a 2D grid view, and uses mouse for interaction.

Going one step further, allow passthrough to other contexts/not "gui" window contexts when the event goes unhandled. That would give us a TON more flexibility.

Of course, all this means a modder/game designer needs to consider mobile vs. non-mobile users. Which adds complexity but is doable as of today.

@v-rob
Copy link
Contributor Author

v-rob commented Jan 6, 2025

I see ipairs used extensively. In general, since this is falling to the main-thread lua, we should aggressively optimize our limited compute here. A small, easy optimization is to instead use #indexing, typically a 10-15% perf boost for iterations for minimal effort:

This sounds like a premature optimization; I doubt ipairs is a significant bottleneck in the UI API, if at all.

A key feature I expected from unifying HUD and formspecs into this new UI interface was control of when the mouse is captured vs not, and keyboard is captured vs not.

Capturing keyboard buttons in the UI layer as a means of implementing custom keybindings is a hack, not really a good example. I don't understand the use for capturing the mouse but leaving the keyboard controls in the third example--do you just mean WASD for moving around?

The only really useful example I've thought of for this is pressing buttons on the HUD with a touchscreen, e.g. for custom controls, while still allowing interaction with the game itself. That is a planned feature in the future, although I was planning on just adding a new window type for that express purpose.

@ExeVirus
Copy link
Contributor

ExeVirus commented Jan 7, 2025

Capturing keyboard buttons in the UI layer as a means of implementing custom keybindings is a hack
Fair

I don't understand the use for capturing the mouse but leaving the keyboard controls in the third example--do you just mean WASD for moving around?

Absolutely, more than that, but at least that, yes. movement while selecting things, walking with a minimap overlay I can click on, RTS overlay buttons , etc.

That is a planned feature in the future

Oh then no worries, I would just put that feature into base "hub" and "gui" contexts, but sure another breakout if needed, sure.

since that's just going to end up with inconsistencies between mods

This comment and the others -> I think you are thinking too traditional MT games specific, and I'm thinking more along the lines of games, where we want more complete control in the game designers hands

That said: this is your baby, you've done 99% of the work. Let's not bike shed flexibility features in what is an initial, experimental UI. I can make the case for the proposal via my own PR rather than asking for it here.

@v-rob
Copy link
Contributor Author

v-rob commented Jan 7, 2025

So, I finished adding proper support for labels two weeks ago (all the basic stuff, with font, size, bold, color, etc.). I was also considering prepend and append properties to add text before and after a label, but I ran into similar issues with over-specialization like icon images. After further thought, I agree that icon images aren't a good idea after all.

As a proposal, I think something akin to CSS ::before might be the best generalization. To include an icon and text on a button, you can do

ui.Style "#ex" {
    before = {
        box_image = "cancel.png",
        -- Probably introduce a property that factors the size of the
        -- background image into the minimum size of the box.
    },
    after = {
        text = "Example text",
        -- Labels would become a style property, not an element property.
    },
}

ui.Button "ex" {
    ui.Elem "a" {},
    ui.Elem "b" {},
}

The before and after properties, if present, would add an entire extra box before or after the box's children. The above example would give the following (assuming a horizontal flex sizer was used):

+---------------------------------------+
| +------+ +---+ +---+ +--------------+ |
| | icon | | a | | b | | Example text | |
| +------+ +---+ +---+ +--------------+ |
+---------------------------------------+

Writing out that kind of styling would be extremely verbose. Adding support for custom attributes could solve that problem:

ui.Style "button" {
    before = {
        box_image = ui.attr "icon",
    },
    after = {
        text = ui.attr "label",
    },
}

ui.Button "ex" {
    icon = "cancel.png",
    label = "Example text",
}

Custom attributes need not be difficult to implement, and honestly could remove the need for groups and group selectors entirely. I do like how much more general and extensible this solution would be, particularly since it can take advantage of the powerful layout sizers. Nevertheless, there's a fair amount of complexity involved, so I'm not entirely sold on the idea.

Comment on lines +742 to +744
under the cursor. Lastly, if the mouse is double clicked outside of any box
except the backdrop of the root element, then the window will be closed if it
is not uncloseable.
Copy link
Member

@grorp grorp Jan 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This behavior is not necessarily needed on desktop since you have ESC there, but on mobile it's often the most convenient way to exit a formsec (in reply to ExeVirus)

Formspecs have since a few versions changed from "double-tap outside to exit" to "single tap outside to exit" because that's a common convention for mobile UIs and makes it more discoverable, so the new UI API should do the same

Copy link
Contributor Author

@v-rob v-rob Jan 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will remove the double click to exit functionality for desktop then.

@grorp
Copy link
Member

grorp commented Jan 11, 2025

Discussion from Matrix:

grorp

  • I'm really happy you changed your mind :))
  • My personal opinion on the concern "Writing out that kind of styling would be extremely verbose": Verbose but boring and predictable is better than brief but weird
  • One thing you could do to remove verbosity and redundancy (and I imagine people will do that anyway because it's so obvious) is having a function for creating buttons with background, label and icon, with a return <Lua DSL syntax> ... could do this for all more high-level UI components actually... this would allow keeping the UI API itself really general, with little annoying hardcoded stuff, because people could simply write their own functions for high-level components if they don't like the built-in ones

grorp
This is similar to the custom attributes thing you suggested, but would be more natural because the syntax and language we're using supports it anyway, instead of it being a UI API-specific feature that also requires implementation effort

grorp
And you wouldn't even need to add the special ::before/::after thing that way, the component function could just add child elements for icon/label without adding verbosity to user code

v-rob
Thanks for feedback :) You know, just providing custom button/element constructors is such an obvious idea, I don't see why in the world I didn't think of it myself. Couldn't see the forest for the trees, I guess :P It's a much better idea than custom attributes, which felt problematic in multiple ways.

I do still favor the idea of before/after over explicit child elements, since I think it's important for images and text content to be controllable by styling without having to resort to child selector weirdness. And, (as my favorite example goes), arrow images on scrollbar buttons and suchlike are possible with a before property but not with child elements.

v-rob
I will get to work on that then. Ho hum, I'll have to revise significant portions of the documentation again...

grorp
For the scrollbar thing, I have one general something: On mobile, you usually don't have scrollbars, but swipe to scroll, with the occasional scrolling progress indicator that looks a bit like a scrollbar. If the API's basic concept of scrolling is a scrollbar (the thing that manages the scrolling is the scrollbar), it's hard to make that work without hacks, since you need scrolling to be bound to an area for swipe to scroll. So I'd prefer if the element managing the scrolling would be a scrolling area/container and the scrollbar would just be an input bound to that, to make it future-proof for a swipe to scroll implementation. (I have no idea how you're currently planning to do scrolling, this is just something that came to my mind after working on a swipe to scroll implementation for formspecs and being annoyed.)

v-rob
I have taken this into consideration. For scroll containers, the scrollbar will be an integrated part of the container itself, as a set of boxes within it. For instance, inside the main box of a scroll container, there would be a scrollbar box that contains the boxes for the scrollbar (e.g. track, thumb, forward, etc.). The scroll container will handle all the user input, including touch swipes. Moreover, if you don't want the up/down buttons, you can trivially hide them via the theme.

v-rob
I want mobile to be nothing less than a first-class citizen. Touch support is deplorable in formspecs.

v-rob
(I plan to implement touch support alongside scroll containers, since that is a good test of touch functionality.)

@Zughy Zughy added the Rebase needed The PR needs to be rebased by its author label Feb 1, 2025
@rubenwardy rubenwardy removed this from the 5.12.0 milestone Feb 1, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Action / change needed Code still needs changes (PR) / more information requested (Issues) @ Client / Audiovisuals Feature ✨ PRs that add or enhance a feature Formspec Rebase needed The PR needs to be rebased by its author Roadmap The change matches an item on the current roadmap @ Script API
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Formspec replacement