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

Debundle Minetest Game #13818

Merged
merged 3 commits into from Nov 7, 2023
Merged

Debundle Minetest Game #13818

merged 3 commits into from Nov 7, 2023

Conversation

rollerozxa
Copy link
Member

@rollerozxa rollerozxa commented Sep 17, 2023

Closes #9509.

This PR debundles Minetest Game from the engine. The necessary preparations for improving UX with no game installed has already been resolved with #13550.

Requires:

To do

This PR is Ready for Review.

How to test

See that there are no mentions of it left in READMEs or buildscripts or such.

@grorp grorp added @ Build CMake, build scripts, official builds, compiler and linker errors Feature ✨ PRs that add or enhance a feature @ Content / PkgMgr labels Sep 17, 2023
@SmallJoker SmallJoker added this to the 5.8.0 milestone Sep 17, 2023
@sfan5
Copy link
Member

sfan5 commented Sep 17, 2023

Note that merging this will make #13800 (and perhaps #13799) a blocker, is there a prospect of anyone working on it soon?

@rubenwardy
Copy link
Member

I suggest making those two things prerequisites of this PR

@rubenwardy rubenwardy added the Roadmap The change matches an item on the current roadmap. label Sep 17, 2023
@SmallJoker SmallJoker added the Waiting (on dependency) Waiting on another PR or external circumstances (not for rebases/changes requested) label Sep 17, 2023
Copy link
Member

@grorp grorp left a comment

Choose a reason for hiding this comment

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

I think games should be removed from here:

public static boolean isInstallValid(@NonNull Context context) {
File userDataDirectory = getUserDataDirectory(context);
return userDataDirectory.isDirectory() &&
new File(userDataDirectory, "games").isDirectory() &&
new File(userDataDirectory, "builtin").isDirectory() &&
new File(userDataDirectory, "client").isDirectory() &&
new File(userDataDirectory, "textures").isDirectory();
}

@rubenwardy
Copy link
Member

rubenwardy commented Oct 1, 2023

Remaining things before debundling:

  1. Android needs to uninstall MTG when updating
  2. Handle the case where MTG is uninstalled with MTG worlds. This is Migration path once MTG is no longer preinstalled #13800
  3. Handle the case where an old version remains installed. This would be the case on Windows if the user updates by extracting the zip over an existing install. Update detection only works when installed from CDB, so users won't receive update notifications in this case

@rollerozxa
Copy link
Member Author

I think games should be removed from here:

yep, otherwise it will repeatedly extract the zip on android if you don't have any games installed...

@sfan5
Copy link
Member

sfan5 commented Oct 8, 2023

3. This would be the case on Windows if the user updates by extracting the zip over an existing install.

Hopefully users don't actually do this because there's a bunch of things that could break in more or less subtle ways if done.

@rollerozxa
Copy link
Member Author

  1. This would be the case on Windows if the user updates by extracting the zip over an existing install.

Hopefully users don't actually do this because there's a bunch of things that could break in more or less subtle ways if done.

This was how I would update Minetest on Windows, by extracting bin and builtin over my existing Minetest install. That's what happens when user and share data are all mixed up in a mess like the Windows builds currently are.

@rollerozxa
Copy link
Member Author

For cases where #13800 wouldn't be able to detect updating from an existing install containing MTG worlds, I feel like it should be important to inform users via Minetest community channels that MTG will be debundled in 5.8.0, and that if they want to continue playing with MTG it can be reinstalled from the content browser. (like this forum thread I made about it)

@SmallJoker SmallJoker removed the Waiting (on dependency) Waiting on another PR or external circumstances (not for rebases/changes requested) label Oct 15, 2023
@SmallJoker
Copy link
Member

Removed the label because #13799 is not too important and by the looks of it, the other dependencies were resolved too.

Relevant discussion: https://irc.minetest.net/minetest-dev/2023-10-15#i_6122535

@rubenwardy
Copy link
Member

Still need to uninstall mtg when the Android app updates

@grorp
Copy link
Member

grorp commented Oct 21, 2023

Still need to uninstall mtg when the Android app updates

Since #13906 also works on Android, merging that PR would change this to-do from a hard requirement to a nice-to-have.

@rubenwardy rubenwardy modified the milestones: 5.8.0, 5.9.0 Oct 27, 2023
Copy link
Member

@grorp grorp left a comment

Choose a reason for hiding this comment

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

Found a leftover mention of MTG:

Look for examples in `games/devtest` or `games/minetest_game`.

I think we can just remove that line altogether.

Copy link
Member

@rubenwardy rubenwardy left a comment

Choose a reason for hiding this comment

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

Works

@rubenwardy
Copy link
Member

Found a leftover mention of MTG:

Look for examples in `games/devtest` or `games/minetest_game`.

I think we can just remove that line altogether.

I noticed this but felt like it didn't matter either way

@srifqi
Copy link
Member

srifqi commented Nov 6, 2023

If we want to truly remove any references to Minetest Game,

* `title`: Required, a human-readable title to address the game, e.g. `title = Minetest Game`.

Any other PNG files will be interpreted as textures. They must have the same
names as the textures they are supposed to override. For example, to override
the apple texture of Minetest Game, add a PNG file named `default_apple.png`.

minetest.chat_send_player(player:get_player_name(), "This is the \"Development Test\" [devtest], meant only for testing and development. Use Minetest Game for the real thing.")

@rollerozxa
Copy link
Member Author

rollerozxa commented Nov 6, 2023

Referencing Minetest Game for examples in documentation sounds okay to me, but that devtest initial message could use being changed. Or even removed, since devtest isn't bundled with builds of the engine anymore and there are already warnings when creating a devtest world pointing you to the content browser, for if you're running in place inside of the source tree.

@grorp
Copy link
Member

grorp commented Nov 6, 2023

I noticed this but felt like it didn't matter either way

Referencing Minetest Game for examples in documentation sounds okay to me

👍 You're right, it doesn't matter.

However, I think the Devtest chat message still has value, as an additonal reminder. Removing the second sentence would be enough IMO.

Copy link
Member

@srifqi srifqi left a comment

Choose a reason for hiding this comment

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

Except other references about Minetest Game (discussed before), this PR looks good to me.

PS The unpacking/unzipping in the Android version is faster, too, without MTG.

@rollerozxa
Copy link
Member Author

rollerozxa commented Nov 6, 2023

However, I think the Devtest chat message still has value, as an additonal reminder. Removing the second sentence would be enough IMO.

Alright, I have done that, should be enough for now.


Merge time? ^_^

Copy link
Member

@grorp grorp left a comment

Choose a reason for hiding this comment

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

Let's do this! :)

@grorp grorp merged commit 570fc90 into minetest:master Nov 7, 2023
15 checks passed
cx384 pushed a commit to cx384/minetest that referenced this pull request Dec 9, 2023
kawogi pushed a commit to kawogi/minetest that referenced this pull request Dec 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
@ Build CMake, build scripts, official builds, compiler and linker errors @ Content / PkgMgr Feature ✨ PRs that add or enhance a feature 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.

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