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

Setup: automatically generate Setup Data for SNIClient games #1262

Closed
wants to merge 9 commits into from

Conversation

Berserker66
Copy link
Member

@Berserker66 Berserker66 commented Nov 27, 2022

What is this fixing or adding?

automatically generates Registry bindings for SNIClient games
Moves Romfile name and checksum to a single place in setup.py, rather than multiple in iss
Prevents pressing Next on SNI games if the currently selected ROM does not exist or does not pass checksum tests, preventing users from ignoring warnings and installing AP in a broken state. Also brings up an explanation message.
Less duplicated code, however the singular code is now a twisted mess of Python string interpolation and pascal, rather than copy-pasted pascal.

Note: ExtraDiskSpaceRequired flag is removed, however I'm fairly sure most of it was false anyway and simply copy-pasted without anyone actually calculating it properly.

How was this tested?

A few times as part of dev. My Registry seems to still be intact, so that's a good sign.

If this makes graphical changes, please attach screenshots.

@Berserker66 Berserker66 changed the title Setup: automatically generate Registry bindings for SNIClient games Setup: automatically generate Setup Data for SNIClient games Jan 6, 2023
Comment on lines +265 to +266
# did not find a good source to automatically pull these
default_rom_file_names: typing.Dict[str, str] = {
Copy link
Member

Choose a reason for hiding this comment

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

Should we ever get "AutoWorldSettings", this could pull from there. Until then Utils.get_default_options(), however we still need a dict "A Link to the Past" -> "lttp_options" here (which is cleaner than putting the rom name here imo)

}

snes_md5s: typing.Dict[str, str] = {
"A Link to the Past": "03a63945398191337e896e5771f77173",
Copy link
Member

Choose a reason for hiding this comment

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

isn't the hash available through AutoPatch? if not, this should be made available through that, instead of listing the hashes here?

Copy link
Collaborator

Choose a reason for hiding this comment

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

{game: patch_type.hash for game, patch_type in AutoPatchRegister.patch_types.items() if game in default_rom_file_names}

Comment on lines +286 to +287
with open("generator_components.iss", "w") as f_gen:
with open("sni_components.iss", "w") as f_sni:
Copy link
Member

Choose a reason for hiding this comment

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

Code below looks like jinja may be the better choice?

}

snes_md5s: typing.Dict[str, str] = {
"A Link to the Past": "03a63945398191337e896e5771f77173",
Copy link
Collaborator

Choose a reason for hiding this comment

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

{game: patch_type.hash for game, patch_type in AutoPatchRegister.patch_types.items() if game in default_rom_file_names}

patch_types: ClassVar[Dict[str, AutoPatchRegister]] = {}
file_endings: ClassVar[Dict[str, AutoPatchRegister]] = {}
patch_types: ClassVar[Dict[str, APDeltaPatch]] = {}
file_endings: ClassVar[Dict[str, APDeltaPatch]] = {}
Copy link
Collaborator

Choose a reason for hiding this comment

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

The previous annotations were correct. the patch types are instances of the type AutoPatchRegister, not APDeltaPatch.

An acceptable annotation in practice might be Type[APDeltaPatch].
(Although the IDE will still complain when inserting new_class into these dicts, since it doesn't know that the only instances of AutoPatchRegister that are created in practice are APDeltaPatch and its subclasses.)

with open("sni_components.iss", "w") as f_sni:

for game_name in AutoSNIClientRegister.game_handlers:
patch_handler: APDeltaPatch = AutoPatchRegister.patch_types[game_name]
Copy link
Collaborator

Choose a reason for hiding this comment

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

patch_handler is not of type APDeltaPatch; see comment in "Files.py".

/generator_components.iss
/files.iss
/sni_components.iss
/code.iss
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe these generated files could all be placed in a single subdirectory? (Maybe even the already used buildfolder?)
(Provided Inno Setup supports including from subfolders.)

with open("generator_components.iss", "w") as f_gen:
with open("sni_components.iss", "w") as f_sni:

for game_name in AutoSNIClientRegister.game_handlers:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Looping over it like this means that a ROM setup option will be created for SMZ3 (which did not exist before, and does nothing useful now), but it does not create one for SoE (which would be required in order to pick a SoE ROM.).

from worlds.AutoSNIClient import AutoSNIClientRegister
from worlds.Files import AutoPatchRegister, APDeltaPatch
with open("generator_components.iss", "w") as f_gen:
with open("sni_components.iss", "w") as f_sni:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not really important, but there is no need for the extra nesting level; multiple contexts can be entered in a single with statement.

Name: "client/sni/dkc3"; Description: "SNI Client - Donkey Kong Country 3 Patch Setup"; Types: full playing; Flags: disablenouninstallwarning
Name: "client/sni/smw"; Description: "SNI Client - Super Mario World Patch Setup"; Types: full playing; Flags: disablenouninstallwarning
Name: "client/sni/l2ac"; Description: "SNI Client - Lufia II Ancient Cave Patch Setup"; Types: full playing; Flags: disablenouninstallwarning
#include "sni_components.iss"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Would "sni_client_components" be a better name?

@ThePhar ThePhar added the is: enhancement Issues requesting new features or pull requests implementing new features. label May 31, 2023
@alwaysintreble
Copy link
Collaborator

do we still need this?

@el-u
Copy link
Collaborator

el-u commented Oct 16, 2023

do we still need this?

Some parts of this would be superseded by #2268, so this would be up for a major rework if that PR were merged.
Nevertheless, there are some parts in here that could still be useful, such as autogenerating the registry keys.

@Berserker66 Berserker66 marked this pull request as draft October 17, 2023 09:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
is: enhancement Issues requesting new features or pull requests implementing new features.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants