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

Emscripten crossbuild support #359

Open
wants to merge 21 commits into
base: master
Choose a base branch
from
Open

Emscripten crossbuild support #359

wants to merge 21 commits into from

Conversation

SimplyLiz
Copy link
Contributor

@SimplyLiz SimplyLiz commented Feb 17, 2020

Fixes #114

Also see #114 for build instructions. It's quite complicated :- /

@SimplyLiz SimplyLiz requested review from AnotherFoxGuy and a user February 24, 2020 14:29
@SimplyLiz SimplyLiz changed the title [WIP] Emscripten Emscripten crossbuild support Feb 24, 2020
Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

I will try to see if I can get it to work, and also try to make it better! But overall great work into making this come back

if (CROSSBUILD_EMSCRIPTEN)
# Turn off not features that are not compatible with emscripten

include(~/.conan/data/emsdk_installer/1.39.6/bincrafters/stable/package/1492a59deb6efdf29776a5734de63fc5f5c58650/upstream/emscripten/cmake/Modules/Platform/Emscripten.cmake )
Copy link

Choose a reason for hiding this comment

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

I'm not sure this is good practice...

Copy link

Choose a reason for hiding this comment

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

The hash is likely to change... We should use the proper conan way to do this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right about both, but i don't know how to do this with conan. Maybe @AnotherFoxGuy could help us out here?

set(CMAKE_MODULE_LINKER_FLAGS "${CMAKE_EXE_LINKER_FLAGS} --no-check-features")

#Debug information
#set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} --profiling-funcs -Oz -g4 -s PTHREAD_POOL_SIZE=8 -s DEMANGLE_SUPPORT=1 -pthread -s DISABLE_EXCEPTION_CATCHING=0 -s ASSERTIONS=1 -s USE_PTHREADS=1 -s USE_SDL_IMAGE=2 -s USE_SDL_TTF=2 -s USE_ZLIB=1" )
Copy link

Choose a reason for hiding this comment

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

You should probably remove this comment

Suggested change
#set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} --profiling-funcs -Oz -g4 -s PTHREAD_POOL_SIZE=8 -s DEMANGLE_SUPPORT=1 -pthread -s DISABLE_EXCEPTION_CATCHING=0 -s ASSERTIONS=1 -s USE_PTHREADS=1 -s USE_SDL_IMAGE=2 -s USE_SDL_TTF=2 -s USE_ZLIB=1" )

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I left this in because those are the flags that can be used to debug emscripten. Instead of removing the command, we should check for CMAKE_BUILD_TYPE=Debug and use the aproprate flags instead.

@@ -42,12 +42,14 @@ void SIG_handler(int signal)
exit(1);
}

#else
// #else
Copy link

Choose a reason for hiding this comment

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

I think you should remove this comment

Suggested change
// #else

@@ -370,3 +358,46 @@ template <typename MQType, typename Visitor> void Game::LoopMain(GameContext &co
// @todo: Call shutdown() here in a safe way
}
}

void gameLoop(void* arg_)
Copy link

Choose a reason for hiding this comment

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

It's good that you were able to get it to work this way. However I don't think this is the right solution. If Emscripten truly doesn't support multithreading then we can try using coroutines instead. For now, this is a good starting point

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i was sure you wouldn't be happy with this solution. (Because i'm not too) According to the documentation:

https://emscripten.org/docs/porting/emscripten-runtime-environment.html#browser-main-loop

it seems to be the only way to do this.
I think we could ifdef this?

@@ -95,6 +141,32 @@ if (USE_PACKAGE_MANAGER)
list(APPEND CMAKE_MODULE_PATH "${CONAN_LIB_DIRS_CATCH2}/cmake/Catch2")
endif ()

# these compiler options are mostly for debugging and trying to get it to work
Copy link

Choose a reason for hiding this comment

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

I think these should go in CompileOptions.cmake

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed

@restyled-io
Copy link
Contributor

restyled-io bot commented Apr 13, 2020

Hey there-

I'm a bot, here to let you know that some code in this PR might not
match the team's automated styling. I ran the team's auto-reformatting tools on
the files changed in this PR and found some differences. Those differences can
be seen in #462.

Please see that Pull Request's description for more details.

@sonarcloud
Copy link

sonarcloud bot commented Apr 13, 2020

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities (and Security Hotspot 0 Security Hotspots to review)
Code Smell A 7 Code Smells

0.0% 0.0% Coverage
0.0% 0.0% Duplication

@lizzyd710 lizzyd710 added the build Build related label Jul 18, 2022
@lizzyd710 lizzyd710 added the Stale label Aug 3, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build Build related Stale
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Port Cytopia to emscripten
2 participants