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

Improve UX when no game exists and drop default_game #13550

Merged
merged 9 commits into from Sep 17, 2023

Conversation

rollerozxa
Copy link
Member

@rollerozxa rollerozxa commented Jun 1, 2023

Prerequisite of #9509

This PR improves the UX when no game exists. If no game exists, the local tab will now point you directly at the content browser when Minetest is started (albeit with the ability to still play in multiplayer of course):

image

This PR is a prerequisite of debundling MTG altogether, which has been split off and will be in another PR. Merging this PR in itself without also debundling MTG is safe, in case there is a delay in merging the MTG debundling PR.

In addition to this, the engine's behaviour when MTG doesn't exist and/or games are removed has been improved, picking the first available game (excluding devtest, for if it is on a RUN_IN_PLACE build running from source) rather than being in a weird no game void where all worlds are visible.

This also means that the default_game setting has been removed, as it will automatically pick the first non-devtest game. When running a dedicated server, it will print an error message pointing you to content.minetest.net to download a game to run your server with, rather than attempting to create a Minetest Game world.

This PR also fixes an issue where the games list doesn't get updated in the content tab, which is especially annoying when installing games via the "You have no games installed" message.

To do

This PR is Ready for Review.

How to test

  1. Start out with no games installed, so you see the "no games" screen.
  2. Install Minetest Game and/or some other games (at least a couple).
  3. Go back and see so that a game has been automatically selected.
  4. Go into the content tab and uninstall the game that got selected.
  5. Go back to the local tab and see that it has selected another game, with the game theming applied and such.

Probably some more edge cases need to be tested.

@sfan5 sfan5 added Roadmap The change matches an item on the current roadmap. Feature ✨ PRs that add or enhance a feature @ Mainmenu labels Jun 1, 2023
@tenplus1
Copy link
Contributor

tenplus1 commented Jun 2, 2023

So long as any mods that depend on 'default' can actually download a game that supplies all dependencies of default then it's ok, otherwise you're in for compatibility hell.

@rollerozxa
Copy link
Member Author

So long as any mods that depend on 'default' can actually download a game that supplies all dependencies of default then it's ok, otherwise you're in for compatibility hell.

Yes, that game would be called Minetest Game.

@rollerozxa
Copy link
Member Author

rollerozxa commented Jun 2, 2023

Since Minetest Game is still wanted by many, it is available at the top and highlighted. Currently it's hardcoded, but maybe there should be more highlighted games, fetched from ContentDB?

ContentDB now has the ability to hoist packages to the top of the content browser, independently of featured packages on the main web page, so it wouldn't be necessary to hardcode this anymore.

@rollerozxa
Copy link
Member Author

rollerozxa commented Jun 25, 2023

ContentDB now has the ability to hoist packages to the top of the content browser, independently of featured packages on the main web page, so it wouldn't be necessary to hardcode this anymore.

Done. ContentDB now hoists Minetest Game and some other featured games to the top of the content browser. Some kind of highlighting of featured packages can come later I guess.

image

@sfan5
Copy link
Member

sfan5 commented Jun 26, 2023

Unsolved issue: What happens when the user upgrades Minetest? minetest_game will disappear and they suddenly won't see any of their worlds anymore. (*)
We need an obvious UI flow with as few steps as possible to allow the user to download MTG so they can get back to their worlds.

Another, related issue: The user now has MTG installed locally. What happens in the future when they update the engine and MTG is still at a version that is not compatible?
There needs to be an ideally offline way of detecting this (game.conf?) and another very obvious flow that leads the user to update MTG before they open their worlds.
(Identically applies to any other game with strict version compat)

(*): Dropping the user into CDB is not sufficient, some people might not even know that they need to download something called Minetest Game to continue playing "the game that was always there".

@sfan5 sfan5 added the Action / change needed Code still needs changes (PR) / more information requested (Issues) label Jun 26, 2023
@rubenwardy
Copy link
Member

Improving the UX and debundling MTG should be two different PRs / commits. And yeah, we need a way to handle the different cases of migrating from system wide MTG to CDB MTG

@sfan5
Copy link
Member

sfan5 commented Jun 26, 2023

It can be in a different PR sure, it just immediately becomes a release blocker as soon as this is merged. (feel free to remove the label)
Calling it "Improving UX" is a bit misleading in that regard because this isn't a nice-to-have, it's a requirement.

@rollerozxa
Copy link
Member Author

Should I rebase this into two commits that get merged without squash?


For migrating from a system-wide MTG to the CDB version, would it be fine to do something like this:

  • Check some temporary setting has_notified_about_mtg, when it's false check if there's any worlds that use MTG
  • If haven't been notified and there's MTG worlds, display a dialog "Minetest Game is no longer bundled with Minetest. If you would like to continue playing Minetest Game, it can be downloaded in the content browser [Go to content browser] [Dismiss]"
  • Either button on the dialog sets has_notified_about_mtg to true
    New installations won't come with any worlds, and as such won't show the dialog, so it will only come up for players upgrading.

In regards to MTG version being desync with engine version, there's already a check in MTG for if certain engine features exist: https://github.com/minetest/minetest_game/blob/master/mods/default/init.lua#L15 But maybe the error message should be improved.

@rubenwardy
Copy link
Member

Should I rebase this into two commits that get merged without squash?

Yes please


For migrating from a system-wide MTG to the CDB version, would it be fine to do something like this:

* Check some temporary setting `has_notified_about_mtg`, when it's false check if there's any worlds that use MTG

* If haven't been notified and there's MTG worlds, display a dialog "Minetest Game is no longer bundled with Minetest. If you would like to continue playing Minetest Game, it can be downloaded in the content browser [Go to content browser] [Dismiss]"

* Either button on the dialog sets `has_notified_about_mtg` to true
  New installations won't come with any worlds, and as such won't show the dialog, so it will only come up for players upgrading.

In regards to MTG version being desync with engine version, there's already a check in MTG for if certain engine features exist: minetest/minetest_game@master/mods/default/init.lua#L15 But maybe the error message should be improved.

That makes sense when the package manager is clever enough to remove Minetest Game. But it's possible that some may just leave it at the older version - I believe this happens on Android, and it also likely if the user is updating by extracting a .zip file. So maybe the engine should detect an outdated MTG version and offer to update it idk

@rubenwardy
Copy link
Member

rubenwardy commented Aug 4, 2023

It can be in a different PR sure, it just immediately becomes a release blocker as soon as this is merged. (feel free to remove the label) Calling it "Improving UX" is a bit misleading in that regard because this isn't a nice-to-have, it's a requirement.

I was thinking that we could make 1 or more PRs to improve the UX and then debundle MTG. But if we're happy with the UX here, we can do it in one - I'd still have the commits separated though

@rubenwardy
Copy link
Member

README.md:15:

If you downloaded the Minetest Engine source code in which this file is
contained, you probably want to download the Minetest Game
project too. See its README.txt for more information.

This should say to download a game, linking to ContentDB's page

@rollerozxa rollerozxa force-pushed the debundle-mtg branch 2 times, most recently from 062a644 to 5a1fd4b Compare August 4, 2023 15:22
@rollerozxa
Copy link
Member Author

rollerozxa commented Aug 4, 2023

I've rebased the PR to only include the UX improvements and dropping default_game (it gets selected automatically, selecting Minetest Game if that is installed). This should be safe to merge without Minetest Game being debundled (that'll be for another PR).

@rollerozxa rollerozxa changed the title Debundle Minetest Game and improve UX when no game exists Improve UX when no game exists and drop default_game Aug 4, 2023
builtin/mainmenu/dlg_create_world.lua Outdated Show resolved Hide resolved
builtin/mainmenu/tab_content.lua Outdated Show resolved Hide resolved
@rubenwardy rubenwardy added this to the 5.8.0 milestone Aug 29, 2023
@rubenwardy rubenwardy removed the Action / change needed Code still needs changes (PR) / more information requested (Issues) label Aug 29, 2023
@rubenwardy
Copy link
Member

rubenwardy commented Aug 29, 2023

Looks good apart from the following

Co-authored-by: rubenwardy <rw@rubenwardy.com>
@rubenwardy rubenwardy added One approval ✅ ◻️ and removed Action / change needed Code still needs changes (PR) / more information requested (Issues) labels Aug 29, 2023
src/main.cpp Outdated Show resolved Hide resolved
src/main.cpp Outdated Show resolved Hide resolved
builtin/mainmenu/dlg_create_world.lua Outdated Show resolved Hide resolved
builtin/mainmenu/tab_local.lua Outdated Show resolved Hide resolved
@rubenwardy rubenwardy added the Action / change needed Code still needs changes (PR) / more information requested (Issues) label Aug 30, 2023
@ancientmarinerdev
Copy link

Another, related issue: The user now has MTG installed locally. What happens in the future when they update the engine and MTG is still at a version that is not compatible? There needs to be an ideally offline way of detecting this (game.conf?) and another very obvious flow that leads the user to update MTG before they open their worlds. (Identically applies to any other game with strict version compat)

100% agree this is important and has been an issue for all games and all mods for a long time. It's a little frustrating that this is only seen as an issue when MTG cannot utilise it's privileged position to be packaged with updates. Many games and mods with breakages have led to a bad user experience for a long time and game teams having to support diagnose and highlight that it was a bug that was already addressed and released on contentDB and they just needed to be telepathically aware that an update is ready. Not all players will raise an issue when something breaks, they just won't play, and they'll play something else. Might create a bad review on the playstore, possibly. Automatic version recognition and update notification is a key feature for any gaming platform (and many that aren't).

@grorp grorp removed the Action / change needed Code still needs changes (PR) / more information requested (Issues) label Sep 5, 2023
src/main.cpp Show resolved Hide resolved
Copy link
Member

@sfan5 sfan5 left a comment

Choose a reason for hiding this comment

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

seems good otherwise

@grorp grorp added the Action / change needed Code still needs changes (PR) / more information requested (Issues) label Sep 6, 2023
@Zughy Zughy removed the Action / change needed Code still needs changes (PR) / more information requested (Issues) label Sep 12, 2023
Copy link
Member

@sfan5 sfan5 left a comment

Choose a reason for hiding this comment

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

@rubenwardy rubenwardy merged commit a88e61c into minetest:master Sep 17, 2023
15 checks passed
kawogi pushed a commit to kawogi/minetest that referenced this pull request Dec 19, 2023
cosin15 pushed a commit to cosin15/minetest that referenced this pull request Feb 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature ✨ PRs that add or enhance a feature @ Mainmenu Roadmap The change matches an item on the current roadmap. >= Two approvals ✅ ✅
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants