Skip to content

Build libultraship into the binary directory and the host asset packer optimized - #418

Open
buddingmonkey wants to merge 2 commits into
HarbourMasters:develop-splitrockfrom
buddingmonkey:lh/cmake-build-fixes
Open

Build libultraship into the binary directory and the host asset packer optimized#418
buddingmonkey wants to merge 2 commits into
HarbourMasters:develop-splitrockfrom
buddingmonkey:lh/cmake-build-fixes

Conversation

@buddingmonkey

Copy link
Copy Markdown

Two independent, small fixes to the top-level CMakeLists.txt. Both are one-line
changes plus explanatory comments.

Asset extraction is ~60x faster on MacOS and iOS

When I was developing my own port to iOS the in-game MacOS and iPad on-device extraction to bk.o2r from a 16 MB ROM, same machine, same input:

Torch build Time
optimized (current behavior) 2312.96 s (38.5 min)
-O3 (this PR) 38.28 s

Byte-identical 24 MB output. The work is single-threaded and 99.8% user CPU, so
this was never I/O or memory bound — it was simply an un-optimized binary.

These numbers were measured on macOS while tracking down why on-device asset
extraction was taking tens of minutes; they have not been re-measured on Linux or
Windows. The absolute times will differ per machine, but the cause is generator-
level and not platform-specific, so any single-config generator build should see
the same class of improvement.

Why it was happening

ExternalProject does not inherit CMAKE_BUILD_TYPE from the parent project, and
TorchExternal never passed one. On a single-config generator — Ninja or
Makefiles, which is the normal Linux and macOS command-line path — an empty build
type means no optimization flags at all, so the asset packer was compiled -O0
even when the surrounding project was configured Release.

Torch is a build-time tool, so the fix is to configure it Release unconditionally
rather than trying to track the host project's configuration:

CMAKE_ARGS -DCMAKE_INSTALL_PREFIX=${CMAKE_BINARY_DIR}/Torch -DENABLE_ASAN=${ENABLE_ASAN} -DCMAKE_BUILD_TYPE=Release

Multi-config generators (Visual Studio, Xcode) ignore CMAKE_BUILD_TYPE at
configure time and are unaffected either way. The if (CMAKE_CONFIGURATION_TYPES)
test that selects TORCH_EXECUTABLE is deliberately left untouched.

libultraship built into the binary directory

-add_subdirectory(libultraship ${CMAKE_CURRENT_SOURCE_DIR}/libultraship)
+add_subdirectory(libultraship)

The second argument to add_subdirectory() is the binary directory, not a second
source path. Pointing it at the source directory made libultraship write its entire
build output — CMakeScripts, cmake_install.cmake, generated headers and every
target's .a — into the submodule's source tree. Consequences:

  • Every build directory shared one output location, so two concurrent builds
    overwrote each other's artifacts.
  • git status inside the libultraship submodule was permanently dirty.

Dropping the argument lets CMake place the output under the binary directory, which
is the normal arrangement.

⚠️ Behavior change reviewers should be aware of

This relocates libultraship's build output. Artifacts that previously appeared
inside the libultraship/ source tree now land under the build directory. Any
scripts, packaging steps, IDE configuration or CI tooling that reference build
artifacts by a path inside libultraship/ will need those paths updated. Nothing
else in the build graph changes — the target name, dependencies and link line are
all as before.

Testing

CMake configure was exercised to confirm the file still parses and that both
changed call sites are reached. No syntax or parse errors; the only failures were
unrelated missing submodule checkouts in the test tree.


Portions of this change were prepared with AI assistance; the diff, the reasoning
and the measurements above were reviewed and verified by hand.

@buddingmonkey
buddingmonkey changed the base branch from develop to develop-splitrock August 6, 2026 16:03
Comment thread CMakeLists.txt Outdated
…r optimised

Two independent build fixes to the top-level CMakeLists.txt.

The second argument to add_subdirectory() is the *binary* directory, but it
was pointed at the source directory. That made libultraship write its entire
build output -- CMakeScripts, cmake_install.cmake, generated headers and every
target's .a -- into the submodule's source tree, so every build directory
shared one output location. Two builds running at once would overwrite each
other's artifacts, and `git status` inside the submodule was permanently
dirty. Dropping the argument lets CMake place the output under the binary
directory as usual. Note that this relocates libultraship's build output:
anyone with scripts or tooling that reference build artefacts inside the
libultraship/ source tree will need to update those paths.

Separately, TorchExternal was being built without optimisation.
ExternalProject does not inherit CMAKE_BUILD_TYPE, and an empty build type on
a single-config generator -- Ninja or Makefiles, which is the normal Linux and
macOS command-line path -- means no optimisation flags at all, so the packer
was compiled -O0 even from a Release tree. Torch is a build-time asset packer,
so it is now configured Release explicitly regardless of the host project's
configuration. Multi-config generators such as Visual Studio and Xcode ignore
CMAKE_BUILD_TYPE at configure time and are unaffected either way.

The cost was not subtle. Extracting bk.o2r from a 16 MB ROM on the same
machine took 2312.96 s (38.5 minutes) unoptimised versus 38.28 s at -O3,
roughly 60x, for byte-identical 24 MB output. The work is single-threaded and
99.8% user CPU, so this was never I/O or memory bound.
@Malkierian

Copy link
Copy Markdown
Contributor

I tried implementing the Torch thing locally, and it was a no-op. If anything, it added 10-15 seconds to my extraction time compared to previous benchmarks. This is on Windows.

Also, I don't think removing that second argument to add_subdirectory is a good thing, it should be changed to the current binary dir instead.

Omitting the second argument to add_subdirectory() already resolves to
${CMAKE_CURRENT_BINARY_DIR}/libultraship, so this is not a behavior change --
it just states the destination rather than relying on the default.
@buddingmonkey

Copy link
Copy Markdown
Author

That tracks. VS is multi-config, and ExternalProject inherits CMAKE_GENERATOR even though it doesn't inherit CMAKE_BUILD_TYPE, so the sub-build ignores it and just follows --config $<CONFIG>. Your Release build was already getting a Release Torch, so there was nothing for this to fix. The -O0 case is single-config generators only (Ninja/Makefiles), i.e. the Linux/macOS CLI path and CI.

Configuring Torch both ways with Ninja, torch.dir FLAGS:

no CMAKE_BUILD_TYPE:       -Wno-narrowing -std=c++20
CMAKE_BUILD_TYPE=Release:  -Wno-narrowing -O3 -std=c++20

Windows Debug builds do still get a Debug Torch, though, since --config Debug gets forwarded down. Happy to send that as a follow-up if it's worth having.

On add_subdirectory: dropping the arg already resolves to ${CMAKE_CURRENT_BINARY_DIR}/libultraship, so same destination but pushed it explicitly, no reason to rely on the default.

buddingmonkey added a commit to buddingmonkey/Lighthouse that referenced this pull request Aug 8, 2026
Mirrors the change made on lh/cmake-build-fixes for PR HarbourMasters#418. Omitting the second
argument to add_subdirectory() already resolves to
${CMAKE_CURRENT_BINARY_DIR}/libultraship, so this is not a behavior change.
@Malkierian

Copy link
Copy Markdown
Contributor

Any ideas, then, on why it still takes 1:30 on Windows in release config, if Mac is less than 40 with this now?

@buddingmonkey

Copy link
Copy Markdown
Author

Honestly, no clue why it's different on Windows. I'm working on an Ubuntu PC and a MacBook Pro with an ARM processor. The best test I can offer is to build twice and make 2 separate builds; with and without the change. Then swap back and forth a few times as a test. This makes both tests function identically.

The extraction is single threaded so there's a small chance you're getting CPU throttled post compile. That said there really shouldn't be any delta for Windows.

What I do know is on Linux, Mac, and especially iOS devices the change is enormous before and after. It still extracts on Apple hardware but it's a slog.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants