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

Add build target "web" #9406

Merged
merged 27 commits into from
Jan 5, 2025
Merged

Add build target "web" #9406

merged 27 commits into from
Jan 5, 2025

Conversation

ololoken
Copy link
Contributor

@ololoken ololoken commented Jan 2, 2025

Prev PR for reference
Demo no files bundled and user can provide own sets of maps, music, mods;

@oleg-derevenetz would you check how synchronous playback is done in AsyncManager::notifyWorker, smelly workaround but it works;

BTW ci/cd pipeline can be done using gh-pages

@ihhub
Copy link
Owner

ihhub commented Jan 3, 2025

Hi @ololoken , please fix code style issues in the code to make the pipeline happy.

@ololoken
Copy link
Contributor Author

ololoken commented Jan 3, 2025

@ihhub Well, I didn't find better way than disable linter checks for places where js code is mixed in cpp files.

@oleg-derevenetz
Copy link
Collaborator

Hi @ololoken please don't do these force-pushes. They completely break my changes.

@ololoken
Copy link
Contributor Author

ololoken commented Jan 3, 2025

huh looks like push fork -f was too much

ok, it's late in my location so, feel free to add your changes. I'll get back tomorrow

ЗЫ можешь меня на lindkedin добавить? Roman

@oleg-derevenetz
Copy link
Collaborator

oleg-derevenetz commented Jan 3, 2025

Hi @ololoken please take a look at #9407. I've changed the AsyncManager a bit for the better and minimized the volume of the Emscripten-dependent C++ code by moving the sync operation to a single place (see my comments in that PR). Also now it should pass all the checks, so you can grab my changes to your PR and make further changes based on them.

ЗЫ можешь меня на lindkedin добавить? Roman

Я не использую linkedin уже давным-давно :) Даже учетку не вижу смысла восстанавливать чтобы войти.

@ololoken
Copy link
Contributor Author

ololoken commented Jan 3, 2025

@ihhub LGTM

Who is responsible for pipelines?

  make-web:
    runs-on: ubuntu-latest
    steps:
      - name: Checkout ↓️
        uses: actions/checkout@v4

      - name: Install and Build ⚙
        run: |
          docker run --rm -v $(pwd):/src emscripten/emsdk:3.1.74 sh -c 'apt update && apt install -y gettext && emmake make  -f Makefile.web'

      - name: Deploy 🚀
        uses: JamesIves/github-pages-deploy-action@v4
        if: ${{ github.event_name == 'push' }}
        with:
          branch: fheroes2-gh-pages # The branch the action should deploy to.
          folder: src/dist/web/dist # The folder the action should deploy.

This requires enabled gh-pages in repo settings. The result is deployed build available via public url https://ihhub.github.io/fheroes2/

@ihhub
Copy link
Owner

ihhub commented Jan 3, 2025

Hi @ololoken ,

every developer can contribute to GitHub Actions pipeline. Since you are the first time contributor you aren't allowed to run the pipeline as per GitHub rules.

In order to add a new stage you need to add a new file inside .github/workflows directory and modify pull_request.yml and push.yml files.

Additionally, you don't need to explicitly use docker commands. All GitHub Runners are Docker based images.

@ihhub ihhub added improvement New feature, request or improvement GitHub Actions GitHub Actions CI labels Jan 3, 2025
@oleg-derevenetz
Copy link
Collaborator

oleg-derevenetz commented Jan 3, 2025

@ihhub LGTM

Who is responsible for pipelines?

  make-web:
    runs-on: ubuntu-latest
    steps:
      - name: Checkout ↓️
        uses: actions/checkout@v4

      - name: Install and Build ⚙
        run: |
          docker run --rm -v $(pwd):/src emscripten/emsdk:3.1.74 sh -c 'apt update && apt install -y gettext && emmake make  -f Makefile.web'

      - name: Deploy 🚀
        uses: JamesIves/github-pages-deploy-action@v4
        if: ${{ github.event_name == 'push' }}
        with:
          branch: fheroes2-gh-pages # The branch the action should deploy to.
          folder: src/dist/web/dist # The folder the action should deploy.

This requires enabled gh-pages in repo settings. The result is deployed build available via public url https://ihhub.github.io/fheroes2/

BTW I don't see the sdl2_mixer.py call here. Is this file still needed? In general, I thought that Emscripten has its own set of ports with (already patched) popular libraries, and scripts of this kind are no longer needed.

@ololoken
Copy link
Contributor Author

ololoken commented Jan 3, 2025

@oleg-derevenetz dropped sdl2_mixer.py, left note how to build web version with enabled pthread.
@ihhub it's just dind(docker in docker call) image with baked emsdk gets pulled mounted do it's job.

Local build is stable an redeployed here https://turch.in/fheroes2/public/

I will continue work on:

  1. UI of html launcher: buttons, general styling.
  2. Zip archives support for saves/settings imports/exports.

@oleg-derevenetz
Copy link
Collaborator

oleg-derevenetz commented Jan 3, 2025

Hi @ololoken

I implemented a few changes (please see my latest commit). Briefly:

  1. I added the CI step (no gh pages so far, we still need to think about how to do it better, just the ZIP archive);
  2. Renamed "web" to "emscripten", "web" is a bit too broad IMHO, because there are alternatives to emscripten to build C++ code to wasm;
  3. Made the build system more similar to other platforms (no translation generation in the src/dist, etc).

I have a question BTW - I was unable to get your demo to work. It asks for a directory, I send it the directory with DATA and MAPS folders like this:

D:\Temp\homm2

03.01.2025  18:53    <DIR>          .
03.01.2025  18:53    <DIR>          ..
03.01.2025  18:03    <DIR>          DATA
03.01.2025  18:03    <DIR>          MAPS

D:\Temp\homm2\DATA

03.01.2025  18:03    <DIR>          .
03.01.2025  18:03    <DIR>          ..
09.03.2021  15:49        43 363 026 HEROES2.AGG
09.03.2021  15:49         2 598 847 HEROES2X.AGG

D:\Temp\homm2\MAPS

03.01.2025  18:03    <DIR>          .
03.01.2025  18:03    <DIR>          ..
09.03.2021  15:49           139 387 BROKENA.MP2

and... nothing happens. Only these:

fheroes2.js:617 still waiting on run dependencies:
fheroes2.js:619 dependency: syncfs
fheroes2.js:622 (end of list)

in the browser console. Why is that?

P.S. In any case, if there will be any fixes for this issue, I suggest to wait for merging this PR and then address this issue later separately in new PRs.

Hi @ihhub

I suggest the following - now we can do a review and merge this PR (if review is OK) so that @ololoken will get his contributor badge and and there will be no need to run CI manually anymore. Then he will be able to gradually add new functionality via individual PRs from separate branches.

@ololoken
Copy link
Contributor Author

ololoken commented Jan 3, 2025

@oleg-derevenetz expected directory structure:
/data/*.agg

@oleg-derevenetz
Copy link
Collaborator

@oleg-derevenetz expected directory structure: /data/*.agg

Yes, I specify my D:\Temp\homm2 folder which has two subfolders - DATA and MAPS. DATA has both AGG files. Should I specify the D:\Temp\homm2\DATA folder instead? How about maps in this case?

@ololoken
Copy link
Contributor Author

ololoken commented Jan 3, 2025

@oleg-derevenetz
I have concerns only about no translation generation in the src/dist. Language files must be prepared before em linker spawns, as linker creates static files bundle with languages font and h2d file

@oleg-derevenetz oleg-derevenetz requested a review from ihhub January 3, 2025 17:34
@oleg-derevenetz oleg-derevenetz added the WASM WASM version of the engine label Jan 3, 2025
@oleg-derevenetz
Copy link
Collaborator

oleg-derevenetz commented Jan 3, 2025

I suppose we will be able to bring threads back when emscripten-core/emscripten#23094 will be merged. Despite the fact that everything is more or less working now (except the MIDI playback), the logic of resuming background music is not working correctly, because it relies on a certain order of execution of interrelated actions due to synchronization between threads, namely - when playback of a music track is stopped, the stopping thread holds a mutex while it remembers the position of the music track and makes a proper cleanup, and only when this mutex is released, the other thread intended to restart the music track gets the chance to run. Now the logic is reversed (sort of) because there are no threads and no proper synchronization. But this issue is a relatively minor one.

@ihhub
Copy link
Owner

ihhub commented Jan 4, 2025

Hi @ololoken , please revert your last commit 37d06f1. Using any resources from the original game is not permitted.

Also, let's leave beautification part for later.

Copy link
Owner

@ihhub ihhub left a comment

Choose a reason for hiding this comment

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

Hi @ololoken , I left 2 more questions here. Could you please check them?

files/emscripten/fheroes2.jpeg Outdated Show resolved Hide resolved
files/emscripten/index.html Show resolved Hide resolved
@zenseii
Copy link
Collaborator

zenseii commented Jan 4, 2025

@oleg-derevenetz

P.S. Here is the ZIP archive (I'll remove the link later):

[removed link]

I suggest deleting the edit history of this post because it still contains the link.

@oleg-derevenetz
Copy link
Collaborator

oleg-derevenetz commented Jan 4, 2025

MIDI music playback also works now. Now we can think about deploying the WASM engine with a demo version on GitHub Pages so that anyone can come and play the demo version (but first we need to resolve the issue with SDL_mixer and pthreads, because currently everything is single-threaded and the music chokes when the main thread is busy with something).

Hi @ihhub what specific issues are left with this PR? Let's solve them and merge this PR, so that @ololoken and, to some extent, I could gradually work on new WASM aspects in separate PRs. @ololoken will get the contributor badge and there will be no need for us to manually confirm all CI runs.

@ololoken
Copy link
Contributor Author

ololoken commented Jan 4, 2025

latest build of this branch is deployed to https://turch.in/fheroes2/public/

@oleg-derevenetz
Copy link
Collaborator

oleg-derevenetz commented Jan 4, 2025

Hi @ololoken

latest build of this branch is deployed to https://turch.in/fheroes2/public/

It looks like it is not the latest build yet - it still calls extra FS.syncfs() (resolved in 9cbb468) and doesn't support the MIDI playback (resolved in ae788dd). Try to pull the latest changes and rebuild.

@ololoken
Copy link
Contributor Author

ololoken commented Jan 4, 2025

Indeed, an old build was uploaded, thanks. Updated.

@ihhub
Copy link
Owner

ihhub commented Jan 5, 2025

HI @ololoken and @oleg-derevenetz , I solved the remaining problems with files:

  • updated the image of a smaller size
  • reverted 37d06f1 commit as it violated copyright

Please take a look.

@oleg-derevenetz
Copy link
Collaborator

Hi @ihhub

  • reverted 37d06f1 commit as it violated copyright

Why is this commit violated copyright? Because of the fonts?

@ihhub
Copy link
Owner

ihhub commented Jan 5, 2025

Hi @ihhub

  • reverted 37d06f1 commit as it violated copyright

Why is this commit violated copyright? Because of the fonts?

Yes, the fonts were originated from the original in-game fonts.

@oleg-derevenetz
Copy link
Collaborator

Yes, the fonts were originated from the original in-game fonts.

Ah, OK then.

@ihhub ihhub merged commit 076150e into ihhub:master Jan 5, 2025
21 checks passed
@ihhub
Copy link
Owner

ihhub commented Jan 5, 2025

@ololoken , huge thanks for the contribution to the project!

@ihhub ihhub mentioned this pull request Jan 5, 2025
12 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
GitHub Actions GitHub Actions CI improvement New feature, request or improvement WASM WASM version of the engine
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants