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

Self installation #33

Closed
wants to merge 12 commits into from
Closed

Self installation #33

wants to merge 12 commits into from

Conversation

Qubus0
Copy link
Collaborator

@Qubus0 Qubus0 commented Jan 15, 2023

to test:

  1. automatic loader setup: add this startup flag --script addons/mod_loader_tools/mod_loader_setup_helper.gd and nothing else. Start the game from the editor or from steam. I won't tell you what the things in the UI are. if something is unclear, tell me because then it needs to be changed.
    1. try out the method mentioned in the readme under For Mod Loader Developers > Clean Setup

Note: you will have to restart the game manually after first load to make the override kick in

Let me know what you think!

@ithinkandicode
Copy link
Collaborator

Compiled PCK for Brotato, hosted this myself due to GitHub's upload limits:

Two test mods, they simply append the version on the title screen.
The 2nd has the 1st as a dependency, so it won't work without it:

@ithinkandicode
Copy link
Collaborator

ithinkandicode commented Jan 16, 2023

Screenshots

Initial Setup - always shown on first load:

PR33-1

After initial setup, with no mods in ./mods. Starts in fullscreen:

PR33-2

After adding mods and restarting:

PR33-3

After disabling the fullscreen through Brotato's own options, restarting, and enabling 1 mod:

PR33-4

README.md Outdated
For detailed info, see the [docs for Delta-V Modding](https://gitlab.com/Delta-V-Modding/Mods/-/blob/main/MODDING.md), upon which ModLoader is based. The docs there cover mod setup and helper functions in much greater detail.

## Mod Setup
## Table of Contents
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@Qubus0
Copy link
Collaborator Author

Qubus0 commented Jan 16, 2023

I will decouple the interface from the self setup and the loader so the user can choose if they want to use it or an external tool/ do it manually

README.md Outdated
* [For Everyone](#for-everyone)
* [Folder locations](#folder-locations)
* [Credits](#credits)
<!-- TOC -->
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yes, completely agree. it's just something for another pr

@Qubus0 Qubus0 mentioned this pull request Jan 16, 2023
Copy link
Collaborator

@ithinkandicode ithinkandicode left a comment

Choose a reason for hiding this comment

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

The code itself looks fine. Most of my notes are for the README. There's also notes on duped code and making comments clearer. The only major issue is using a separate dir for the save files, which would disable Steam saves and has shown to be confusing for users.

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
ProjectSettings.get_setting(project_setting)


static func cmd_line_arg_exists(argument: String) -> bool:
Copy link
Collaborator

Choose a reason for hiding this comment

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

duped from mod_loader

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

there are a few dupes in here, which I would prefer to move over here as static methods.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Is mod_loader_helper.gd called before mod_loader.gd? I can understand why these methods would be duped if that's the case.

Would it be viable to make another class, eg "mod_loader_utils" or something, and have that as the top-most autoload? It could contain the duped methods for stuff like getting CLI args, probably logging too, which would have the extra benefit of making the methods in mod_loader focused entirely on just loading mods.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ModLoader is not available in the setup helper before it is, well, set up. That also means having another autoload won't fix it. I even have to use the classes directly from the script because the class isn't registered as global yet.

# mod_loader_setup_helper.gd

# IMPORTANT: use the ModLoaderHelper through this in this script! Otherwise
# script compilation will break on first load since the class is not defined
var modloaderhelper: Node = load("res://addons/mod_loader_tools/mod_loader_helper.gd").new()

var skip_mod_selection: bool = modloaderhelper.should_skip_mod_selection()

# mod_loader_helper.gd
static func should_skip_mod_selection() -> bool:
	return cmd_line_arg_exists("--skip-mod-selection") or\
		is_project_setting_true(settings.SKIP_MOD_SELECTION)

Copy link
Collaborator

@ithinkandicode ithinkandicode Jan 16, 2023

Choose a reason for hiding this comment

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

So if I understand it correctly, mod_loader_helper is a set of static utilities, is that right?

It seems like this should be a larger, separate PR: One that makes such a utility class, and moves anything that's potentially duplicatable out of mod_loader and into its utility class

I think the filenames could be shortened too. Eg "mod_loader_setup_helper" could just be called "autosetup.gd", and "mod_loader_helper" could just be "utils.gd".

# 3.x does not support ligatures (eg. "tag" being replaced by the glyph)
# so the glyph is used directly (but it's invisible in the code editor :/ )
# the name is the same as used in font awesome
# refer to https://fontawesome.com/search?o=r&m=free&s=solid&f=classic
Copy link
Collaborator

Choose a reason for hiding this comment

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

Having things that are invisible in the editor isn't really desirable as it can easily cause issues

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I agree. the only way to make them visible is to change the editor font though

Copy link
Collaborator

Choose a reason for hiding this comment

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

Contributors may be editing these files in Godot, rather than an external editor, so I think making things invisible could be a point of impasse here. Is there an alternative implementation to whatever FontAwesome is used for here?

# rename application to make it clear to the user that they are playing modded
# godot also creates a new user:// folder for logs (and other things using user:// for saves etc.)
var original_name: String = ProjectSettings.get_setting(modloaderhelper.settings.APPLICATION_NAME)
ProjectSettings.set_setting(modloaderhelper.settings.APPLICATION_NAME, original_name + " (Modded)")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Definitely not a fan of this, users will think their progress has been wiped (as they do for Brotato mods like Assassin, which disables Steam). It also disables Steam saves, which we shouldn't do. Using mods shouldn't affect how the game loads your save file.

It's a good idea in theory, but we've seen how this plays out in practice with Brotato.

@ithinkandicode
Copy link
Collaborator

ithinkandicode commented Jan 16, 2023

Notes on the GUI itself

Moved to PR #36

README.md Outdated Show resolved Hide resolved
@Qubus0
Copy link
Collaborator Author

Qubus0 commented Jan 16, 2023

@ithinkandicode we should merge your pr first, then adjust and merge this one on top.

let's keep the whole readme out of this and reconsolidate it into a proper wiki with separate pages for everything. I have created an issue for that where we can discuss everything: #35. I'll resolve all the comments about the readme here so we can focus on the code
What we do with the Interface branch is also a thing we can decide after that

@Qubus0 Qubus0 changed the title Self installation and UI Self installation Jan 16, 2023
@ithinkandicode
Copy link
Collaborator

@Qubus0 I've copied over the content from the README in this PR to a temporary wiki page:

https://github.com/GodotModding/godot-mod-loader/wiki/@TEMP:-PR33

This ensures we keep your new content safe, which should help you fix any merge conflicts with the newer README from the main branch.

@ithinkandicode
Copy link
Collaborator

So I think that what's being done in this PR would benefit from being split into smaller PRs. Atm you have:

  1. The auto setup stuff (setup_helper)
  2. A utility file, with some duped stuff from mod_loader.gd (helper)
  3. A mod class, with its own validation (mod_details)
  4. Code that adds support for the GUI in #36

I think you could split this into four PRs, one for each of the above

@ithinkandicode
Copy link
Collaborator

RE: override.cfg is used: What happens when the game updates? It looks like the override.cfg that got created when I ran the setup for Brotato includes all the vanilla stuff. Can you use an override.cfg that only includes certain settings, or does it have to be the entire project?

If it has to be the entire project's configs, perhaps could you check to see if the game has been updated since override.cfg was created? Eg. you could create a backup of the vanilla override.cfg during the initial setup phase, then you could re-regenerate it from vanilla again later and check its checksum. That'd show you if anything's been changed in vanilla

(note: I think I said this elsewhere but can't find it)

@Qubus0
Copy link
Collaborator Author

Qubus0 commented Jan 17, 2023

oh yep good idea. the interface only covers manual resets so far

@Qubus0
Copy link
Collaborator Author

Qubus0 commented Jan 17, 2023

reworking this into multiple new prs

@Qubus0 Qubus0 closed this Jan 17, 2023
@ithinkandicode
Copy link
Collaborator

oh yep good idea. the interface only covers manual resets so far

Cool. I mentioned saving a copy of the vanilla override.cfg, just noting here that it might be best to save it in user:// so it doesn't bloat the game dir too much.

@Qubus0 Qubus0 deleted the self_installation_and_ui branch January 19, 2023 22:50
@Qubus0 Qubus0 mentioned this pull request Jan 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants