Mainmenu Improvements: "Join Game" tab + code cleanup #9505

appgurueu · 2020-03-11T18:48:57Z

Goal of the PR? Improves the mainmenu, in particular the "Join Game" tab.
How does the PR work? Cleaned up & tweaked builtin mainmenu stuff.
Does it resolve any reported issue? Works towards Improve mainmenu [See proposal] #6733
If not a bug fix, why is this PR needed? What usecases does it solve? Takes less space, makes the "Join Game" tab more intuitive, cleans up code. See [To do].

Before

After

To do

This PR is Ready For Review.

Remove simple main menu
Clean up code, migrate to real coordinates
Organize form, improve style
Merge gamemode indicators
Add dividers, sort servers as favorites - discover - incompatible
Network indicators: Replaced with lag + ping lagometers
Gamemode indicators: Creative + Damage combination

How to test

Fire up Minetest.

builtin/mainmenu/common.lua

paramat · 2020-03-11T22:10:32Z

I am 👎 for combining the game mode icons in this way.
See #6733 (comment) for why.

If done, this needs discussion in an issue first of what information needs to be shown, then how to display that in a single column, probably by using a new set of icons with superimposed elements.

…k size

appgurueu · 2020-03-12T14:27:41Z

Well, here goes my (disagreeing) reply:

I have written a script to filter all "weird" combinations.
FLAGS: creative; damage, COMPILED TO: creative
*** BuildBattle *** by Telesight
MT   L I T T L E   P O N I E S
Voor iedereen, for everybody
Walhalla
FLAGS: creative; damage; pvp, COMPILED TO: creative
--Parkour-- by Survival--
Ahmed Hamza Kenan
DudePro
German-Trash-server
MUNDO CREATIVO
Outland
Prool's server. Сервер Пруля
SP Server
Sockhausen
TroncoPaja
ardiblox
minetest.zirtin.net
« Parkour » by Lowe
FLAGS: creative; pvp, COMPILED TO: creative
Adventskalender
Go! Go! Sango!! Tatsuta Club (^^)v
MSC_Minetest
Mesozoic Minetest!
Osterwelt Kemnat
Tempelwelt
\__+albATros+__/
There are three categories - creative + damage, creative + pvp, and creative + damage + pvp.
Most of the less than 20 servers on the list are rather unpopular (and/or not too well maintained).

The only notable (IMO) is Telesight's BuildBattle, which would be (properly) displayed as creative instead of the confusing "creative, damage enabled".

rubenwardy · 2020-03-12T14:31:16Z

The problem is that creative is a concept that is defined by minetest game. Creative, damage, and PvP is perfectly valid in certain games - the server may not have a way to heal using creative. I can imagine a PvP game where you can place infinite blocks whilst attempting to knock people off their blocks - like spleef

appgurueu · 2020-03-12T14:49:37Z

A spleef server should be marked as just PvP. There shouldn't be three flags at all.

rubenwardy · 2020-03-12T14:55:21Z

In which case, the order of your priorities in this pr is wrong

appgurueu · 2020-03-12T14:57:32Z

Let me rephrase: A server should have one gamemode.

Current common gamemodes are creative, survival and PvP, so they are supported by the serverlist & my mainmenu improvements.

rubenwardy · 2020-03-12T15:10:11Z

Minetest has no concept of a game mode

appgurueu · 2020-03-12T15:23:05Z

Well, servers which fall outside of the common categories can just set none and get no image at all.

rubenwardy · 2020-03-12T15:30:37Z

The flags are set using the setting which control the various things. It's not possible to set none and retain the functionality

MoNTE48 · 2020-03-12T15:42:36Z

In my fork I use
Mode: Creative / Survival
PVP: On / Off
The most understandable for the player!

MoNTE48 · 2020-03-12T15:43:40Z

Your X to clear the search looks so disgusting. Sorry, but it’s better to remove it.
And you can separate
Address: Port:
Name: Password:

builtin/settingtypes.txt

appgurueu · 2020-03-12T16:23:29Z

Your X to clear the search looks so disgusting. Sorry, but it’s better to remove it.

I won't. I will probably replace the texture (with a trash bin or the like, this one has ugly scaling artifacts). Note that I have taken the "X" and the magnifying glass from the creative mod of MTG.

And you can separate [...]

Done.

appgurueu · 2020-03-14T11:01:09Z

Here's my compromise: the "unknown" indicator, which is applied for creative + (damage enabled or pvp enabled).

appgurueu · 2020-03-19T16:08:17Z

Thanks for your review. I'll address your points by their numbers:

No, it's not. I imagine the clear button being useful to mobile players. Furthermore, it exists to shrink the gargantuan search bar (nobody types more than few letters in there anyways). It's oversized currently (it's width serves no purpose).
Indeed, the height differs slightly. And that could be fixed hackily. But I don't think that's the way to go. In theory, they should have the same height (as I'm using real coordinates). I'm afraid that if I tweak this, there will be differences if it's scaled. In other words: not my job, this should be fixed in the engine.
No, I don't think that there should necessarily be free space. Having no free space makes the buttons appear grouped to the search bar as they should. See Bootstrap for examples.
Indeed. Will tweak.
Disagree. Again, grouping. Those are two separate panels.
Yes. And I'd like to keep it as is, "maximizing" the filling of the scale. I'm wondering whether there is another option, though: involving the ping, one could calculate a new "lag-score". Ping should mean less than lag, but shouldn't be meaningless. Furthermore, 100 isn't optimal, as mods etc usually increase the lag slightly above that.
I'd like to keep the grouping, again. No need for unused space.
No, it's not. The panels have enormous width (and an IMO bad width/height ratio), and my PR reduces the width for one panel (which I consider an improvement). I'm not going to stretch it artificially. There are no issues with having one panel of a different size. I plan to change the other panels as well, but that's out of the scope of this PR.
Please elaborate. The description is displayed in the box, which should be fine.

So summarized: I will address point 4 and reduce the empty space next to the icons. I will also tweak the lag indicator, addressing point 6. All other points are still to be discussed.

appgurueu · 2020-03-23T11:47:42Z

Points addressed, screenshots updated. Feedback is welcome.

The lagometer now shows a combination of lag + ping, where lag is weighted twice as much as ping.

sfan5 · 2020-03-24T23:56:20Z

builtin/fstk/tabview.lua

-					self.tablist[self.last_tab_index].tabdata,
-					self.tablist[self.last_tab_index].tabsize
-					)
+		formspec = formspec .. tab_content
 	end
 	return formspec


there is more potential in this function:

invert the if not self.hidden and (self.parent == nil or not self.parent.hidden) then condition and return nil early

store self.tablist[self.last_tab_index] in a variable since it's often used.

will do when I have time

sfan5 · 2020-03-24T23:57:32Z

builtin/mainmenu/common.lua

 	end

+	table.insert(details, ",")


since , is the field separator, shouldn't this be

table.insert(details, "") table.insert(details, "")

instead?

I originally had it this way, but it doesn't matter - because table.concat({"whatever", "", "", "whatever"}, ",") == table.concat({"whatever", ",", "whatever"}, ",") - and this way, it should be better for performance.

I know that, but for correctness I consider it better not to mix the separator into the real data just because it gets the right result.

sfan5 · 2020-03-25T00:01:04Z

src/script/lua_api/l_mainmenu.cpp

-			std::string topush = server["name"].asString();
-			lua_pushstring(L,topush.c_str());
-			lua_settable(L, top_lvl2);
+		for (const auto& string_member : {"version", "description", "name", "address", "port"})


please declare the array explicitly on a separate line (the others too)

this defeats the purpose of shortening the code; if the arrays are only used at one place, why explicitly declare them?

For readability?

sfan5 · 2020-03-25T00:10:47Z

PR structure

You need to either split refactoring/maintenance changes into a separate PR *or* squash the commits in this PR so that there is a separate commit for at least:

general improvements (removing -------------------------, changing DIR_DELIM, ...) to any file that isn't "tab_online.lua"
removing the simple main menu
improving the online tab

Design feedback

The improvements are very good overall, but I have a few points:

the clear button is useless
at the default window size the text fields are so close together that their borders touch
instead of "Discover Servers" I'd perhaps call it "Public Servers"
and it should say "Favorited Servers" (or "Server Favorites"?) too
the discover icon could use a better look

Code feedback

(see review)

builtin/mainmenu/common.lua

sfan5 · 2020-03-25T00:16:26Z

If you change the side bar, you need to then change the other tabs in the main menu.

I disagree here, it's fine for one tab to have a colored sidebar.

I have changed the code so that the tabs needn't all have the same size anymore.

Hmm, this might be an issue though. Would it be detrimental to the design to expand the servers tab to be sized consistently with the other ones?

sfan5 · 2020-03-25T00:18:47Z

builtin/mainmenu/tab_online.lua

@@ -15,7 +15,7 @@
 --with this program; if not, write to the Free Software Foundation, Inc.,
 --51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.

--------------------------------------------------------------------------------
+local server_lookup


almost forgot: why does this need to be global variable and which purpose does it serve?

server_lookup is a local variable I introduced. It basically represents the table rows. It is used while generating the table & when to identify selected entries.

rubenwardy · 2020-03-25T00:27:16Z

I disagree here, it's fine for one tab to have a colored sidebar.

It is inconsistent for one tab to add this color, and it adds nothing

sfan5 · 2020-03-25T00:36:23Z

It is inconsistent for one tab to add this color

Is it really? None of the menu tabs have matching layout, so there is barely any consistency you can take as a measure what is allowed to be different and what isn't.

it adds nothing

Increased visual appeal is definitely not "nothing"

rubenwardy · 2020-03-25T01:24:24Z

I'm in favour of adding a distinction to the sidebar - this exact same background is in my mainmenu tabs redesigns (ignore the global sidebar experiment)

I'm don't like how this is done as part of this PR, or without doing other tabs. I was under the impression that it was three tabs - this one, start Game, and content - that have full sidebars, but it's actually just content

I'm not going to oppose it in this PR, but you should at least use the correct spacings. The space between elements is the most important thing to determine how the user perceives hierarchy and grouping. There needs to be 0.375 padding and 0.25 spacing. Padding is the padding from an important container to its contents. In most cases, this is the formspec window, in this case the two important containers should be the server list and the sidebar. Spacing is the distance between not directly related elements, such as the search bar and the server table. Directly related elements should be touching, like the search bar and the search button. These values of padding and spacing are done semi automatically by the formspec code when you don't have real coordinates on. See the design I linked for an example.

Tldr: add more space been sidebar and the serverlist, use consistent space throughout. Put a gap between the refresh and search bar as they are not directly related

It would be good to document this somewhere, and create design guidelines. The values I gave for padding and spacing are arbitrary and could be tweaked - the important thing is to have different amounts of spacing to show hierarchy

Tabs need to be the same size.

None of the menu tabs have matching layout

The content tab does. The start game tab could be layouted to match it, but doesn't currently

rubenwardy · 2020-03-25T02:02:01Z

Ok I've changed my mind-go ahead with the sidebar but use consistent spacing

appgurueu · 2020-03-25T13:44:25Z

There needs to be 0.375 padding and 0.25 spacing

I've currently worked with a consistent spacing of 0.25, the sidebar however has 0.125. Will increase that to 0.25. Edit: done.

Hmm, this might be an issue though. Would it be detrimental to the design to expand the servers tab to be sized consistently with the other ones?

Yes, I think so. Forcing all tabs to have the same size doesn't work well.

Regarding PR structure, I'd like to save me the hassle of creating multiple PRs/commits. Is this possible, at least if I get my design improvements accepted?

sfan5 · 2020-03-25T15:42:42Z

Regarding PR structure, I'd like to save me the hassle of creating multiple PRs/commits. Is this possible, at least if I get my design improvements accepted?

I don't understand your question but having commits as useful "units" is a hard requirement either way.
The reason for this is if e.g. the DIR_DELIM changes cause a problem, we don't want to pick apart a huge commit that touches everything related to the menu just to revert a small portion of changes.

Depending on which of your current commits touches what, it may be sufficient to squash a few to get some organization into them.

Wuzzy2 · 2020-04-01T18:08:34Z

I have the impression many people misinterpret “Rebase needed” as “Please squash into one commit” … Note sure if I'm right.

appgurueu · 2020-04-11T21:27:14Z

Opinions needed: What about having all tabs of equals size, but not fully using up the available width (leaving free space). The free space won't be as noticeable, as it will have no background there.

Wuzzy2 · 2020-04-11T21:36:02Z

I think the current tab size is fine, i.e. the tabs are as large as they need to be. This tab design is pretty standard across the board in software UIs, as far I can tell. So, no change to the tabs is needed in your PR IMO.

The real problem with Minetest tabs IMO is that they're forced to be semi-transparent, but that's completely off-topic.

sfan5 · 2020-09-29T14:14:32Z

Any progress? I think these changes are good and would like to see then in 5.4.0.

appgurueu · 2020-12-31T12:29:22Z

Closing this. The mainmenu has to be redone entirely.

sfan5 · 2021-01-01T15:54:18Z

Who knows when that will come. I'd still prefer these improvements in the meantime.

appgurueu added 2 commits March 11, 2020 19:35

Improve serverlist

c13afed

Completely remove simple menustyle

4baef50

MoNTE48 reviewed Mar 11, 2020

View reviewed changes

builtin/mainmenu/common.lua Outdated Show resolved Hide resolved

builtin/mainmenu/common.lua Outdated Show resolved Hide resolved

builtin/mainmenu/common.lua Show resolved Hide resolved

builtin/mainmenu/common.lua Show resolved Hide resolved

appgurueu added 2 commits March 11, 2020 20:35

Fix empty table column option

729cc81

Re-add render client count

3f589ae

paramat added @ Mainmenu Maintenance Tasks to keep the codebase and related parts in order, including architectural improvements labels Mar 11, 2020

appgurueu added 3 commits March 12, 2020 14:26

Overhaul Join Game tab

5faf475

Minor layout tweaks

b8d896b

Layout tweaks; allow sending custom formspec headers in order to twea…

29102e8

…k size

MoNTE48 reviewed Mar 12, 2020

View reviewed changes

builtin/settingtypes.txt Show resolved Hide resolved

Separate Address/Port and Name/Password

957cf91

appgurueu mentioned this pull request Mar 14, 2020

Ship MTEngine with no games, choose and install from the Content Database (after main menu improvements) #9509

Closed

Add "unknown" indicator

b4f4b25

Decrease spacing; tweak lag indicator; remove unneeded code duplication

b201332

sfan5 added the Feature ✨ PRs that add or enhance a feature label Mar 24, 2020

sfan5 reviewed Mar 25, 2020

View reviewed changes

builtin/mainmenu/common.lua Show resolved Hide resolved

sfan5 reviewed Mar 25, 2020

View reviewed changes

Increase spacing, make spacing more consistent

14fe4e1

appgurueu force-pushed the master branch from aeb9b1c to 14fe4e1 Compare March 25, 2020 13:51

appgurueu changed the title ~~Mainmenu: Improve serverlist~~ Mainmenu Improvements: "Join Game" tab + code cleanup Mar 25, 2020

sfan5 added the Rebase needed The PR needs to be rebased by its author. label Mar 25, 2020

Merge branch 'master' into master

3b216ce

SmallJoker removed the Rebase needed The PR needs to be rebased by its author. label Mar 26, 2020

sfan5 added the Rebase needed The PR needs to be rebased by its author. label Sep 29, 2020

appgurueu closed this Dec 31, 2020

sfan5 mentioned this pull request Mar 17, 2021

Mainmenu: Improve "Join Game" tab #11078

Merged

1 task

Mainmenu Improvements: "Join Game" tab + code cleanup #9505

Mainmenu Improvements: "Join Game" tab + code cleanup #9505

Conversation

appgurueu commented Mar 11, 2020 • edited

Before

After

To do

How to test

paramat commented Mar 11, 2020

appgurueu commented Mar 12, 2020 • edited

rubenwardy commented Mar 12, 2020

appgurueu commented Mar 12, 2020

rubenwardy commented Mar 12, 2020

appgurueu commented Mar 12, 2020

rubenwardy commented Mar 12, 2020

appgurueu commented Mar 12, 2020

rubenwardy commented Mar 12, 2020

MoNTE48 commented Mar 12, 2020

MoNTE48 commented Mar 12, 2020

appgurueu commented Mar 12, 2020 • edited

appgurueu commented Mar 14, 2020

appgurueu commented Mar 19, 2020 • edited

appgurueu commented Mar 23, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

appgurueu Apr 1, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sfan5 Mar 25, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sfan5 commented Mar 25, 2020

PR structure

Design feedback

Code feedback

sfan5 commented Mar 25, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rubenwardy commented Mar 25, 2020

sfan5 commented Mar 25, 2020

rubenwardy commented Mar 25, 2020 • edited

rubenwardy commented Mar 25, 2020

appgurueu commented Mar 25, 2020 • edited

sfan5 commented Mar 25, 2020

Wuzzy2 commented Apr 1, 2020

appgurueu commented Apr 11, 2020

Wuzzy2 commented Apr 11, 2020

sfan5 commented Sep 29, 2020

appgurueu commented Dec 31, 2020

sfan5 commented Jan 1, 2021

appgurueu commented Mar 11, 2020 •

edited

appgurueu commented Mar 12, 2020 •

edited

appgurueu commented Mar 12, 2020 •

edited

appgurueu commented Mar 19, 2020 •

edited

appgurueu commented Mar 23, 2020 •

edited

appgurueu Apr 1, 2020 •

edited

sfan5 Mar 25, 2020 •

edited

rubenwardy commented Mar 25, 2020 •

edited

appgurueu commented Mar 25, 2020 •

edited