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

Mac build improvements #95

Draft
wants to merge 14 commits into
base: golf-1.15
Choose a base branch
from
Draft

Mac build improvements #95

wants to merge 14 commits into from

Conversation

JonnyPtn
Copy link
Contributor

@JonnyPtn JonnyPtn commented Dec 23, 2023

Things to note:

  • Increased minimum cmake version to 3.23. A lot more modern than it was but not cutting edge and should still be easily available on most platforms
  • Removed mojoal, as audio seems fine using the system openAL on Mac (not sure if there were other reasons to use it?)
  • Imgui font loading needed some tweaking. Firstly there were some missing fonts which I've commented out and then it also needs the resource path prepended for them to load correctly when bundled

Should make building for Mac (and other platforms in theory) a bit simpler, so you can just run these two commands:
cmake --preset xcode
cmake --build --preset xcode --config release --target install

And it will do a release build using Xcode and install a complete standalone bundle to the install/Xcode folder.

Next up... code signing, which is typically an arduous task to get working correctly. Current implementation when building with Xcode will just use the default ad-hoc signing, which will work fine for the machines it's built on but nowhere else (as far as I understand) so will need to work out a solution for proper distribution signing

@fallahn
Copy link
Owner

fallahn commented Dec 23, 2023

Fantastic! Thank you.

So regarding mojoal - there were two reasons I used it. One was in preparation for a Switch port, however that's now moot as Nintendo rejected the game 😢

The second was with Apple's OpenAL lib I had this weird problem with sounds playing at random pitches - and not just a little bit either. Sometimes the announcer would sound like a chipmunk, and sometimes the player would sound like Barry White. mojoal appeared to fix this. Really I'd like to use OpenAL-soft if only for consistency but, uh.. I couldn't get cmake to pick the correct library, even though I'd installed it from brew. If you're not getting the pitch problem I'll give it another try and see if I still get it.

RE: the non-existent fonts... they do exist. However I currently have all the assets in a separate, private, repo because it also has all the Steam stuff in it. You can get a (more or less) up to date copy of the assets from Steam by switching to the beta branch on Windows/Linux or I can grant you access to the repo if you prefer.

Thanks again - I appreciate all the effort!

@JonnyPtn
Copy link
Contributor Author

I didn't notice any audio issues but will test a bit more and see if there are any issues, using openal-soft should certainly be possible.

Will grab the other font assets from the steam branch then and update this PR once I've done that.

I also just came across an issue where it said I was disconnected from the server while only playing locally so will debug that a bit

@JonnyPtn JonnyPtn marked this pull request as draft December 30, 2023 14:55
@fallahn
Copy link
Owner

fallahn commented Dec 30, 2023

I've temporarily had to revert the cmake file for the golf project, as it broke the build script for SteamOS. I really I need to update the build script, but I was in a rush - sorry! I'll get it fixed this week.

@JonnyPtn
Copy link
Contributor Author

JonnyPtn commented Jan 2, 2024

No problem at all - I was conscious these changes might break other platforms so just relying on what the CI says, but hoping to get the cmake (and CI ideally) updated so it's consistent and reliable for all platforms without extra scripts

@fallahn
Copy link
Owner

fallahn commented Jan 2, 2024

So I'm working on fixing up the SteamOS script, however my limited CMake knowledge has hit a roadblock 🤦‍♀️ - I'll try to explain it as best I can:
So with your updated CMake file building the regular version works perfectly. I just configure CMake

cmake -DCMAKE_BUILD_SAMPLES=true
make install

However, as the SteamOS build is containerised (and also built inside another container... 🙄) I need to configure the two projects, golf and crogine, separately with different settings

cmake -DUSE_GL_41=true -DCMAKE_INSTALL_PREFIX=~/work/out/crogine

for crogine (it gets installed into a local directory long enough for the build to complete), and

cmake -DUSE_GNS=true -DUSE_NEWSFEED=false -DCMAKE_PREFIX_PATH=~/work/out/crogine -DCMAKE_SKIP_RPATH=TRUE -DCMAKE_EXE_LINKER_FLAGS="-Wl,-R,\$ORIGIN/lib"

which includes linker settings that shouldn't be applied to crogine. However, if I'm not mistaken, all these settings will be configured for both projects if I use the new CMake file.

Any thoughts?

@JonnyPtn
Copy link
Contributor Author

JonnyPtn commented Jan 3, 2024

Yeah that's great info, and yeah definitely solvable.

So presuming the bit I broke was building golf/crogine as separate projects - I didn't consider that too much as they are ostensibly in the same project so having to install crogine then configure golf separately seems like needless extra work (and things to go wrong). It also wouldn't match up with the VS projects you primarily use, if I understand correctly, as that's just one solution containing both targets, so it makes sense for cmake to work the same way.

If you're OK with continuing along those lines, then it would be the extra linker settings that need fixing, as you mentioned - As I've started updating the cmake to set properties on targets rather than globally, this problem should effectively go away, as we can set different linker flags for golf/crogine and not have to configure them separately (or add them on the command line).

I'll look at fixing that up in this PR, and add a cmake preset for building on linux which you can use - I've only currently got headless linux machines available so won't be able to verify it plays, but at least can make sure it builds with the settings you've noted above

@fallahn
Copy link
Owner

fallahn commented Jan 3, 2024

Sounds perfect! Thanks 😁 - just to clarify though, this is only for the SteamOS build. Regular linux builds work fine with your existing CMake file. The extra linker/rpath settings are for libraries which get bundled with the executable on Steam.

@JonnyPtn
Copy link
Contributor Author

JonnyPtn commented Jan 3, 2024

Ah cool good to know - I'll do a Linux preset and then a steamos one that inherits it with the extra options

@fallahn
Copy link
Owner

fallahn commented Feb 18, 2024

A little update: on the 1.16 branch I have the mac version building and linking to openal-soft from brew. I had to force the CMAKE_PREFIX_PATH at the top of the file - not sure yet if this breaks anything, particularly if brew/openal-soft isn't installed. It's nice to be able to ditch mojoal though 😎

image

@JonnyPtn
Copy link
Contributor Author

Sounds good - as usual life got in the way :(

This got a bit out of hand anyway with various cmake improvements so what I'll probably do is try and break it down into smaller changes you can merge in gradually rather than trying to do it all at once

@fallahn
Copy link
Owner

fallahn commented Feb 21, 2024

No worries! Just an FYI really, in case I broke anything you'd done. I'm not making a big drive on the mac version right now, what really happened is that I changed some stuff on Windows which is using OpenAL-soft which then broke the CI for the mac build... and that made my scalp itch. You only have to look at the recent commit history to see my opinions 😅

I've also tested that if OpenAL-soft isn't installed cmake still finds the default OpenAL lib without any problems.

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.

None yet

2 participants