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

Rework escape/pause menu #5719

Merged
merged 7 commits into from May 11, 2017

Conversation

Projects
None yet
10 participants
@red-001
Copy link
Contributor

commented May 7, 2017

Changes

  • Remove build information
  • Use current controls instead of default controls
  • Add information about the current server in place of the build information
  • Add text saying the game is paused to if in singleplayer mode.
    rework pause/escape menu

see also #3845

Screenshots

Hosting server
screenshot_20170507_172346
Singleplayer
screenshot_20170507_172420
Connected to server
screenshot_20170507_172502

@SmallJoker

This comment has been minimized.

Copy link
Member

commented May 7, 2017

Rather than using different sentences for each mode, it should be made consistent with the other "parameters", like PvP and creative: Play mode: [Singleplayer / Hosting server / Remote server]
Also, how about Public server: [On / Off] ?

@lisacvuk

This comment has been minimized.

Copy link
Contributor

commented May 7, 2017

Where can I find build info, then? That was quite useful.

@SmallJoker

This comment has been minimized.

Copy link
Member

commented May 7, 2017

@lisacvuk minetest(server) --version and you'll have the same information.
Of course, it's easier to read it directly from the GUI but running the command will allow you to copy the text ;)

Rework escape/pause menu
- Remove build information
- Use current controls instead of default controls
- Add information about the current server in place of the build information
- Add text saying the game is paused to if in singleplayer mode.
rework pause/escape menu
@lisacvuk

This comment has been minimized.

Copy link
Contributor

commented May 7, 2017

Maybe we could keep the old info, and add the new one?

@Fixer-007

This comment has been minimized.

Copy link
Contributor

commented May 7, 2017

Do we really need "Exit to OS" button?

@red-001

This comment has been minimized.

Copy link
Contributor Author

commented May 7, 2017

@Fixer-007 fullscreen mode?

@red-001 red-001 force-pushed the red-001:rework_escape_menu branch from 172e858 to 30e4a58 May 7, 2017

@red-001 red-001 force-pushed the red-001:rework_escape_menu branch from 30e4a58 to 513089c May 7, 2017

@red-001

This comment has been minimized.

Copy link
Contributor Author

commented May 7, 2017

@red-001

This comment has been minimized.

Copy link
Contributor Author

commented May 7, 2017

New screenshots

Hosting

screenshot_20170507_213259

Singleplayer

screenshot_20170507_213314

Connected to server

screenshot_20170507_213431

@GreenXenith

This comment has been minimized.

Copy link
Contributor

commented May 7, 2017

Do we really need "Exit to OS" button?

Um, yes. I use it all the time when I want to close the game immediately. It is helpful for modders when testing and they are opening/closing the game over and over to update code.

@nerzhul

This comment has been minimized.

Copy link
Member

commented May 8, 2017

For me Game pauses should be shown outside of the main menu as a h1 label in the center of the area over the formspec, it's a common choice :)

Very very nice changes, gg
Exit to OS is important yes, keep it

#endif

float ypos = simple_singleplayer_mode ? 0.5 : 0.1;
float ypos = simple_singleplayer_mode ? 0.7 : 0.1;

This comment has been minimized.

Copy link
@nerzhul

nerzhul May 8, 2017

Member

don't forget the f for floats

<< "textarea[0.4,0.25;3.9,6.25;;" << PROJECT_NAME_C " " VERSION_STRING "\n"
<< "\n"
<< strgettext("Game info:") << "\n";
std::string address = client->getAddressName();

This comment has been minimized.

Copy link
@nerzhul

nerzhul May 8, 2017

Member

const address &

const static std::string mode = strgettext("- Mode: ");
if (!simple_singleplayer_mode) {
Address serverAddress = client->getServerAddress();
if(address != "") {

This comment has been minimized.

Copy link
@nerzhul

nerzhul May 8, 2017

Member

missing space

os << mode << strgettext("Singleplayer") << "\n";
}
if (simple_singleplayer_mode || address == "") {
const static std::string on = strgettext("On");

This comment has been minimized.

Copy link
@nerzhul

nerzhul May 8, 2017

Member

static const instead of const static

os << mode << strgettext("Remote server") << "\n"
<< strgettext("- Address: ") << address;
} else {
os << mode << strgettext("Hosting Server");

This comment has been minimized.

Copy link
@SmallJoker

SmallJoker May 8, 2017

Member

Hosting server (consistent remote server)

os << strgettext("- PvP: ") << pvp << "\n"
<< strgettext("- Public: ") << announced << "\n";
std::string server_name = g_settings->get("server_name");
if(announced == on && server_name != "")

This comment has been minimized.

Copy link
@SmallJoker

SmallJoker May 8, 2017

Member

Missing space

<< strgettext("- Public: ") << announced << "\n";
std::string server_name = g_settings->get("server_name");
if(announced == on && server_name != "")
os << strgettext("- Server Name: ") << server_name;

This comment has been minimized.

Copy link
@SmallJoker

SmallJoker May 8, 2017

Member

The missing space above can be taken from here.

);

char control_text[500];

This comment has been minimized.

Copy link
@nerzhul

nerzhul May 9, 2017

Member

this memory area is not properly initialized with zeros

This comment has been minimized.

Copy link
@Zeno-

Zeno- May 9, 2017

Contributor

Is that a problem? It shouldn't be a problem at all because the snprintf() takes care of terminating things properly, or am I missing something?

This comment has been minimized.

Copy link
@nerzhul

nerzhul May 9, 2017

Member

initialized is not portable, okay, but i can cause problem on BSD systems, also nobody use BSD as desktop for minetest, okay then

@red-001 red-001 force-pushed the red-001:rework_escape_menu branch from 9acbf31 to 6493146 May 9, 2017

@red-001 red-001 force-pushed the red-001:rework_escape_menu branch from 6493146 to f4726ea May 9, 2017

@red-001

This comment has been minimized.

Copy link
Contributor Author

commented May 9, 2017

new location of game paused message

screenshot_20170509_184613

@nerzhul

nerzhul approved these changes May 9, 2017

if (simple_singleplayer_mode || address == "") {
static const std::string on = strgettext("On");
static const std::string off = strgettext("Off");
std::string damage = g_settings->getBool("enable_damage") ? on : off;

This comment has been minimized.

Copy link
@nerzhul

nerzhul May 9, 2017

Member

is this possible to use const ref for the 5 std::string there or compiler doesn't like it ?

@nerzhul

nerzhul approved these changes May 9, 2017

Copy link
Member

left a comment

Nice job !

@SmallJoker

This comment has been minimized.

Copy link
Member

commented May 9, 2017

👍 from me when Travis doesn't tell something else.

char control_text[500];

snprintf(control_text, 500, control_text_template.c_str(),
GET_KEY_NAME(keymap_forward),

This comment has been minimized.

Copy link
@rubenwardy

rubenwardy May 9, 2017

Member

what happens if the key is unbound?

This comment has been minimized.

Copy link
@red-001

red-001 May 9, 2017

Author Contributor

it will just look a bit strange as instead of a key there will be an empty string. That's an edge case anyway that would require the user to manually unbind the key in minetest.conf

@@ -4545,8 +4561,11 @@ void Game::showPauseMenu()
if (!simple_singleplayer_mode) {
os << "button_exit[4," << (ypos++) << ";3,0.5;btn_change_password;"
<< strgettext("Change Password") << "]";
} else {
os << "textarea[4.95,0;3.5,6;;" << strgettext("Game Paused") << ";]";

This comment has been minimized.

Copy link
@rubenwardy

rubenwardy May 9, 2017

Member

Why isn't this a label?

This comment has been minimized.

Copy link
@red-001

red-001 May 9, 2017

Author Contributor

it's not a setting just the current state of the game

This comment has been minimized.

Copy link
@rubenwardy

rubenwardy May 9, 2017

Member

textareas are multiline text fields though, unless I'm mistaken.

os << mode << strgettext("Singleplayer") << "\n";
}
if (simple_singleplayer_mode || address == "") {
static const std::string on = strgettext("On");

This comment has been minimized.

Copy link
@rubenwardy

rubenwardy May 9, 2017

Member

please use true and false.
on and off aren't booleans according to is_yes

This comment has been minimized.

Copy link
@red-001

red-001 May 9, 2017

Author Contributor

This is just for displaying information to the player. It makes more sense to use on and off for this usecase imo.

This comment has been minimized.

Copy link
@rubenwardy

rubenwardy May 9, 2017

Member

fair enough, doesn't matter too much to me

This comment has been minimized.

Copy link
@nerzhul

nerzhul May 9, 2017

Member

It's the text to show on screen, not a boolean

@Ekdohibs
Copy link
Member

left a comment

Concept looks good, there are at least a few formspec injection issues.

@@ -4545,8 +4561,11 @@ void Game::showPauseMenu()
if (!simple_singleplayer_mode) {
os << "button_exit[4," << (ypos++) << ";3,0.5;btn_change_password;"
<< strgettext("Change Password") << "]";
} else {
os << "textarea[4.95,0;3.5,6;;" << strgettext("Game Paused") << ";]";

This comment has been minimized.

Copy link
@Ekdohibs

Ekdohibs May 9, 2017

Member

Don't we have a version of strgettext for formspecs (or is that only for Lua?)? If we do, it would be best to use it instead, as it could break the formspec if for some reason special characters are included in the translation (I'm mainly thinking about , and ; here).

This comment has been minimized.

Copy link
@red-001

red-001 May 9, 2017

Author Contributor

it's lua only afaik

char control_text[500];

snprintf(control_text, 500, control_text_template.c_str(),
GET_KEY_NAME(keymap_forward),

This comment has been minimized.

Copy link
@Ekdohibs

Ekdohibs May 9, 2017

Member

So what happens if I bind the ; key?

This comment has been minimized.

Copy link
@red-001

red-001 May 9, 2017

Author Contributor

That would be displayed as OEM 6 (it's consistent with the key change menu). not sure what would happen with other formspec special characters. Will look into that.
Edit: OEM 1 not OEM 6

@nerzhul

This comment has been minimized.

Copy link
Member

commented May 9, 2017

waiting for last fixes/comments

@red-001 red-001 force-pushed the red-001:rework_escape_menu branch from feab89e to 9847e6a May 9, 2017

@red-001 red-001 force-pushed the red-001:rework_escape_menu branch from 9847e6a to f60a40d May 9, 2017

@nerzhul

This comment has been minimized.

Copy link
Member

commented May 10, 2017

@rubenwardy @Ekdohibs please review another time

@Ekdohibs

This comment has been minimized.

Copy link
Member

commented May 10, 2017

It seems to me that using gettext for key names is quite problematic: key names won't be added to translation files automatically, and there still is the formspec injection problem if keys such as ";" are mapped with gettext disabled (or did I misread something?)

@red-001

This comment has been minimized.

Copy link
Contributor Author

commented May 10, 2017

I reused the method that the key change menu used to translate key names so it should work correctly.

Formspec escape is not done by gettext so gettext being enabled or disabled wouldn't make a difference.

@nerzhul nerzhul dismissed stale reviews from Ekdohibs and rubenwardy May 10, 2017

answer done

@benrob0329

This comment has been minimized.

Copy link

commented May 10, 2017

+1


char control_text_buf[500];

snprintf(control_text_buf, 500, control_text_template.c_str(),

This comment has been minimized.

Copy link
@Zeno-

Zeno- May 11, 2017

Contributor

I'd prefer the 500 here be replaced with ARRLEN (see basic_macros.h)
Edit: if not ARRLEN then sizeof control_text_buf, but the macro is there so may as well use it.

@nerzhul nerzhul merged commit 0e0c824 into minetest:master May 11, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@nerzhul

This comment has been minimized.

Copy link
Member

commented May 11, 2017

approved by @Zeno- on IRC this morning

@red-001 red-001 deleted the red-001:rework_escape_menu branch May 12, 2017

@red-001 red-001 restored the red-001:rework_escape_menu branch May 12, 2017

@red-001 red-001 deleted the red-001:rework_escape_menu branch May 20, 2017

@red-001 red-001 restored the red-001:rework_escape_menu branch May 20, 2017

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