Fix NON_PORTABLE build. Ship::Context needs to be initialized first.

[fabienr]

Please check the commit log. As I'm new to SpaghettiKart, I don't know yet how to use mods. The change looks safe but if someone can confirm mod still works, I would appreciate.

Patches found on https://aur.archlinux.org/cgit/aur.git/tree/spaghettikart-pkg-fixes.patch?h=spaghettikart-git

Without the patches it fails like this on OpenBSD (as example) :

[New process 403760]
Core was generated by `spaghettikart'.
Program terminated with signal SIGSEGV, Segmentation fault.
#0  0x0000098cb67e0338 in std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> >::operator= (this=0x764e8de9aaf8, __str=...)
    at /usr/src/gnu/lib/libcxx/../../../gnu/llvm/libcxx/include/string:2651
2651      if (this != std::addressof(__str)) {
(gdb) bt
#0  0x0000098cb67e0338 in std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> >::operator= (this=0x764e8de9aaf8, __str=...)
    at /usr/src/gnu/lib/libcxx/../../../gnu/llvm/libcxx/include/string:2651
#1  0x0000098a7fe5692f in Ship::Context::GetAppDirectoryPath (appName=...)
    at /mnt/ext/_ports/pobj/spaghettikart-0.9.9.1pl20251231/SpaghettiKart-186ea294aedd05efc9ab799507dd96040a05741c/libultraship/src/ship/Context.cpp:488
#2  0x0000098a7fe5851d in Ship::Context::GetPathRelativeToAppDirectory (path=..., appName=...)
    at /mnt/ext/_ports/pobj/spaghettikart-0.9.9.1pl20251231/SpaghettiKart-186ea294aedd05efc9ab799507dd96040a05741c/libultraship/src/ship/Context.cpp:506
#3  0x0000098a7f95ae4b in __cxx_global_var_init.4(void) ()
    at /mnt/ext/_ports/pobj/spaghettikart-0.9.9.1pl20251231/SpaghettiKart-186ea294aedd05efc9ab799507dd96040a05741c/src/engine/mods/ModManager.cpp:22
#4  0x0000098a7f95ad25 in _GLOBAL__sub_I_ModManager.cpp ()
#5  0x0000098cb9f8fbe3 in _dl_call_init_recurse (object=0x98c82c12000, initfirst=0) at /usr/src/libexec/ld.so/loader.c:918
#6  0x0000098cb9f8a78b in _dl_call_init (object=0x98c82c12000) at /usr/src/libexec/ld.so/loader.c:857
#7  _dl_boot (argv=<optimized out>, envp=<optimized out>, dyn_loff=10500020133888, dl_data=0x764e8de9acb0) at /usr/src/libexec/ld.so/loader.c:766
#8  0x0000098cb9f8af26 in _dl_start () at /usr/src/libexec/ld.so/amd64/ldasm.S:61
#9  0x0000000000000000 in ?? ()

[coco875]

hmm then move hardcoded string "mk64.o2r" and "spaghetti.o2r" in const variable then

[fabienr]

hmm then move hardcoded string "mk64.o2r" and "spaghetti.o2r" in const variable then

Not sure what you mean, you would like to define them in a single place for all their occurence in the whole project ?

NON_PORTABLE mean the ressource manager will search in CMAKE_INSTALL_PATH & in user's HOME but to do so it need to be properly initialised before, which was fixed in the GameEngine(), see src/port/Engine.cpp in https://github.com/HarbourMasters/SpaghettiKart/pull/532/files.

[MegaMech]

This is a good change! Thank you.

I believe coco is asking to put

constexpr std::string assets_path = "spaghetti.o2r";

And then you use assets_path in the function call just like you have
Like so:
const std::string assets_path = Ship::Context::LocateFileAcrossAppDirs(assets_path);

[coco875]

I just mean have

const char main_file[] = "mk64.o2r";
const char asset_file[] = "spaghetti.o2r";

to be a bit more future proof if we change file name in this file not all the code

[fabienr]

What about using define in src/port/Engine.h, like this :

#define GAME_ASSET_FILE "mk64.o2r"
#define ENGINE_ASSET_FILE "spaghetti.o2r"

[coco875]

That can work but why use macro and not static char array ?

[MegaMech]

What about using define in src/port/Engine.h, like this :

#define GAME_ASSET_FILE "mk64.o2r" #define ENGINE_ASSET_FILE "spaghetti.o2r"

The issue is this is global for the whole code-base. If that's what we want that's fine. But modmanager.h isn't the best place for this. Maybe defines.h, maybe mk64.h

If wanting it only in the local file, constexpr, const char*, or #define with an #unfdef at the end of the file are the best options.

[fabienr]

The "spaghetti.o2r" is also used in src/port/Engine.cpp.
So I went further and start thinking how this could be generic for all games.

That can work but why use macro and not static char array ?

To be able to use globally without thinking about extern stuff. If we declare the variable in a file, then other files must use extern to reference it. I should check how it compiles if in the same file we have the extern declaration (from header) and the const variable. Note I didn't audit header and how they are included.

But this complexity just to gain a few byte ?
I guess the compiler may also deduplicate those strings in the binary.

What about using define in src/port/Engine.h, like this :
#define GAME_ASSET_FILE "mk64.o2r" #define ENGINE_ASSET_FILE "spaghetti.o2r"

The issue is this is global for the whole code-base. If that's what we want that's fine. But modmanager.h isn't the best place for this. Maybe defines.h, maybe mk64.h

If wanting it only in the local file, constexpr, const char*, or #define with an #unfdef at the end of the file are the best options.

I'm thinking how this could be generic for all games. Using a define could be done from cmake also. All games share the same construct with engine and game assets.

[coco875]

For string see how we have done for path of asset, we never use extern thanks to the static key word, also compiler have optimisation in release to de-duplicate string

[MegaMech]

This specific situation is so trivial I don't think it's necessary to think about all games unless you want to put in the work of PR'ing to all of them. Some projects may decide they don't want the change.

Engine.h is also a good place. I don't think I considered it earlier, but it's also an applicable place.

I think we've mentioned a number of good solutions. Please pick one. #define for engine.h is okay. constexpr is okay, if static const char* works across multiple files that's fine too.

I think coco's suggestion is good.

[fabienr]

Sorry I'm lagging on this PR. I'm sick thus, I didn't hack yesterday and I'm a bit slow.
I wanted to (re)learn a little about static, const char* vs char* const, char* vs [] ...

A static is ok and builds fine. Without static, it complains about duplicate symbols. I didn't try to define an extern.

If we build with static, then we have twice spaghetti.o2r, one for ModManager and one for Engine.
mk64.o2r is only used in ModManager.

Atm I'm building at -O0 so I want to build in prod just to know if clang will also duplicate strings.
Again I don't think that matters a lot but still I would like to check and understand.

If we build with #define then we only have one occurrence of each string.

I used this command to check for those strings (on OpenBSD but I guess the same applies to Linux also):
objdump -s -j .rodata Spaghettify | xxd -r > rodata

I know I started this bikeshed, but I do not have any strong opinions.
Just one remark, if we go this way we should also use the same variable (or macro) in Engine.cpp.

Considering assets are variables (with memory alignment?) I think we should do the same for coherence purposes.
Then for the correct header to put such strings, I can't really advise. Atm it's only for Engine and ModManager so using a local header looks ok but I can't tell if this is a good option for future and other HM games.

Regarding other games, it seems to me some of the code could be factorised and shared. I can't tell how far I would invest my time in this project. At least, I want to fix audio on OpenBSD and all games share the same issues:
SpaghettiKart always does bounds check in src/audio/synthesis.c L488 but others crash (out of bound read).
All have choppy audio and sound isn't as good as an emulator (tested with mupen64plus).

Conclusion. I think we all agree for:

static const char game_asset_file[] = "mk64.o2r";
static const char engine_asset_file[] = "spaghetti.o2r";

Then the only question remaining is how we share this variable (in which header) with Engine.cpp and ModManager.cpp ?

[coco875]

Put it in Engine.h for now and we will move if necessary, it's just a variable