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

Bump vcpkg and favour fluidsynth #2322

Merged
merged 9 commits into from
May 7, 2023
Merged

Conversation

TheCycoONE
Copy link
Member

@TheCycoONE TheCycoONE commented Apr 23, 2023

vcpkg dependencies are bumped. The new set is listed below.

SDL2_Mixer has some vcpkg changes. native_midi is no longer an option when installing through vcpkg. This is a packaging change, not a code change upstream. Manually adding the dependency dlls for sdl2_mixer is also no longer required, they are pulled in properly on build now.

Since fluidsynth is now the preferred midi engine for CorsixTH and it only works if a soundfont is loaded, some initialization code to find soundfonts has been added. It is expected that we will be able to ship one with the installer for the next release version.

To give us more options in case of problems with appveyor a windows github action pipeline was added.

vcpkg packages:

brotli 1.0.9#5
bzip2 1.0.8#4
catch2 3.3.2
dirent 1.23.2#2
ffmpeg 5.1.2#6
fluidsynth 2.3.2#2
freetype 2.12.1#3
gettext 0.21.1
glib 2.76.1#1
libffi 3.4.4#1
libflac 1.4.2
libiconv 1.17#1
libmodplug 0.8.9.0#10
libogg 1.3.5#1
libpng 1.6.39#1
libsndfile 1.2.0
libvorbis 1.3.7#2
lpeg 1.0.2#4
lua 5.4.4#7
luafilesystem 1.8.0#4
luasec 1.3.1
luasocket 3.1.0
mp3lame 3.10
mpg123 1.31.3
openssl 3.1.0#3
opus 1.4
opusfile 0.12+20221121#1
pcre2 10.40#1
pkgconf 1.8.0#5
sdl2-mixer 2.6.3#1
sdl2 2.26.5
yasm-tool-helper 2020-03-11#1
yasm-tool 2021-12-14
yasm 1.3.0#5
zlib 1.2.13

@TheCycoONE TheCycoONE marked this pull request as draft April 23, 2023 02:31
@TheCycoONE TheCycoONE changed the title Bump vcpkg Bump vcpkg and favour fluidsynth Apr 23, 2023
@TheCycoONE TheCycoONE marked this pull request as ready for review April 23, 2023 14:16
@TheCycoONE TheCycoONE changed the title Bump vcpkg and favour fluidsynth [WIP] Bump vcpkg and favour fluidsynth Apr 24, 2023
@TheCycoONE
Copy link
Member Author

Unfortunately I cannot build release only because some of the vcpkg ports don't work with that triplet.

The first one that fails is luasocket, which was also outdated, I've submitted a PR: microsoft/vcpkg#31078

TheCycoONE added 6 commits May 3, 2023 20:14
SDL2_Mixer has some vcpkg changes. native_midi is no longer an option
so fluidsynth is now defined. Midi will no longer work without a
soundfont specified e.g. with the SDL_SOUNDFONTS environment variable.

brotli 1.0.9#5
bzip2 1.0.8#4
catch2 3.3.2
dirent 1.23.2#2
ffmpeg 5.1.2#6
fluidsynth 2.3.2#2
freetype 2.12.1#3
gettext 0.21.1
glib 2.76.1#1
libffi 3.4.4#1
libflac 1.4.2
libiconv 1.17#1
libmodplug 0.8.9.0#10
libogg 1.3.5#1
libpng 1.6.39#1
libsndfile 1.2.0
libvorbis 1.3.7#2
lpeg 1.0.2#4
lua 5.4.4#7
luafilesystem 1.8.0#4
luasec 1.3.1
luasocket 3.1.0
mpg123 1.31.3
openssl 3.1.0#3
opus 1.4
opusfile 0.12+20221121#1
pcre2 10.40#1
pkgconf 1.8.0#5
sdl2-mixer 2.6.3#1
sdl2 2.26.5
yasm-tool-helper 2020-03-11#1
yasm-tool 2021-12-14
yasm 1.3.0#5
zlib 1.2.13
Search for a soundfont in some common locations and use it if available.
Without native_midi we don't have the bug that made us disable proper
pause.
Adds the following indirect dependencies:

libsndfile 1.2.0
mp3lame 3.10
opus 1.3.1
opusfile 0.12+20221121
To cut down the build time when vcpkg cannot be loaded from cache.
@TheCycoONE TheCycoONE requested a review from Alberth289346 May 4, 2023 00:23
@TheCycoONE TheCycoONE changed the title [WIP] Bump vcpkg and favour fluidsynth Bump vcpkg and favour fluidsynth May 4, 2023
@Alberth289346
Copy link
Contributor

I don't see anything weird, although I must say most of the stuff I don't know much about.

Copy link
Member

@lewri lewri left a comment

Choose a reason for hiding this comment

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

Artefact generated for windows crashed on launch (cmd flashes then shuts) -- probably needs addressing.
Edit: Only Github Actions one crashes, appveyor is fine.

@colinjmatt will run a test MacOS build too when he has time, but the test won't block PR merge.

Other comments generally are just trivial but worth noting.


for _, sf_path in ipairs(possible_locations) do
if sf_path and lfs.attributes(sf_path) then
return sf_path
Copy link
Member

Choose a reason for hiding this comment

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

A warning would be potentially beneficial here to the user if a soundfont wasn't found. However, there is an error catch anyway at audio.lua L167, but may be catching a different kind of problem.

Though to be fair it an error shouldn't ever happen here...

Copy link
Member Author

Choose a reason for hiding this comment

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

Not finding a soundfont degrades to timidity on supporting platforms. Maybe a console message.

Copy link
Member Author

@TheCycoONE TheCycoONE May 6, 2023

Choose a reason for hiding this comment

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

actually if SDL_SOUNDFONT is defined it can not find anything here and still use freesynth; there just isn't enough information to warn at this point.

local data_dir = debug.getinfo(1, "S").source:sub(2, -12)

local possible_locations = {
self.config.soundfont or false,
Copy link
Member

Choose a reason for hiding this comment

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

Code for the relevant config file areas affected will need to be added either now or in a future PR, ideally before 0.67.

Copy link
Member Author

Choose a reason for hiding this comment

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

Preferably yes.

@lewri
Copy link
Member

lewri commented May 6, 2023

Artefact generated for windows crashed on launch (cmd flashes then shuts) -- probably needs addressing.
Edit: Only Github Actions one crashes, appveyor is fine.

There are file/folder differences inc missng files on the github actions one (most notably all the lua is missing)
image

@TheCycoONE
Copy link
Member Author

Ok I'll add the data files. Didn't realize that appveyor made ready to play artifacts

The new action produces RelWithDebInfo artifacts that can be downloaded.
Like appveyor the vcpkg builds are cached between runs.
@lewri
Copy link
Member

lewri commented May 6, 2023

Just to note -- this is what happens when there isn't a soundfont target found. The game hangs for quite a bit when the movie ends/skipped:

Running: The callback handler.
A stack trace is included below, and the handler has been disconnected.
Lua\audio.lua:583: Could not load music file 'Sound\Midi\ATLANTIS.XMI' (No SoundFonts have been requested)
stack traceback:
        [C]: in function 'coroutine.yield'
        Lua\app.lua:1084: in function <Lua\app.lua:1079>

An error has occurred!
Almost anything can be the cause, but the detailed information below can help the developers find the source of the error.
Running: The callback handler.
A stack trace is included below, and the handler has been disconnected.
Lua\audio.lua:583: Could not load music file 'Sound\Midi\STEADY.XMI' (No SoundFonts have been requested)
stack traceback:
        [C]: in function 'coroutine.yield'
        Lua\app.lua:1084: in function <Lua\app.lua:1079>

@TheCycoONE TheCycoONE merged commit 0765784 into CorsixTH:master May 7, 2023
@lewri lewri mentioned this pull request May 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants