Repository navigation
Build: prepare the shaders at the end of the build, not at the first start - #1026
klimovich008 wants to merge 4 commits into
Conversation
Cretezy
left a comment
There was a problem hiding this comment.
Thanks for this, preparing the shaders at build time is a nice improvement. A few issues below, the first three being the most important.
One more that falls outside the diff, in builder/linux/hookshot_builder.py (~line 500): compile_started and eta are set during the compile and never cleared on a later @@stage, so during the (now minutes-long) shaders stage the progress page keeps showing a growing "compiling M:SS · about N minutes left". That makes the build look stuck. Clearing them when a new stage starts would fix it.
🤖 Generated with Claude Code
| if (rows.TryGetValue("download", out var d)) { d.Done(); d.Small("Build tools checked"); } | ||
| Run(tools, new List<string> { "--update", folder }, keepRows: true); | ||
| var args = new List<string> { "--update", folder }; | ||
| if (PrepareTick.IsChecked != true) args.Add("--no-prepare-shaders"); // (as the last build chose: ApplySettings) |
There was a problem hiding this comment.
On the launcher's Update button (LaunchArg -> ShowLauncher -> LaunchUpdateClick) and on the --update restart after a self-update (Updater.FromCommandLine), we return before ApplySettings() runs, so PrepareTick keeps its XAML default IsChecked=True. A player who unticked "Prepare shaders now" gets the preparation anyway on those updates, so the comment here doesn't hold for exactly the update flows. Reading the saved PrepareLater setting directly here (rather than the checkbox) would fix both paths.
| ("lift", "Translate the game to PC code", "Translate the game to PC code"), | ||
| ("compile", "Compile", "Compile changed files"), | ||
| ("finish", "Put the game together", "Swap in the new game"), | ||
| ("shaders", "Prepare the shaders", "Prepare the shaders"), |
There was a problem hiding this comment.
This row is always added, and Ended() marks every row Done, so it gets a check mark even when the step was skipped (tick off / HOOKSHOT_NO_PREPARE) or failed (no Vulkan, exit 3). The player is told the shaders are ready and still gets the long black-window wait at first start. Maybe only add the row when preparation is on, and show it as skipped/failed when no @@stage shaders arrives or the game exits non-zero.
| print("the shaders were not prepared now (%s): the game prepares them at its first start" % e, flush=True) | ||
| return 1 | ||
| no_throttle(getattr(p, "_handle", None)) | ||
| for line in p.stdout: |
There was a problem hiding this comment.
There's no timeout here: if the game hangs in --prepare-shaders (driver deadlock, a Windows crash dialog left open on hookshot.exe), for line in p.stdout / p.wait() never return and the build sits at 92% forever. The game folder is already complete by then, so the only way out is cancelling, which reports failure. That goes against "failures don't fail the build". A watchdog (e.g. a reader thread plus p.wait(timeout=...) and p.kill()) would keep that promise.
| Its @@stage and @@progress lines go on to the builder; its own @@done and @@error would end or fail the build there, so they | ||
| go on as plain text. A PC whose Vulkan can't (exit 3), or any other failure, leaves the work to the first start, as before""" | ||
| try: | ||
| p = subprocess.Popen(cmd, stdout=subprocess.PIPE, stderr=subprocess.STDOUT, text=True, encoding="utf-8", errors="replace", |
There was a problem hiding this comment.
This runs the game with make_game's full environment. hookshot-launch and play() strip HOOKSHOT_*, PYTHON*, APPDIR, APPIMAGE, ARGV0, OWD and LD_LIBRARY_PATH first, so the game uses its own SDL and the system libraries. Here, inside the AppImage, the Vulkan loader/driver can pick up the image's libraries: either the run fails (exit 3) and silently falls back to first start, or it prepares under a different environment than the player's game runs in. Reusing the same env-cleaning as the other launches would avoid that.
| print("the shipped pipeline list: %d KB, built before the first title" % (os.path.getsize(seed) >> 10), flush=True) | ||
|
|
||
|
|
||
| def prepare_shaders(cmd): |
There was a problem hiding this comment.
nit: this largely re-implements run() (Popen with pipe, no_throttle, rstrip/print loop). Only the @@ line rewrite differs, and a line-filter argument to run() could cover it. As it stands the two copies already differ (no cwd=ROOT, no PYTHONIOENCODING).
| # changed; the builder says how much stays and lets it go. Kept in a development checkout) | ||
| if os.path.normcase(work) != os.path.normcase(os.path.abspath(os.path.join(ROOT, "work"))): | ||
| shutil.rmtree(exefs, ignore_errors=True) | ||
| if not a.no_prepare_shaders and not os.environ.get("HOOKSHOT_NO_PREPARE"): # (either: left to the first start, as before) |
There was a problem hiding this comment.
os.environ.get("HOOKSHOT_NO_PREPARE") is a truthiness check, so HOOKSHOT_NO_PREPARE=0 also skips preparation. The game's HOOKSHOT_NO_UPDATE_CHECK handling compares against "0"; matching that would be more consistent.
| if os.path.normcase(work) != os.path.normcase(os.path.abspath(os.path.join(ROOT, "work"))): | ||
| shutil.rmtree(exefs, ignore_errors=True) | ||
| if not a.no_prepare_shaders and not os.environ.get("HOOKSHOT_NO_PREPARE"): # (either: left to the first start, as before) | ||
| prepare_shaders([os.path.join(game_dir, BESIDE[0]), "--prepare-shaders"]) |
There was a problem hiding this comment.
With RX_TARGET=windows on Linux (CROSS), BESIDE[0] is hookshot.exe, so this tries to exec a PE binary on the build host. Without binfmt it fails with a misleading "prepared at first start" message. With Wine registered through binfmt_misc, it prepares a cache for the build machine's GPU instead of the target PC's. I think this should be skipped when CROSS is set.
| shutil.rmtree(exefs, ignore_errors=True) | ||
| if not a.no_prepare_shaders and not os.environ.get("HOOKSHOT_NO_PREPARE"): # (either: left to the first start, as before) | ||
| prepare_shaders([os.path.join(game_dir, BESIDE[0]), "--prepare-shaders"]) | ||
| total_seconds = time.time() - STARTED |
There was a problem hiding this comment.
minor: preparation runs before total_seconds / record_times, so build_times.json and the "the build took X (the last one Y)" comparison mix in a GPU-dependent step that can be switched off. Recording it after (or separately) keeps the timings comparable between runs.
| self.assertLess(at, main.index("say(\"done\", out)")) | ||
|
|
||
| def test_the_builders_can_leave_it_to_the_first_start(self): | ||
| text = SOURCE.read_text(encoding="utf-8") |
There was a problem hiding this comment.
These tests check the source text (substring positions, exact call text, exactly 2 --no-prepare-shaders per builder file) rather than behaviour. Harmless refactors break them (renaming game_dir, wrapping the guard, a third use of the flag), while the launcher/--update issue above still passes. Could prepare_shaders be tested against a stub executable that prints @@ lines / hangs / exits 3 instead?
518656b to
f1432f8
Compare
|
Thanks for the thorough review. All ten points are addressed in f1432f8 (rebased on current main):
Checks: |
hckrman101
left a comment
There was a problem hiding this comment.
Thanks, that's a thorough rework. I went through all ten points and they're addressed: BeginUpdate covers all three update entry points, the row only gets a check mark when the shaders really were prepared, game_env() matches hookshot-launch, the stub-exe tests are a big improvement, and cross builds / HOOKSHOT_NO_PREPARE=0 / the Linux stage clock are fixed. A few things in the new code, mostly around the watchdog and Cancel:
One outside the diff: in the Linux builder's update-game mode (build(), ~line 434), the update starts without saving the config, so unticking "Prepare shaders now" there only affects this update. The next launcher or self-update reads prepare=1 again. Windows saves the settings before UpdateFolder, so the two builders differ here.
| on_line(line) | ||
| except Silent: | ||
| p.kill() | ||
| p.wait() |
There was a problem hiding this comment.
After the silence timeout this p.wait() has no timeout. The case the watchdog is mainly for is a game stuck inside a GPU driver call, and such a process often can't die right away (D state on Linux, a pending kernel wait on Windows). Then wait() blocks forever and we're back to a build that never ends. Something like p.wait(timeout=30), then giving up on the process and carrying on, would close that gap.
| the game's folder and environment, as the builders' Play starts it. A PC whose Vulkan can't (exit 3), a run silent for `silence` seconds (then stopped), or | ||
| any other failure leaves the work to the first start, as before: never a failed build. Returns its exit code (None: stopped)""" | ||
| try: | ||
| code = run(cmd, check=False, show=show_prepared, cwd=folder or os.path.dirname(cmd[0]) or None, |
There was a problem hiding this comment.
Success is judged only by the exit code. If the game prints @@done shaders (cache saved, marker removed) and then hangs in Vulkan/driver teardown at exit, which some drivers do, the 15-minute silence kills it, and the build reports "not prepared" 15 minutes late although the work is done. Noting the @@done shaders line in show_prepared/on_line and treating it as success (or switching to a short silence after it) would handle that.
| } | ||
| SetProgress(1); | ||
| ShowReady(); | ||
| return; |
There was a problem hiding this comment.
A Cancel during the optional shaders step goes through the "cancelled" branch just below, back to the files page, even though the new game is already installed by then. So ShowReady never runs: no shortcuts, and the launcher doesn't learn the folder is ready. On an AMD GPU where preparation takes 20+ minutes, pressing Cancel there is quite likely. Linux has the same issue (on_exit shows "The build stopped"). Ending a cancel that comes after the finish stage as "ready, shaders left to the first start" would fit the step being optional.
| VersionChip.Visibility = Visibility.Visible; | ||
| VersionChipText.Text = from != null && from != to ? $"{from} → {to}" : to; | ||
| MakeRows(update: true); | ||
| prepareNow = BuilderSettings.PrepareNow(SettingsPath); |
There was a problem hiding this comment.
This reads the saved choice correctly, but on the launcher path ApplySettings still never runs, so PrepareTick keeps its default (ticked). If the player then goes Back and presses Build in that window, SaveSettings writes PrepareLater = false over their saved true, and every later update prepares again. Loading the tick from prepareNow here (or calling ApplySettings on that path) would keep them in sync.
| @@ -602,6 +611,8 @@ def on_exit(self, code): | |||
| text += ", compiled in %d:%02d" % (int(compiled) // 60, int(compiled) % 60) | |||
| if previous > 0: | |||
| text += " · last time %d:%02d" % (int(previous) // 3600, int(previous) // 60 % 60) | |||
There was a problem hiding this comment.
minor: on Linux "built in" is wall time including the preparation, while "last time" is the @@times total, which now leaves it out. So a build that took exactly as long as last time reads 10 minutes slower when preparation took 10. Windows uses builtIn for this; comparing against self.times[0] here would match it.
| """A step, its output printed as it comes. show: what of each line is printed (its None: nothing; on_line and a failure's | ||
| reason still get the line itself). base: the environment it starts from (os.environ). silence: seconds without a line after | ||
| which the step is stopped; it returns None then (with check, the build fails)""" | ||
| e = dict(os.environ if base is None else base, PYTHONIOENCODING="utf-8") |
There was a problem hiding this comment.
nit: with base=game_env() this puts PYTHONIOENCODING back in, so the game's environment isn't quite the one game_env() / hookshot-launch give. Harmless for the game, but the env test doesn't notice because that name isn't in its list. Adding it only when base is None would make the claim exact.
f1432f8 to
d2837c4
Compare
…start make_game.py now runs hookshot --prepare-shaders as its last step, so the minutes of shader preparation happen under the builder's progress bar instead of behind the game's black window at its first start (issue 711). The command has been in the game since PR 320, but nothing ran it. Its @@stage/@@progress lines go on to the builder; its own @@done and @@error go on as plain text, since both builders read those as the build's end or failure. A PC without usable Vulkan (exit 3) or any other failure leaves the work to the first start, as before. HOOKSHOT_NO_PREPARE=1 skips the step. The Windows builder gets a "Prepare the shaders" row. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Unticked, the builders pass make_game.py --no-prepare-shaders and the game prepares its shaders at its first start, as before. The choice is kept (builder-settings.json on Windows, builder.ini on Linux) and used for updates too. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…bs harness The harness's window now has the tick (ticked, the default), the saved settings carry prepare=1, and a new case checks that unticked it passes --no-prepare-shaders and saves prepare=0. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…t row, watchdog, the game's own environment - Windows builder: the update flows (the launcher's Update, the --update start after a self-update) read the saved PrepareLater setting (BuilderSettings.PrepareNow) instead of the tick, which ApplySettings never set there. - The "Prepare the shaders" row only when they are prepared; it ends skipped, with the reason under it, when no @@stage shaders came or make_game printed its "the shaders were not prepared now (<why>)" line (a plain line: older builders only show it). The Linux builder notes it on its ready line. - make_game: run() gains show (a line filter), cwd, base (environment) and silence (a reader thread; a step quiet that long is killed). prepare_shaders uses it, in the game folder and game_env() (the Play button's environment, now shared with the Linux builder), and stops a run silent for 15 minutes: left to the first start, never a failed build. - HOOKSHOT_NO_PREPARE skips unless "0", as the game reads its switches; RX_TARGET=windows on Linux (CROSS) skips. - The build times are recorded and said before the preparation (build_times.json stays comparable). - Linux builder: a new @@stage clears the compile's clock and countdown. - Tests: prepare_shaders against stub games (relay, exit 3, crash, silent hang, missing exe, environment and folder), run()'s silence and filter, the skip decision, the Linux builder's stage handling; the Windows builder's settings and row decisions in the builder_workers harness. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
d2837c4 to
d6594bf
Compare
Reopens #819, rebased on current main (GitHub wouldn't reopen it after the rebase). The "Prepare shaders now" tick requested in review is included.
tests/test_linux_builder_jobs.pynow passes: its harness window has the tick, the saved settings carryprepare=1, and a new case checks the unticked path (--no-prepare-shaders,prepare=0).What
make_game.pynow runshookshot --prepare-shadersas its last step, so shader preparation happens during the build, under the builder's progress bar, instead of at the game's first start.@@stageand@@progresslines go on to the builder unchanged. Its own@@done shadersand@@error ...lines go on as plain text, because both builders read those as the build itself finishing or failing.make_game.py --no-prepare-shadersand the game prepares at its first start, as today. The choice is remembered and used for updates.HOOKSHOT_NO_PREPARE=1also skips the step.Why
hookshot --prepare-shadershas been in the game since PR #320, but nothing runs it: notmake_game.py, the Windows builder or the Linux builder. So every player gets the preparation at their first start, as minutes of a black game window. Its progress only shows in a console window that can open behind the game, which is why it looks like a hang (#26, #711, the "stuck at 99%" reports behind #701).On my PC (RX 9070 XT, Ryzen 7 5700X3D), the first start after a fresh v2.0.3 build took 197 s before the first frame; it was 302 s on v2.0.2. A warm start takes 5.5–7.5 s. With this change, those minutes move into the build, where the player expects to wait and can see progress.
This PR doesn't change what gets prepared. It runs the same
prepare_runthe first start runs, so it neither fixes nor worsens the compute-layout bug that #772 fixes. #701, #702 and #772 change how the preparation works, not when it runs, so this combines with them.Testing
tests/test_prepare_at_build.py(4 cases):@@done: the first two are relayed, and@@donecomes through as plain text;@@errorand exits 3: no@@line reaches the builder, the function returns 3, and the message says the game will prepare at first start;main(): it runs after the game folder is swapped in, the shader list is written and the pipeline seed is copied, and before@@done.test_finish_output.py,test_builder_status.py,test_build_log.py.hookshot.exe --prepare-shadersrun with its stdout piped printed its@@progressand@@done shaderslines and exited 0. That confirms the relay gets the lines on Windows too.tests/test_linux_builder_jobs.py(14 cases),test_builder_status.py,test_builder_scale.py,test_builder_python.py,test_make_game_cache.py,test_finish_output.py,test_build_log.pyandtest_prepare_at_build.pypass on current main.StagesandWeighttables.🤖 Generated with Claude Code