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

Fix emscripten compilation #289

Merged
merged 5 commits into from
Nov 13, 2024
Merged

Fix emscripten compilation #289

merged 5 commits into from
Nov 13, 2024

Conversation

bchapuis
Copy link
Contributor

@bchapuis bchapuis commented Nov 12, 2024

Really nice project; everything has gone smoothly so far. I had to make a couple of modifications to ensure the examples compile and run in the browser (maybe this can be addressed with a parameter instead, but I'm not a cmake expert). I also added a GitHub Actions step that I haven't been able to test, as the actions are not triggered in PRs. Hope this helps.

Best regards,

Bertil

@markaren
Copy link
Owner

markaren commented Nov 12, 2024

I kinda assume that the extra ../ is due to where the binaries are located on your system vs mine. The solution would be to fix this in the CMake config.

set(CMAKE_RUNTIME_OUTPUT_DIRECTORY ${CMAKE_CURRENT_BINARY_DIR}/bin) was supposed to this but apparantly not.

        #ifndef __EMSCRIPTEN__
        // Only run this on desktop OpenGL, not in WebGL
        glGetTexImage(GL_TEXTURE_2D, 0, gl::toGLFormat(texture.format), gl::toGLType(texture.type), data.data());
        #endif

I have not had any issues with this, are you saying it has no effect?

Edit: I'm not sure if I have ever tested this code path with emscripted at any point..

@markaren
Copy link
Owner

markaren commented Nov 12, 2024

I think it is best to keep this PR to the CI addition and potentially the changes to GLRenderer. The paths needs another type of change.

@markaren
Copy link
Owner

For some reason I thought I had to use relative paths for emscripten files, but seems I don't. So I will push a change for that.

@bchapuis
Copy link
Contributor Author

Cool, I reverted the changes to the CMakeList.txt files and moved the glGetTexImage in a dedicated commit so you can cherry pick what you need. As said, I havn't been able to test the CI job, but it would be nice to have one for emscripten.

@bchapuis
Copy link
Contributor Author

Let me know if you want me to try the change you will make for the paths.

@markaren markaren changed the base branch from master to dev November 13, 2024 07:29
@markaren
Copy link
Owner

I were going to make some changes to this PR, but I see that the PR is coming from your master branch.

Would you mind closing this PR, make a new one in a new branch based off dev?

@bchapuis
Copy link
Contributor Author

I didn't notice the dev branch. I just rebased in this PR, Is this sufficient?

@markaren
Copy link
Owner

Your master branch will diverge from upstream, but ok for me.

@markaren
Copy link
Owner

You can test these changes, and I need to figure out how to make the CI run. Or I can just merge to dev first. We'll see later

@bchapuis
Copy link
Contributor Author

Cool, the emscripten compilation went well on my mac.

@markaren markaren merged commit 5e95e0e into markaren:dev Nov 13, 2024
@markaren
Copy link
Owner

-DCMAKE_TOOLCHAIN_FILE=${{env.EM_CACHE_FOLDER}}/libexec/cmake/Modules/Platform/Emscripten.cmake does not seem to be correct.

@markaren
Copy link
Owner

This works:

  linux-emscripten:

    runs-on: ${{ matrix.os }}
    env:
      CC: gcc-${{ matrix.compiler_version }}
      CXX: g++-${{ matrix.compiler_version }}
    strategy:
      fail-fast: false
      matrix:
        os: [ ubuntu-22.04 ]
        compiler_version: [ 11 ]

    steps:
      - uses: actions/checkout@v3

      - name: Install prerequisites
        run: |
          sudo apt-get update && sudo apt-get install -y \
          libxinerama-dev \
          libxcursor-dev \
          xorg-dev \
          libglu1-mesa-dev \
          pkg-config

      - uses: mymindstorm/setup-emsdk@v14
        with:
          version: 3.1.71
          actions-cache-folder: 'emsdk-cache'

      - name: Configure and build
        run: |
          cmake . -B build -DCMAKE_TOOLCHAIN_FILE=${{env.EMSDK}}/upstream/emscripten/cmake/Modules/Platform/Emscripten.cmake -DTHREEPP_BUILD_TESTS=OFF -DCMAKE_BUILD_TYPE="Release"
          cmake --build build

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.

2 participants