- 
                Notifications
    You must be signed in to change notification settings 
- Fork 1.2k
Core: Add a user interface for Archipelago generation #4878
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
base: main
Are you sure you want to change the base?
Conversation
        
          
                GeneratorUI.py
              
                Outdated
          
        
      | name = document["name"] | ||
| game = document["game"] | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| name = document["name"] | |
| game = document["game"] | |
| name = document["name"] | |
| game = document["game"] | 
How does this handle the situation where name or game are dict and not str?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adding isinstance checks for this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A couple of major worries, but still pretty clean. A couple of things I'd personally like to see:
- A way to provide certain CLI args such as players, plando, spoiler, and race mode.
- Some way to either copy errors from the error log, or a way to open the log file directly from the GUI. Could swap to SelectableLabel here for this.
        
          
                GeneratorUI.py
              
                Outdated
          
        
      | for file_path in self.dropped_files: | ||
| self._add_yaml_file(file_path) | ||
|  | ||
| for document in yaml.load_all( | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| for document in yaml.load_all( | |
| for document in yaml.load_all( | 
All uses of the yaml library for loading should be replaced with Utils.parse_yaml and Utils.parse_yamls. See #4872 for more information.
        
          
                GeneratorUI.py
              
                Outdated
          
        
      | self.players.append({"name": name, "game": game, "document": document, "widgets": widgets}) | ||
|  | ||
| async def _generate_async(self) -> None: | ||
| with tempfile.TemporaryDirectory(delete=False) as temp_dir: | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| with tempfile.TemporaryDirectory(delete=False) as temp_dir: | |
| with tempfile.TemporaryDirectory() as temp_dir: | 
This will fail on <py 3.12. AP currently supports py 3.10 and py 3.11.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The delete argument was actually a leftover from my debugging. Thanks.
| for widget in widgets: | ||
| self.player_table.add_widget(widget) | ||
|  | ||
| self.players.append({"name": name, "game": game, "document": document, "widgets": widgets}) | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't appear to account for yaml files that contain multiple valid documents (separated by ---).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These files do not get parsed at all it seems. You'll need to use Utils.parse_yamls here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By the time we get here the YAMLs have already been parsed. They get parsed in the _on_drop_end function when it's a text drop and the _add_yaml_file function when it's coming from files (either dropped or selected through the file manager)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I left this comment in the wrong spot. You need to use it in _add_yaml_file, as a singular yaml file can hold multiple yaml documents.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This comment is still open but the change has been done.
        
          
                GeneratorUI.py
              
                Outdated
          
        
      | on_ref_press: app.on_ref_press(*args) | ||
| adaptive_height: True | ||
| BoxLayout: | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| BoxLayout: | |
| MDBoxLayout: | |
| md_bg_color: self.theme_cls.backgroundColor | 
| process = await asyncio.create_subprocess_exec( | ||
| "python", | ||
| "Generate.py", | ||
| "--player_files_path", | ||
| temp_dir, | ||
| stdin=asyncio.subprocess.PIPE, | ||
| stdout=asyncio.subprocess.PIPE, | ||
| stderr=asyncio.subprocess.PIPE, | ||
| ) | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe this will fail on frozen? We don't require users to install python, and Generate.py will not be on their devices.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not familiar with how things work on frozen. How do you run a python script from another script in frozen, for example how do you launch Generate or LTTP Adjuster from the main Launcher window? I've looked into the launcher code and cannot seem to figure it out. I might need some pointers here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On frozen, you don't have a python script but a full exe. For this to be valid on frozen, the first two arguments must be replaced with ArchipelagoGenerate and then that must also check for linux/windows.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I used the get_exe function from Launcher in this function, it should work on frozen now. I actually need to test this on frozen to see.
        
          
                GeneratorUI.py
              
                Outdated
          
        
      | f"[color=7F7FFF]Generation failed with status code { | ||
| process.returncode}[/color]" | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| f"[color=7F7FFF]Generation failed with status code { | |
| process.returncode}[/color]" | |
| f"[color=7F7FFF]Generation failed with status code {process.returncode}[/color]" | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks a bit weird but I get "SyntaxError: unterminated f-string literal (detected at line 323)" if I apply your suggestion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's what you get with what it is currently, you can't split the {} of an f-string (you can split it in the middle of the string though).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I found a way to fix the f-string while staying below the 120-character limit. It's not exactly your suggestion, I put in all on one line and then asked black to format it.
        
          
                GeneratorUI.py
              
                Outdated
          
        
      | async def _generate_async(self) -> None: | ||
| with tempfile.TemporaryDirectory(delete=False) as temp_dir: | ||
| for idx, player in enumerate(self.players): | ||
| filename = f"P{idx}_{player["name"]}.yaml" | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| filename = f"P{idx}_{player["name"]}.yaml" | |
| filename = f"P{idx}_{player['name']}.yaml" | 
Invalid syntax
| 
 There's a short and a long way I could see the CLI args get implemented: On the short term I could just add a textual input below the player's table to pass in additional options with no verification. On the long run we probably want a separate Options section with more granular controls. The text input could stay as a fallback for stuff we don't support in the UI. | 
        
          
                GeneratorUI.py
              
                Outdated
          
        
      | or not isinstance(document["name"], str) | ||
| or not isinstance(document["game"], str) | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is invalid. See https://archipelago.gg/tutorial/Archipelago/advanced_settings/en. Yamls are allowed to have multiple different options for name/game that can be rolled by the generator.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just learned about that. That means I have to cover the cases when a user has assigned more than one name in their file.
        
          
                GeneratorUI.py
              
                Outdated
          
        
      | on_ref_press: app.on_ref_press(*args) | ||
| adaptive_height: True | ||
| MDBoxLayout: | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| MDBoxLayout: | |
| MDBoxLayout: | |
| md_bg_color: self.theme_cls.backgroundColor | 
I believe I put this in the first review, this is what actually sets the background color.
| process = await asyncio.create_subprocess_exec( | ||
| "python", | ||
| "Generate.py", | ||
| "--player_files_path", | ||
| temp_dir, | ||
| stdin=asyncio.subprocess.PIPE, | ||
| stdout=asyncio.subprocess.PIPE, | ||
| stderr=asyncio.subprocess.PIPE, | ||
| ) | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On frozen, you don't have a python script but a full exe. For this to be valid on frozen, the first two arguments must be replaced with ArchipelagoGenerate and then that must also check for linux/windows.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pretty much just styling comments. My main gripe is that the module is named incorrectly, it should be snake_case. For what this is doing, I think a kivy solution for opening files is better than a modified version of the utils function. Not any kind of blocker but being able to modify host options and having GUI elements for the args would be huge pluses. The current method of launching doesn't work frozen at all, on windows or linux. I'm not sure the exact path you should take but I know launcher has already solved this for the root level scripts that get bundled as their own exe's, but also have relevant components.
        
          
                GeneratorUI.py
              
                Outdated
          
        
      | from kivymd.uix.textfield import MDTextField | ||
| from Utils import is_linux, is_macos, is_windows, parse_yamls | ||
|  | ||
| kv = """ | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
imo this kv should be in its own file instead of in here, especially since it's already a root level script.
        
          
                GeneratorUI.py
              
                Outdated
          
        
      | async def open_filename( | ||
| title: str, filetypes: typing.Iterable[typing.Tuple[str, typing.Iterable[str]]], suggest: str = "" | ||
| ) -> Optional[str]: | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we import just what is needed from typing rather than the full package and native types don't need the package at all.
| async def open_filename( | |
| title: str, filetypes: typing.Iterable[typing.Tuple[str, typing.Iterable[str]]], suggest: str = "" | |
| ) -> Optional[str]: | |
| async def open_filename( | |
| title: str, | |
| filetypes: Iterable[tuple[str, Iterable[str]]], | |
| suggest: str = "" | |
| ) -> str | None: | 
        
          
                GeneratorUI.py
              
                Outdated
          
        
      | async def open_filename( | ||
| title: str, filetypes: typing.Iterable[typing.Tuple[str, typing.Iterable[str]]], suggest: str = "" | ||
| ) -> Optional[str]: | ||
| """Opens a file browser. | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://peps.python.org/pep-0287/
| """Opens a file browser. | |
| """ | |
| Opens a file browser. | 
        
          
                GeneratorUI.py
              
                Outdated
          
        
      | I would have preferred to update the existing function to support async, but it does not exactly work that way. The | ||
| blocking nature of the function freezes the entire UI and leads to a program crash. | ||
| """ | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
while it's not in the PEP we follow full reST style docstrings so these should have param and return typing descriptions.
https://www.sphinx-doc.org/en/master/usage/restructuredtext/basics.html#field-lists
        
          
                GeneratorUI.py
              
                Outdated
          
        
      | """ | ||
|  | ||
|  | ||
| async def show_in_file_explorer(path: str) -> str: | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i doubt this function will be reused anywhere but still probably a good idea to add a docstring in case
        
          
                GeneratorUI.py
              
                Outdated
          
        
      | """Opens a file browser. | ||
| Opens the native file browser if possible, falls back to tkinter otherwise. | ||
| I copy-pasted this functions from utils to use the create_subprocess_exec function from asyncio and make the whole | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TBH since this function has to be async i think you're better off doing a kivy only solution of this instead of trying to emulate the behavior of the synchronous function. https://kivymd.readthedocs.io/en/latest/components/filemanager/
        
          
                GeneratorUI.py
              
                Outdated
          
        
      |  | ||
| def __init__(self): | ||
| super(GeneratorApp, self).__init__() | ||
| self.yamls = [] | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this should be typed in the body
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I removed this. I renamed this field players, I think it's clearer this way. Removed.
        
          
                GeneratorUI.py
              
                Outdated
          
        
      | ui_log: UILog | ||
| options_field: TextInput | ||
|  | ||
| players: list = [] | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i'm not sure if there's a case where another app can be spawned in the same process but still probably safer to initialize these in the constructor
| Also, feel free to reject this for now as out of scope, but a button to press on a successful generation to either start hosting the game locally, or upload it to the website would be a fantastic addition. | 
| 
 I did add this in my future ideas list in the summary of the PR. I want to avoid feature creep in this PR for now, I just want a good base to add features to later on. | 
…le explorer for Windows
| I tested this on Windows now, in VS Code and with an installer (so frozen and not frozen). It works fine. I would like to test on Mac OS but this will require setting up a VM on my end. It's mostly just to verify that my command in the show_in_file_explorer method will work. | 
| this looks really cool! perchance is it possible to have a timer for the gen whenever it starts the gen? I know once the gen finishes it gives the total time, but having a way to see the time spent in gen while gen is still going would be nice. | 
        
          
                worlds/LauncherComponents.py
              
                Outdated
          
        
      | Component('Host', 'MultiServer', 'ArchipelagoServer', cli=True, | ||
| file_identifier=SuffixIdentifier('.archipelago', '.zip')), | ||
| Component('Generate', 'Generate', cli=True), | ||
| Component('Generate (UI)', 'GeneratorUI'), | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| Component('Generate (UI)', 'GeneratorUI'), | |
| Component("Generate (UI)", "GeneratorUI", description="Open the interactive seed generator."), | 
| I could not get a Mac VM working to test this on. So I can't make sure that the method in show_in_file_explorer will work on Mac. | 
| 
 I can probably test that as I do a lot of my dev work on my macbook. | 
| 
 I found an online man page for the command, and -R seems correct, but I'd need to test it myself. Will do that when I get some time. | 
| Something seems wrong on MacOS pressing the "add file" button... I don't know what's causing that, but it's doing something strange, and I think it's the open file explorer thing? | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not sure why that crashing is happening, but I think you might have to use the openers from AP itself. Lemme do some testing.
| f'["file://{path}"]', | ||
| '""', | ||
| ) | ||
| elif is_macos: | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Line 85 in b0f41c0
| def open_folder(folder_path): | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
or just get rid of the -R parameter... will test that myself
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
getting rid of -R didn't fix anything for me:
*** Terminating app due to uncaught exception 'NSInvalidArgumentException', reason: '-[SDLApplication macOSVersion]: unrecognized selector sent to instance 0x7fda230781a0'
*** First throw call stack:
(
	0   CoreFoundation                      0x00007ff802ccaf82 __exceptionPreprocess + 242
	1   libobjc.A.dylib                     0x00007ff80279912d objc_exception_throw + 62
	2   CoreFoundation                      0x00007ff802d7c300 -[NSObject(NSObject) __retain_OA] + 0
	3   CoreFoundation                      0x00007ff802c3dc12 ___forwarding___ + 1335
	4   CoreFoundation                      0x00007ff802c3d648 _CF_forwarding_prep_0 + 120
	5   libtk8.6.dylib                      0x00000001294a13ed GetRGBA + 49
	6   libtk8.6.dylib                      0x00000001294a090f SetCGColorComponents + 125
	7   libtk8.6.dylib                      0x00000001294a11f4 TkpGetColor + 1758
	8   libtk8.6.dylib                      0x00000001293feca3 Tk_GetColor + 141
	9   libtk8.6.dylib                      0x00000001293f1d6e Tk_Get3DBorder + 121
	10  libtk8.6.dylib                      0x00000001293f1c0f Tk_Alloc3DBorderFromObj + 124
	11  libtk8.6.dylib                      0x00000001293fff8f DoObjConfig + 956
	12  libtk8.6.dylib                      0x00000001293ffab7 Tk_InitOptions + 330
	13  libtk8.6.dylib                      0x00000001293ff9b2 Tk_InitOptions + 69
	14  libtk8.6.dylib                      0x0000000129427ea6 CreateFrame + 1567
	15  libtk8.6.dylib                      0x000000012942813d TkListCreateFrame + 138
	16  libtk8.6.dylib                      0x0000000129420a94 Initialize + 2008
	17  _tkinter.cpython-312-darwin.so      0x0000000124a89646 Tcl_AppInit + 86
	18  _tkinter.cpython-312-darwin.so      0x0000000124a835e7 Tkapp_New + 743
	19  _tkinter.cpython-312-darwin.so      0x0000000124a82f82 _tkinter_create + 642
	20  Python                              0x000000010cf7e458 cfunction_vectorcall_FASTCALL + 96
	21  Python                              0x000000010d026d62 _PyEval_EvalFrameDefault + 50831
	22  Python                              0x000000010cf2ea6b _PyObject_FastCallDictTstate + 87
	23  Python                              0x000000010cfa29de slot_tp_init + 209
	24  Python                              0x000000010cf998eb type_call + 135
	25  Python                              0x000000010cf2ec4b _PyObject_MakeTpCall + 140
	26  Python                              0x000000010d026f07 _PyEval_EvalFrameDefault + 51252
	27  Python                              0x000000010d01a517 PyEval_EvalCode + 197
	28  Python                              0x000000010d081693 run_eval_code_obj + 83
	29  Python                              0x000000010d07f932 run_mod + 107
	30  Python                              0x000000010d07f0d3 PyRun_StringFlags + 113
	31  Python                              0x000000010d07f01b PyRun_SimpleStringFlags + 68
	32  Python                              0x000000010d0a222d Py_RunMain + 724
	33  Python                              0x000000010d0a2889 pymain_main + 378
	34  Python                              0x000000010d0a293c Py_BytesMain + 42
	35  dyld                                0x000000020471a530 start + 3056
)
libc++abi: terminating due to uncaught exception of type NSException```
| open.md | 

What is this fixing or adding?
This is adding a GUI for Archipelago generation. I thought it would be useful to have a GUI to do so. This is an MVP and could be improved with more features to streamline this UI flow.
Features
Technical Notes
To make the "Add File" button work, I have had to copy the open_filename function from Utils and make an async version of it, including calls to asyncio.create_subprocess_exec rather than subprocess.run. It is in the GeneratorUI.py file, though it could move to Utils.py if necessary. I would like to find a way to reuse the logic in both functions without duplicating it, or find a way to call the sync variant of the function from my async code without crashing the UI.That was fixed with asyncio and using a ThreadPoolExecutor.If there is an error in the Generate.py script, the process stops with a "Press enter to close." message, which waits for keyboard input before finishing the process. I patched this out by sending a bunch of new lines in stdin, but it would be good to have for example a "batch" mode that does not wait on input. Or rework the Generate code to call this directly from the UI and not rely on a subprocess exec.Seeming that will be fixed in Generate/Launcher: Add arg to prevent closing window on gen success and make Launcher use it. #4886, although I'm going to keep my workaround just in case.Future Improvements
How was this tested?
If this makes graphical changes, please attach screenshots.
This shows the normal UI flow (happy path) to generate games in Archipelago:
Screencast from 2025-04-14 18-42-11.webm
This shows what happens if an unsupported YAML file is added to the table:
Screencast from 2025-04-14 18-44-40.webm
This shows the console window with an error message when generation fails:
Screencast from 2025-04-14 18-45-11.webm