Sounds: Various little improvements (#12486)

Use SimpleSoundSpec where reasonable (OpenAL)
Ensure the sound IDs do not underflow or get overwritten -> loop in u16
Proper use of an enum.

9 files changed, 112 insertions, 108 deletions

diff --git a/src/client/sound.h b/src/client/sound.h
index a3bb3ce87..213d20831 100644
--- a/src/client/sound.h
+++ b/src/client/sound.h
@@ -51,10 +51,8 @@ public:
 
 	// playSound functions return -1 on failure, otherwise a handle to the
 	// sound. If name=="", call should be ignored without error.
-	virtual int playSound(const std::string &name, bool loop, float volume,
-			float fade = 0.0f, float pitch = 1.0f) = 0;
-	virtual int playSoundAt(const std::string &name, bool loop, float volume, v3f pos,
-			float pitch = 1.0f) = 0;
+	virtual int playSound(const SimpleSoundSpec &spec) = 0;
+	virtual int playSoundAt(const SimpleSoundSpec &spec, const v3f &pos) = 0;
 	virtual void stopSound(int sound) = 0;
 	virtual bool soundExists(int sound) = 0;
 	virtual void updateSoundPosition(int sound, v3f pos) = 0;
@@ -62,15 +60,6 @@ public:
 	virtual float getSoundGain(int id) = 0;
 	virtual void step(float dtime) = 0;
 	virtual void fadeSound(int sound, float step, float gain) = 0;
-
-	int playSound(const SimpleSoundSpec &spec)
-	{
-		return playSound(spec.name, spec.loop, spec.gain, spec.fade, spec.pitch);
-	}
-	int playSoundAt(const SimpleSoundSpec &spec, const v3f &pos)
-	{
-		return playSoundAt(spec.name, spec.loop, spec.gain, pos, spec.pitch);
-	}
 };
 
 class DummySoundManager : public ISoundManager
@@ -88,16 +77,9 @@ public:
 	{
 	}
 	void setListenerGain(float gain) {}
-	int playSound(const std::string &name, bool loop, float volume, float fade,
-			float pitch)
-	{
-		return 0;
-	}
-	int playSoundAt(const std::string &name, bool loop, float volume, v3f pos,
-			float pitch)
-	{
-		return 0;
-	}
+
+	int playSound(const SimpleSoundSpec &spec) { return -1; }
+	int playSoundAt(const SimpleSoundSpec &spec, const v3f &pos) { return -1; }
 	void stopSound(int sound) {}
 	bool soundExists(int sound) { return false; }
 	void updateSoundPosition(int sound, v3f pos) {}
diff --git a/src/client/sound_openal.cpp b/src/client/sound_openal.cpp
index 0eda8842b..b015aba25 100644
--- a/src/client/sound_openal.cpp
+++ b/src/client/sound_openal.cpp
@@ -322,7 +322,7 @@ private:
 	OnDemandSoundFetcher *m_fetcher;
 	ALCdevice *m_device;
 	ALCcontext *m_context;
-	int m_next_id;
+	u16 m_last_used_id = 0; // only access within getFreeId() !
 	std::unordered_map<std::string, std::vector<SoundBuffer*>> m_buffers;
 	std::unordered_map<int, PlayingSound*> m_sounds_playing;
 	struct FadeState {
@@ -342,8 +342,7 @@ public:
 	OpenALSoundManager(SoundManagerSingleton *smg, OnDemandSoundFetcher *fetcher):
 		m_fetcher(fetcher),
 		m_device(smg->m_device.get()),
-		m_context(smg->m_context.get()),
-		m_next_id(1)
+		m_context(smg->m_context.get())
 	{
 		infostream << "Audio: Initialized: OpenAL " << std::endl;
 	}
@@ -379,6 +378,22 @@ public:
 		infostream << "Audio: Deinitialized." << std::endl;
 	}
 
+	u16 getFreeId()
+	{
+		u16 startid = m_last_used_id;
+		while (!isFreeId(++m_last_used_id)) {
+			if (m_last_used_id == startid)
+				return 0;
+		}
+
+		return m_last_used_id;
+	}
+
+	inline bool isFreeId(int id) const
+	{
+		return id > 0 && m_sounds_playing.find(id) == m_sounds_playing.end();
+	}
+
 	void step(float dtime)
 	{
 		doFades(dtime);
@@ -437,7 +452,7 @@ public:
 				<< std::endl;
 		assert(buf);
 		PlayingSound *sound = new PlayingSound;
-		assert(sound);
+
 		warn_if_error(alGetError(), "before createPlayingSoundAt");
 		alGenSources(1, &sound->source_id);
 		alSourcei(sound->source_id, AL_BUFFER, buf->buffer_id);
@@ -463,28 +478,17 @@ public:
 	{
 		assert(buf);
 		PlayingSound *sound = createPlayingSound(buf, loop, volume, pitch);
-		if(!sound)
+		if (!sound)
 			return -1;
-		int id = m_next_id++;
-		m_sounds_playing[id] = sound;
-		return id;
-	}
 
-	int playSoundRawAt(SoundBuffer *buf, bool loop, float volume, const v3f &pos,
-			float pitch)
-	{
-		assert(buf);
-		PlayingSound *sound = createPlayingSoundAt(buf, loop, volume, pos, pitch);
-		if(!sound)
-			return -1;
-		int id = m_next_id++;
-		m_sounds_playing[id] = sound;
-		return id;
+		int handle = getFreeId();
+		m_sounds_playing[handle] = sound;
+		return handle;
 	}
 
 	void deleteSound(int id)
 	{
-		std::unordered_map<int, PlayingSound*>::iterator i = m_sounds_playing.find(id);
+		auto i = m_sounds_playing.find(id);
 		if(i == m_sounds_playing.end())
 			return;
 		PlayingSound *sound = i->second;
@@ -580,39 +584,46 @@ public:
 		alListenerf(AL_GAIN, gain);
 	}
 
-	int playSound(const std::string &name, bool loop, float volume, float fade, float pitch)
+	int playSound(const SimpleSoundSpec &spec)
 	{
 		maintain();
-		if (name.empty())
+		if (spec.name.empty())
 			return 0;
-		SoundBuffer *buf = getFetchBuffer(name);
+		SoundBuffer *buf = getFetchBuffer(spec.name);
 		if(!buf){
-			infostream << "OpenALSoundManager: \"" << name << "\" not found."
+			infostream << "OpenALSoundManager: \"" << spec.name << "\" not found."
 					<< std::endl;
 			return -1;
 		}
+
 		int handle = -1;
-		if (fade > 0) {
-			handle = playSoundRaw(buf, loop, 0.0f, pitch);
-			fadeSound(handle, fade, volume);
+		if (spec.fade > 0) {
+			handle = playSoundRaw(buf, spec.loop, 0.0f, spec.pitch);
+			fadeSound(handle, spec.fade, spec.gain);
 		} else {
-			handle = playSoundRaw(buf, loop, volume, pitch);
+			handle = playSoundRaw(buf, spec.loop, spec.gain, spec.pitch);
 		}
 		return handle;
 	}
 
-	int playSoundAt(const std::string &name, bool loop, float volume, v3f pos, float pitch)
+	int playSoundAt(const SimpleSoundSpec &spec, const v3f &pos)
 	{
 		maintain();
-		if (name.empty())
+		if (spec.name.empty())
 			return 0;
-		SoundBuffer *buf = getFetchBuffer(name);
-		if(!buf){
-			infostream << "OpenALSoundManager: \"" << name << "\" not found."
+		SoundBuffer *buf = getFetchBuffer(spec.name);
+		if (!buf) {
+			infostream << "OpenALSoundManager: \"" << spec.name << "\" not found."
 					<< std::endl;
 			return -1;
 		}
-		return playSoundRawAt(buf, loop, volume, pos, pitch);
+
+		PlayingSound *sound = createPlayingSoundAt(buf, spec.loop, spec.gain, pos, spec.pitch);
+		if (!sound)
+			return -1;
+		int handle = getFreeId();
+		m_sounds_playing[handle] = sound;
+		return handle;
 	}
 
 	void stopSound(int sound)
@@ -624,8 +635,9 @@ public:
 	void fadeSound(int soundid, float step, float gain)
 	{
 		// Ignore the command if step isn't valid.
-		if (step == 0)
+		if (step == 0 || soundid < 0)
 			return;
+
 		float current_gain = getSoundGain(soundid);
 		step = gain - current_gain > 0 ? abs(step) : -abs(step);
 		if (m_sounds_fading.find(soundid) != m_sounds_fading.end()) {
diff --git a/src/gui/guiFormSpecMenu.cpp b/src/gui/guiFormSpecMenu.cpp
index e4ea9ff58..342f74b16 100644
--- a/src/gui/guiFormSpecMenu.cpp
+++ b/src/gui/guiFormSpecMenu.cpp
@@ -4498,7 +4498,7 @@ bool GUIFormSpecMenu::OnEvent(const SEvent& event)
 				if ((s.ftype == f_TabHeader) &&
 						(s.fid == event.GUIEvent.Caller->getID())) {
 					if (!s.sound.empty() && m_sound_manager)
-						m_sound_manager->playSound(s.sound, false, 1.0f);
+						m_sound_manager->playSound(SimpleSoundSpec(s.sound, false, 1.0f));
 					s.send = true;
 					acceptInput();
 					s.send = false;
@@ -4543,7 +4543,7 @@ bool GUIFormSpecMenu::OnEvent(const SEvent& event)
 
 				if (s.ftype == f_Button || s.ftype == f_CheckBox) {
 					if (!s.sound.empty() && m_sound_manager)
-						m_sound_manager->playSound(s.sound, false, 1.0f);
+						m_sound_manager->playSound(SimpleSoundSpec(s.sound, false, 1.0f));
 
 					s.send = true;
 					if (s.is_exit) {
@@ -4568,7 +4568,7 @@ bool GUIFormSpecMenu::OnEvent(const SEvent& event)
 						}
 					}
 					if (!s.sound.empty() && m_sound_manager)
-						m_sound_manager->playSound(s.sound, false, 1.0f);
+						m_sound_manager->playSound(SimpleSoundSpec(s.sound, false, 1.0f));
 					s.send = true;
 					acceptInput(quit_mode_no);
 
@@ -4586,7 +4586,7 @@ bool GUIFormSpecMenu::OnEvent(const SEvent& event)
 					s.fdefault = L"";
 				} else if (s.ftype == f_Unknown || s.ftype == f_HyperText) {
 					if (!s.sound.empty() && m_sound_manager)
-						m_sound_manager->playSound(s.sound, false, 1.0f);
+						m_sound_manager->playSound(SimpleSoundSpec(s.sound, false, 1.0f));
 					s.send = true;
 					acceptInput();
 					s.send = false;
diff --git a/src/network/clientpackethandler.cpp b/src/network/clientpackethandler.cpp
index fba0fe72d..1901c6675 100644
--- a/src/network/clientpackethandler.cpp
+++ b/src/network/clientpackethandler.cpp
@@ -820,12 +820,12 @@ void Client::handleCommand_PlaySound(NetworkPacket* pkt)
 	s32 server_id;
 
 	SimpleSoundSpec spec;
-	u8 type; // 0=local, 1=positional, 2=object
+	SoundLocation type; // 0=local, 1=positional, 2=object
 	v3f pos;
 	u16 object_id;
 	bool ephemeral = false;
 
-	*pkt >> server_id >> spec.name >> spec.gain >> type >> pos >> object_id >> spec.loop;
+	*pkt >> server_id >> spec.name >> spec.gain >> (u8 &)type >> pos >> object_id >> spec.loop;
 
 	try {
 		*pkt >> spec.fade;
@@ -836,22 +836,20 @@ void Client::handleCommand_PlaySound(NetworkPacket* pkt)
 	// Start playing
 	int client_id = -1;
 	switch(type) {
-		case 0: // local
-			client_id = m_sound->playSound(spec);
-			break;
-		case 1: // positional
-			client_id = m_sound->playSoundAt(spec, pos);
-			break;
-		case 2:
-		{ // object
+	case SoundLocation::Local:
+		client_id = m_sound->playSound(spec);
+		break;
+	case SoundLocation::Position:
+		client_id = m_sound->playSoundAt(spec, pos);
+		break;
+	case SoundLocation::Object:
+		{
 			ClientActiveObject *cao = m_env.getActiveObject(object_id);
 			if (cao)
 				pos = cao->getPosition();
 			client_id = m_sound->playSoundAt(spec, pos);
 			break;
 		}
-		default:
-			break;
 	}
 
 	if (client_id != -1) {
diff --git a/src/script/common/c_content.cpp b/src/script/common/c_content.cpp
index 1f9f97781..5595f3b14 100644
--- a/src/script/common/c_content.cpp
+++ b/src/script/common/c_content.cpp
@@ -1056,7 +1056,7 @@ void read_server_sound_params(lua_State *L, int index,
 		if(!lua_isnil(L, -1)){
 			v3f p = read_v3f(L, -1)*BS;
 			params.pos = p;
-			params.type = ServerPlayingSound::SSP_POSITIONAL;
+			params.type = SoundLocation::Position;
 		}
 		lua_pop(L, 1);
 		lua_getfield(L, index, "object");
@@ -1065,7 +1065,7 @@ void read_server_sound_params(lua_State *L, int index,
 			ServerActiveObject *sao = ObjectRef::getobject(ref);
 			if(sao){
 				params.object = sao->getId();
-				params.type = ServerPlayingSound::SSP_OBJECT;
+				params.type = SoundLocation::Object;
 			}
 		}
 		lua_pop(L, 1);
diff --git a/src/script/lua_api/l_client.cpp b/src/script/lua_api/l_client.cpp
index aaced7cd0..05ac53cbb 100644
--- a/src/script/lua_api/l_client.cpp
+++ b/src/script/lua_api/l_client.cpp
@@ -268,30 +268,32 @@ int ModApiClient::l_sound_play(lua_State *L)
 	SimpleSoundSpec spec;
 	read_soundspec(L, 1, spec);
 
+	SoundLocation type = SoundLocation::Local;
 	float gain = 1.0f;
-	float pitch = 1.0f;
-	bool looped = false;
-	s32 handle;
+	v3f position;
 
 	if (lua_istable(L, 2)) {
 		getfloatfield(L, 2, "gain", gain);
-		getfloatfield(L, 2, "pitch", pitch);
-		getboolfield(L, 2, "loop", looped);
+		getfloatfield(L, 2, "pitch", spec.pitch);
+		getboolfield(L, 2, "loop", spec.loop);
 
 		lua_getfield(L, 2, "pos");
 		if (!lua_isnil(L, -1)) {
-			v3f pos = read_v3f(L, -1) * BS;
+			position = read_v3f(L, -1) * BS;
+			type = SoundLocation::Position;
 			lua_pop(L, 1);
-			handle = sound->playSoundAt(
-					spec.name, looped, gain * spec.gain, pos, pitch);
-			lua_pushinteger(L, handle);
-			return 1;
 		}
 	}
 
-	handle = sound->playSound(spec.name, looped, gain * spec.gain, spec.fade, pitch);
-	lua_pushinteger(L, handle);
+	spec.gain *= gain;
 
+	s32 handle;
+	if (type == SoundLocation::Local)
+		handle = sound->playSound(spec);
+	else
+		handle = sound->playSoundAt(spec, position);
+
+	lua_pushinteger(L, handle);
 	return 1;
 }
 
diff --git a/src/server.cpp b/src/server.cpp
index 24e352658..4eee14a30 100644
--- a/src/server.cpp
+++ b/src/server.cpp
@@ -138,21 +138,27 @@ void *ServerThread::run()
 
 v3f ServerPlayingSound::getPos(ServerEnvironment *env, bool *pos_exists) const
 {
-	if(pos_exists) *pos_exists = false;
-	switch(type){
-	case SSP_LOCAL:
+	if (pos_exists)
+		*pos_exists = false;
+
+	switch (type ){
+	case SoundLocation::Local:
 		return v3f(0,0,0);
-	case SSP_POSITIONAL:
-		if(pos_exists) *pos_exists = true;
+	case SoundLocation::Position:
+		if (pos_exists)
+			*pos_exists = true;
 		return pos;
-	case SSP_OBJECT: {
-		if(object == 0)
-			return v3f(0,0,0);
-		ServerActiveObject *sao = env->getActiveObject(object);
-		if(!sao)
-			return v3f(0,0,0);
-		if(pos_exists) *pos_exists = true;
-		return sao->getBasePosition(); }
+	case SoundLocation::Object:
+		{
+			if (object == 0)
+				return v3f(0,0,0);
+			ServerActiveObject *sao = env->getActiveObject(object);
+			if (!sao)
+				return v3f(0,0,0);
+			if (pos_exists)
+				*pos_exists = true;
+			return sao->getBasePosition();
+		}
 	}
 	return v3f(0,0,0);
 }
@@ -2071,9 +2077,9 @@ s32 Server::playSound(ServerPlayingSound &params, bool ephemeral)
 {
 	// Find out initial position of sound
 	bool pos_exists = false;
-	v3f pos = params.getPos(m_env, &pos_exists);
+	const v3f pos = params.getPos(m_env, &pos_exists);
 	// If position is not found while it should be, cancel sound
-	if(pos_exists != (params.type != ServerPlayingSound::SSP_LOCAL))
+	if(pos_exists != (params.type != SoundLocation::Local))
 		return -1;
 
 	// Filter destination clients
diff --git a/src/server.h b/src/server.h
index 00b6f897d..cb7d77067 100644
--- a/src/server.h
+++ b/src/server.h
@@ -99,11 +99,7 @@ struct MediaInfo
 // Combines the pure sound (SimpleSoundSpec) with positional information
 struct ServerPlayingSound
 {
-	enum Type {
-		SSP_LOCAL,
-		SSP_POSITIONAL,
-		SSP_OBJECT
-	} type = SSP_LOCAL;
+	SoundLocation type = SoundLocation::Local;
 
 	float gain = 1.0f; // for amplification of the base sound
 	float max_hear_distance = 32 * BS;
diff --git a/src/sound.h b/src/sound.h
index ddb4e3dc2..801c552a9 100644
--- a/src/sound.h
+++ b/src/sound.h
@@ -59,3 +59,11 @@ struct SimpleSoundSpec
 	float pitch = 1.0f;
 	bool loop = false;
 };
+
+
+// The order must not be changed. This is sent over the network.
+enum class SoundLocation : u8 {
+	Local,
+	Position,
+	Object
+};