Skip to content

Conversation

@Desour
Copy link
Member

@Desour Desour commented Sep 13, 2022

  • Big refactor of the client-side sound manager, and other things related to sounds. See below for a list of changes.
  • Does this relate to a goal in the roadmap: Most of the PR can be considered internal code refactoring.
  • PR contains bugfixes.

PR is based on:

Changes

This PR includes the following changes (list may miss minor changes that I forgot):

Review help

  • The client/sound* files contain the most real changes. Everything else is essentially just adjusted to the new interface.
    You can compare behaviour of sound_openal_internal.h/cpp with the old sound_openal.cpp, but idk if it helps.
    Notice that the new interface handles client sound handles differently.
  • client/client.h/cpp contains two interesting changes: It creates sound groups based on base names, and m_sounds_client_to_server now owns client-handles.
  • game.cpp: GameOnDemandSoundFetcher was moved to client/sound.cpp. Also, sounds are paused here.
  • guiEngine.h/cpp: Uses no more raw ptrs (creates some code-change-noise, but not really much). MenuMusicFetcher is simplified, by using base-class functions. GUIEngine::playSound is removed, it's only used by mainmenu scripting.
  • clientpackethandler.cpp: Uses ids. And time_offset is added.
  • l_sound.h/cpp: Is now l_mainmenu_sound.h/cpp.
  • The scripting changes are much code, but very simple. It's mostly copied code and boilerplate code for userdata handles. Otherwise, it just has the SoundSpec renames, and it uses the new id system.
  • server.cpp: Sends time_offset.
  • src/sound.h: The use_local_fallback is currently unused. But it's passed to the sound manager and should be functional, if the sound wasn't played before.
  • Everything else is just trivial renaming (SoundSpec) and smart-ptrs.

To do

This PR is a Ready for Review.

How to test

  • Just play and look if there are any warnings or similar.
  • Walk to listen to footsteps.
  • Punch.
  • Press esc (to pause sounds).
  • Use the devtest jukebox, e.g. to play a technic track from your sound-pack: https://wiki.minetest.net/Sound_Packs#Technic
  • /set sound_doppler_factor Edit: Doppler effect is disabled, and this setting does not exist.
  • Drive around in cart.
  • To enable hrtf: $ alsoft-config -> HRTF -> HRTF mode: Always on.
  • Use the entity spawner item in devtest to spawn a racecar. Give it a name with the branding iron. Play a sound attached to it with the jukebox. Or attach yourself to it with the object attacher devtest tool.

@Zughy Zughy added Roadmap The change matches an item on the current roadmap Bugfix 🐛 PRs that fix a bug Sounds Feature ✨ PRs that add or enhance a feature Code quality @ Script API labels Sep 13, 2022
@MisterE123
Copy link
Contributor

MisterE123 commented Sep 13, 2022

I hope this gets merged. I recognized several bugs from the description that I was annoyed at in the game without realizing it.

@appgurueu
Copy link
Contributor

Sounds are loaded lazily. This makes the client connect much faster.

Does this introduce any significant delay in sound playback?

@rubenwardy
Copy link
Contributor

rubenwardy commented Sep 14, 2022

Does this introduce any significant delay in sound playback?

Yeah, sounds shouldn't be lazy loaded. Only music should be. Mods should then choose between music and sound based on what kind of latency then can tolerate

However, I suspect they mean lazy loading from client cache on disk, rather than over the network, which isn't as bad

@sfan5
Copy link
Member

sfan5 commented Sep 14, 2022

I think Minetest should automatically decide whether to pre-load sounds, quoting #10603 (comment):

There's a good reason to preload short sounds, but not music or ambient textures, because that's a ton of decoding work and wasted RAM.

Game engines usually give you both options. Unity for example lets you choose between preloaded ADPCM in memory for short clips, and Vorbis decoded on the fly for longer sounds. We could autodetect this based on the clip length, something like 10 seconds should disqualify a sound from being preloaded.

The first few seconds of streamed clips could still be cached to give the decoder some time to build a buffer.


other feedback as already written on IRC:

  • at the very least the "I happened to touch this code and made it better" changes (for code unrelated to sound) should be in a separate commit
  • also the addition of ServerSoundRef

@Desour
Copy link
Member Author

Desour commented Sep 14, 2022

Sounds are loaded lazily. This makes the client connect much faster.

Does this introduce any significant delay in sound playback?

(The loading I was referring to was just the decoding from ogg vorbis data (from ram, or sound-pack; not from network) to pcm data, and giving that data to OpenAL.)
There's no delay in the sound playback. The client freezes until the sound is loaded (for much less than 1/60 second, if it's not too many new sounds at once).
The sounds are also never de-loaded.
So, the only thing that is worse because of not pre-loaded sounds is that, if you play many different sounds, that the client hasn't ever played yet during the current game session, the client can freeze for a second or so. This is not too likely to happen, and it's not that big of a problem, imo. Therefore, I came to the conclusion that such an option is currently not worth the implementation effort.


I'll split the PR and make it more reviewable.

@Zughy Zughy added the WIP label Sep 17, 2022
@Desour Desour force-pushed the sound_stuff branch 2 times, most recently from c5b592b to 1dc0a32 Compare September 21, 2022 01:05
@Desour
Copy link
Member Author

Desour commented Sep 21, 2022

I consider this PR ready for review now.

It would make sense to review and merge the dependencies first though.

@Desour Desour marked this pull request as ready for review September 21, 2022 18:40
@Zughy Zughy added Waiting (on dependency) Waiting on another PR or external circumstances (not for rebases/changes requested) and removed WIP labels Sep 21, 2022
@Desour
Copy link
Member Author

Desour commented Sep 27, 2022

Rebased.
And also positional stereo sounds are played now, to keep backwards compatibility. They are still broken (=non-positional) tough. There's a warning.

@Desour
Copy link
Member Author

Desour commented Sep 30, 2022

Rebased.

@Desour
Copy link
Member Author

Desour commented Oct 1, 2022

Rebased.

@ghost
Copy link

ghost commented Oct 28, 2022

I hope this gets merged soon. One of the biggest issues that the minetest engine has, is that playing soundtracks is very taxing for some reason.

@sfan5 sfan5 added the Rebase needed The PR needs to be rebased by its author label Dec 4, 2022
@Desour
Copy link
Member Author

Desour commented Jun 15, 2023

Rebased (+ 2 commits). (The now missing util/Optional.h would've failed silently FYI. Might be worth re-running CI on some other PRs.)

Copy link
Member

@sfan5 sfan5 left a comment

Choose a reason for hiding this comment

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

I suppose the "just merge it and see" approach is best for this

@Desour
Copy link
Member Author

Desour commented Jun 15, 2023

Looked a last time over it, and removed some trivial changes left over from a rebase. Should be fine.

@Desour Desour merged commit edcbfa3 into luanti-org:master Jun 16, 2023
@Desour Desour deleted the sound_stuff branch June 16, 2023 18:15
@Wuzzy2 Wuzzy2 mentioned this pull request Jun 17, 2023
Wuzzy2 pushed a commit to Wuzzy2/minetest that referenced this pull request Jul 31, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment