search

ReMinecraftPE / mcpe

ReMinecraftPE - A custom experience based on Minecraft PE as of 2011.
https://discord.gg/UKGhuKNmFu
Other
335 stars 50 forks source link

Clean Up CMake And Add GitHub Actions #82

Closed TheBrokenRail closed 11 months ago

TheBrokenRail commented 11 months ago

The CMake build system was a bit of a mess, so I cleaned it up.

What I mean by this, is that there were multiple cases of folders that should have been their own CMake projects instead being pulled in directory other CMake projects. This created un-intuitive[^1] behavior like platforms/emscripten/AppPlatform_emscripten.cpp being part of the platforms/sdl project, rather than a non-existent platforms/emscripten project. While definitely subjective, this seemed quite messy to me.

I made the following changes to remedy this:

I also fixed #81, which was an odd bug. For some reason thirdparty/raknet/RakNetDefinesOverrides.h included source/common/Utils.hpp, which required SDL. For obvious reasons[^2], the RakNet project doesn't have a dependency on SDL, so the header file requiring SDL caused a build error[^3]. Removing the include didn't seem to break anything, so that's what I did.

I also added a bare-bones GitHub Actions pipeline that just runs the desktop SDL and the WASM build.

[^1]: Most CMake projects put source-code inside the project's folder. For instance, if you have a CMake project located in /data, every source file used will be inside that folder as well. To be more precise, /data/CMakeLists.txt depending on /data/main.cpp or /data/src/util.cpp would be normal, but /data/CMakeLists.txt depending on /other/util.cpp would not be.

[^2]: RakNet is a networking library and should not need to depend on SDL.

[^3]: Counter-intuitively, this doesn't actually break a standard Linux build. This is because Linux just shoves all the headers into /usr/include, making everything already in the include path by default. Meanwhile, Emscripten only puts libraries in the include path that the user asks for, and since RakNet doesn't ask for SDL, it's not included in the path.

iProgramMC commented 11 months ago

Are you able to let me know how to use the desktop SDL project? I couldn't figure it out, that's why I rolled out my own makefile. But I think it will work better.

TheBrokenRail commented 11 months ago

Just do:

cd platforms/sdl
mkdir build
cd build
cmake ..
make -j$(nproc)
./reminecraftpe
iProgramMC commented 11 months ago

Do you mind rebasing the project?

TheBrokenRail commented 11 months ago

OK, I've done my best to rebase my changes. (Also, why are PRs being sent to master instead of devel?)

Off-topic, but after seeing all the cursed nonsense needed for Windows and VS, I've come to the conclusion that both are cursed abominations that need to be put out of their misery.

iProgramMC commented 11 months ago

why are PRs being sent to master instead of devel?

Because devel is a branch where I can work on whatever the hell I want without breaking master.

[...] need to be put out of their misery.

Problem is I use Windows, and many other people use Windows.

BTW, sorry for not letting you know earlier, but I decided to actually move the emscripten stuff back into the SDL directory, of course conditionally compiled in through the CMakeLists.txt for the SDL build. I don't agree with compartmentalizing every single thing, and I especially don't agree with having one project for one *.cpp file, unless it's an external library, which the SDL stuff is not.

TheBrokenRail commented 11 months ago

Problem is I use Windows, and many other people use Windows.

Yeah, I understand it's necessary. I can still hate it though.

I don't agree with compartmentalizing every single thing, and I especially don't agree with having one project for one *.cpp file, unless it's an external library, which the SDL stuff is not.

There were two "good" options:

  1. Everything is contained in the reminecraftpe-sdl project. This project would select whether to compile the desktop or WASM AppPlatform_sdl.cpp in CMakeLists.txt.
  2. Make the desktop and WASM AppPlatforms their own project, so they can each have their own header file with the same name. This has the unfortunate consequence of requiring that AppPlatform_sdl_base be its own library so the AppPlatforms can link to it.

Ultimately, I went with option 2 because of personal preference.

Personally, I would much prefer just having one AppPlatform_sdl that switched behavior with #ifdefs like in my original PR, but it seems that I'm alone in that considering it was split off into the AppPlatform_sdlbase/sdl/emscriptem system shortly after my PR was merged.

iProgramMC commented 11 months ago

I can still hate it though.

Of course, I dislike Windows as a development platform as well, because it's much more involved to install libraries and stuff. Unfortunately it is pretty much a requirement if we don't want to exclude folks who simply don't know how to use Linux.

I'm alone in that considering it was split off into the AppPlatform_sdlbase/sdl/emscriptem system shortly after my PR was merged.

Yes, Brent apparently wasn't happy with that. Unfortunately we have to compromise.

BrentDaMage commented 11 months ago

Using polymorphism and abstraction is much easier to manage than a bunch of #ifdefs in one big file.

TheBrokenRail commented 11 months ago

Well that's what this PR does. The unfortunate reality is that implementing that nicely requires a few small CMake projects.

iProgramMC commented 11 months ago

No. Just leave the SDL stuff together combined into one CMake project and it should be good.

BrentDaMage commented 11 months ago

Well that's what this PR does. The unfortunate reality is that implementing that nicely requires a few small CMake projects.

If it means not making a massive gargantuan file fill of #ifdefs, then I'm all for it.

BrentDaMage commented 11 months ago

Or just leave what we have. As long as no one has combined all of the SDL classes into one big file with a bunch of #ifdefs, I'm fine with however it is.

TheBrokenRail commented 11 months ago

I've combined the reminecraft-sdl-* CMake projects into the main reminecraftpe project. The Emscripten and desktop implementations of AppPlatform_sdl are still separate files however.

TheBrokenRail commented 11 months ago

I have rebased this PR... again.

iProgramMC commented 11 months ago

Isn't the SDL layout already good enough? The emscripten part is compiled conditionally.

TheBrokenRail commented 11 months ago

The changes to platforms/sdl are only a small part of this PR.

It also:

  1. Sets up GitHub Actions for automatic build testing.
  2. Makes platforms/openal its own CMake project.
  3. Updates the version of Emscripten used in build-wasm.sh.
  4. Makes platforms/sdl/CMakeLists.txt use game/assets as the default assets directory just like the other builds.
  5. Fixes MCPE just ignoring the first mouse click.
  6. Allows building MCPE without sound data (needed for the CI build).
  7. Updates getBestScaleForThisScreenSize to handle windows taller than 1600 pixels.
  8. Removes oddities like set(CMAKE_BUILD_TYPE Debug) and #undef main.