Skip to content

Feature/protobuf savedata#370

Merged
SaintWish merged 8 commits into
MSRevive:st-protofrom
PuppetMasterHex:feature/protobuf-savedata
May 6, 2026
Merged

Feature/protobuf savedata#370
SaintWish merged 8 commits into
MSRevive:st-protofrom
PuppetMasterHex:feature/protobuf-savedata

Conversation

@PuppetMasterHex
Copy link
Copy Markdown

  • Added protobuf save feature and made it the new default
    • Saving -> ProtoSave::SaveCharProtobuf
    • Loading -> ProtoSave::LoadCharProtobuf
  • Protobuf can export to JSON -> ProtoSave::SaveCharJSON
  • Compatible with old save files
    • First protobuf tries to parse the data
    • If it fails there is a fallback to the old save loader
  • Added dockerfile for easy compiling and testing
    • Can volume mount hlds in the container and run the server there
    • docker/run_server.sh copies the ms.so and config files before starting hlds for faster testing
  • Minor improvements
    • Compile multithreaded with -j8 on linux
    • Made msstring::c_str() const

@SaintWish
Copy link
Copy Markdown
Member

Sorry for the delay I'll get to looking at this soon, been busy these last few days.

Copy link
Copy Markdown
Member

@SaintWish SaintWish left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems windows fails to compile because of the find_package for protobuf (https://github.com/MSRevive/MasterSwordRebirth/actions/runs/23979449975/job/70516350440?pr=370)

Comment thread src/game/server/save/protosave.h
Comment thread src/game/server/save/protosave.cpp
Comment thread cmake/LinuxToolchain.cmake
@tschumann
Copy link
Copy Markdown
Contributor

tschumann commented Apr 9, 2026 via email

@SaintWish
Copy link
Copy Markdown
Member

Still a few todo debugs as well.

Also, what are the URLs in the .cfg files?

On Thu, 9 Apr 2026 at 13:23, Saint Wish @.***> wrote:

@.**** requested changes on this pull request.

Seems windows fails to compile because of the find_package for protobuf (
https://github.com/MSRevive/MasterSwordRebirth/actions/
runs/23979449975/job/70516350440?pr=370)

On src/game/server/save/protosave.h
#370 (comment)
:

That license at the top would restrict anyone but you from ever modifying
this code, and is incompatible with the HL SDK and GPL3.0 license.

On src/game/server/save/protosave.cpp
#370 (comment)
:

That license at the top would restrict anyone but you from ever modifying
this code, and is incompatible with the HL SDK and GPL3.0 license.

Instead of using the C++ std filesystem please use the engine's filesystem
https://github.com/MSRevive/MasterSwordRebirth/blob/dev/
src/game/shared/filesystem_shared.h

On cmake/LinuxToolchain.cmake
#370 (comment)
:

I'm not sure why the compiler and compile flags are set here when they
were already set here https://github.com/MSRevive/
MasterSwordRebirth/blob/dev/CMakeLists.txt


Reply to this email directly, view it on GitHub
#370 (review),
or unsubscribe
https://github.com/notifications/unsubscribe-auth/AA34IYSAF2FTUJEB5HU3GTT4U4JU5AVCNFSM6AAAAACXMUMW2KVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHM2DANZZGQ3TCNZTHA
.
You are receiving this because you are subscribed to this thread.Message
ID: @.***>

Good catch!

@PuppetMasterHex PuppetMasterHex force-pushed the feature/protobuf-savedata branch from 1dfa5b8 to df8479f Compare April 9, 2026 18:16
@PuppetMasterHex
Copy link
Copy Markdown
Author

PuppetMasterHex commented Apr 9, 2026

Still a few todo debugs as well. Also, what are the URLs in the .cfg files?

I have removed the debug comments, only kept the /// @todo replace with range based for of mslist for loops, since I think that list implementation should support range based for loops :)

The .cfg files (and these URLs) come from the default .cfgs shipped with MSR, I have replaced the URLs with example.org

@PuppetMasterHex
Copy link
Copy Markdown
Author

Seems windows fails to compile because of the find_package for protobuf (https://github.com/MSRevive/MasterSwordRebirth/actions/runs/23979449975/job/70516350440?pr=370)

The WindowsToolchain.cmake includes vcpkg but I cannot find the file included there.
Most package managers support protobuf

I think cpm would also work.
What is the preferred way to add dependencies for the windows build?

@PuppetMasterHex PuppetMasterHex force-pushed the feature/protobuf-savedata branch from df8479f to d7540df Compare April 9, 2026 19:41
@SaintWish
Copy link
Copy Markdown
Member

SaintWish commented Apr 9, 2026

The preferred way for now is to just add them manually to thirdpart either just the code or if it needs to be compiled then the compiled form, like for example the libcurl. I tried using vcpkg once but had nothing but problems with it and git dependencies.

I never tried CPM though it might worth using it it works better than VCPKG
After looking at CPM, it might be worth using that since all it requires is CMake.

@SaintWish
Copy link
Copy Markdown
Member

Still a few todo debugs as well. Also, what are the URLs in the .cfg files?

I have removed the debug comments, only kept the /// @todo replace with range based for of mslist for loops, since I think that list implementation should support range based for loops :)

The .cfg files (and these URLs) come from the default .cfgs shipped with MSR, I have replaced the URLs with example.org

At some point we want to get rid of mslist and just use the appropriate standard lib option same for msstring.

Signed-off-by: Genesis <10389486+PuppetMasterHex@users.noreply.github.com>
Signed-off-by: Genesis <10389486+PuppetMasterHex@users.noreply.github.com>
@PuppetMasterHex PuppetMasterHex force-pushed the feature/protobuf-savedata branch from d7540df to adb4bff Compare April 11, 2026 13:06
@PuppetMasterHex
Copy link
Copy Markdown
Author

I never tried CPM though it might worth using it it works better than VCPKG After looking at CPM, it might be worth using that since all it requires is CMake.

I have added cmake/protobuf.cmake that uses CPM to fetch and build protobuf for windows, but I cannot test if the target_link_libaries works for $<$<PLATFORM_ID:Windows>:libprotobuf>

@PuppetMasterHex PuppetMasterHex force-pushed the feature/protobuf-savedata branch from adb4bff to 85db73d Compare April 11, 2026 13:12
@SaintWish
Copy link
Copy Markdown
Member

If it fails to build again on windows I can just merge it and work on it when I have time

@tschumann
Copy link
Copy Markdown
Contributor

tschumann commented Apr 14, 2026 via email

…onfigs

Signed-off-by: Genesis <10389486+PuppetMasterHex@users.noreply.github.com>
The data is not modified in the function so it should be const

Signed-off-by: Genesis <10389486+PuppetMasterHex@users.noreply.github.com>
@PuppetMasterHex PuppetMasterHex force-pushed the feature/protobuf-savedata branch from 85db73d to 169ac65 Compare April 14, 2026 10:38
Also committed the protoc generated files since BPM doesnt support a two stage build to build protoc before running 'cmake -B'

Signed-off-by: Genesis <10389486+PuppetMasterHex@users.noreply.github.com>
With the fedora dockerfile setting the paths manually is not necessary anymore

Signed-off-by: Genesis <10389486+PuppetMasterHex@users.noreply.github.com>
@PuppetMasterHex PuppetMasterHex force-pushed the feature/protobuf-savedata branch from 169ac65 to 5f4a8d6 Compare April 14, 2026 12:38
@PuppetMasterHex
Copy link
Copy Markdown
Author

I'm trying to fix the build for Ubuntu, but it doesn't feel sane.

The cmake find_package for opengl and protobuf is broken. It only works partially and fails to find existing libraries (like the OPENGL_gl_LIBRARY and OPENGL_glx_LIBRARY)

OTOH the fedora dockerfile at docker/Dockerfile just installs the packages and the build works out of the box.

@tschumann
Copy link
Copy Markdown
Contributor

tschumann commented Apr 15, 2026 via email

@PuppetMasterHex
Copy link
Copy Markdown
Author

If it fails to build again on windows I can just merge it and work on it when I have time

I think the easiest way to get it to work is to provide the protoc executable and then set up the target protobuf::protoc to point to that path.
Protobuf is BSD licensed so that should be possible.

Is there anything missing for this pull request?

@SaintWish
Copy link
Copy Markdown
Member

SaintWish commented Apr 30, 2026

Sorry for the long wait, it looks like it has a problem with the submodules parameter in the workflow, but I think that can probably be removed since we don't use git submodules.

For the Windows related build issues I can work on so not worried about those.

@PuppetMasterHex PuppetMasterHex force-pushed the feature/protobuf-savedata branch from f00cb33 to 023bbad Compare April 30, 2026 20:56
@PuppetMasterHex
Copy link
Copy Markdown
Author

I have removed the submodules: recursive entry for the failing build.

@PuppetMasterHex PuppetMasterHex force-pushed the feature/protobuf-savedata branch from 023bbad to 8feed30 Compare April 30, 2026 21:27
@PuppetMasterHex
Copy link
Copy Markdown
Author

PuppetMasterHex commented Apr 30, 2026

Strange in my local tests ${{github.workspace}} is the same as $GITHUB_WORKSPACE but here on github its not and checkout@v4 checks the code out in $GITHUB_WORKSPACE.

Pushed a change that uses $GITHUB_WORKSPACE, hmm github actions is really weird

Edit: seems to be a known bug that may or may not be fixed actions/runner#2058

@SaintWish
Copy link
Copy Markdown
Member

Yeah not surprising, Github actions do be weird sometimes.

Looks like that fixed that issue, now it's just missing a package it looks like?

@PuppetMasterHex PuppetMasterHex force-pushed the feature/protobuf-savedata branch 2 times, most recently from 57cea33 to c3025af Compare April 30, 2026 23:36
@PuppetMasterHex
Copy link
Copy Markdown
Author

PuppetMasterHex commented Apr 30, 2026

Yeah cmake wants to compile a test program to check gcc but it requires libatomic.x86_64
Pushed that now

First I thought it needs the 32 bit version, but since it is only for the test program, so it seems to be no problem that it links a 64 bit lib.

Edit: I really dont get why this doesn't happen locally
Edit2: Seems it needs both versions

@PuppetMasterHex PuppetMasterHex force-pushed the feature/protobuf-savedata branch from c3025af to 0c75332 Compare May 1, 2026 00:08
…of ubuntu

Signed-off-by: Genesis <10389486+PuppetMasterHex@users.noreply.github.com>
@PuppetMasterHex PuppetMasterHex force-pushed the feature/protobuf-savedata branch from 0c75332 to a2b28af Compare May 1, 2026 00:42
@PuppetMasterHex
Copy link
Copy Markdown
Author

Found why the errors don't happen locally, 3 days ago the fedora:latest tag was moved to fedora 44, after deleting my local image and starting from scratch I get these errors too.

I pushed a change that sets the image to fedora:43 for now, I'll check how to fix the errors in the next days but the build should work for now.

Also set the github actions docker image back to fedora:latest

Signed-off-by: Genesis <10389486+PuppetMasterHex@users.noreply.github.com>
@PuppetMasterHex
Copy link
Copy Markdown
Author

PuppetMasterHex commented May 2, 2026

Pushed a fix for fedora 44
I doubt that the relocations with libdiscord-rpc.a can be resolved easily, also not sure if PIE / ASLR works that well with 32bit libraries.
But at least it now compiles just like before.

@SaintWish SaintWish changed the base branch from dev to st-proto May 6, 2026 04:52
@SaintWish
Copy link
Copy Markdown
Member

LGTM, thanks. I'll merge it to it's own branch so I can work on fixing Windows build when I have time.

@SaintWish SaintWish merged commit 4f1291d into MSRevive:st-proto May 6, 2026
1 of 2 checks passed
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