Add more fishing sounds! #3800

Niterux · 2023-06-16T02:08:43+02:00

Niterux commented

2023-06-16 02:08:43 +02:00

Tell us about your pull request! Reference related issues, if necessary
In Minecraft when you go fishing there's sound effects when your bobber sinks or when you reel the bobber in or when your bobber hits the water, this pull request adds sounds for when those things happen.

Testing

Tell us how to test your changes!
go fishing with the fishing rod

Tell us about your pull request! Reference related issues, if necessary In Minecraft when you go fishing there's sound effects when your bobber sinks or when you reel the bobber in or when your bobber hits the water, this pull request adds sounds for when those things happen. ### Testing Tell us how to test your changes! go fishing with the fishing rod

Niterux added 1 commit 2023-06-16 02:08:48 +02:00

451a4413c4 add more fishing sounds!

kneekoo commented

2023-06-16 02:31:24 +02:00

Thanks for this! :)

Do you know how to edit/tweak sounds? In an old comment I mentioned two sounds, and the first is actually a better fit for the bobber landing into water. But the sound needs to be trimmed a bit, maybe even sped up slightly, and normalized.

My issue with the bloop and splash sounds you added here is that they're obviously indoor sounds, which would be quite weird fishing outdoors. So at least the splash sound I mentioned would be a pretty good choice if properly edited and normalized.

The reeling sound is great, I love it! :D Considering our rudimentary fishing rod, it doesn't match what we have in-game, but I think it's short enough not to be distracting. At least people who are used to fishing will likely appreciate it. :D

Thanks for this! :) Do you know how to edit/tweak sounds? In an [old comment](https://git.minetest.land/MineClone2/MineClone2/issues/46#issuecomment-12020) I mentioned two sounds, and the first is actually a better fit for the bobber landing into water. But the sound needs to be trimmed a bit, maybe even sped up slightly, and normalized. My issue with the bloop and splash sounds you added here is that they're obviously indoor sounds, which would be quite weird fishing outdoors. So at least the splash sound I mentioned would be a pretty good choice if properly edited and normalized. The reeling sound is great, I love it! :D Considering our rudimentary fishing rod, it doesn't match what we have in-game, but I think it's short enough not to be distracting. At least people who are used to fishing will likely appreciate it. :D

Niterux added 1 commit 2023-06-16 06:45:27 +02:00

d9a13c89b2 add new sounds, add janky sound hack because I couldn't figure out how to move sounds

ancientmarinerdev reviewed 2023-06-18 20:31:03 +02:00

mods/ITEMS/mcl_fishing/init.lua Outdated

					
				@ -161,6 +161,8 @@ local fish = function(itemstack, player, pointed_thing)

										end

										--Destroy bobber.

										ent.object:remove()

										minetest.sound_play("reel", {exclude_player=player:get_player_name(), pos=player:get_pos(), gain=0.1}, true)

ancientmarinerdev commented

2023-06-18 20:31:03 +02:00

Why do you play the sound twice here?

Considering you're playing it from the pos of the player, the first probably should be fine.

Also, can you specify max_hear_distance to 16 like in here, please:

https://git.minetest.land/MineClone2/MineClone2/pulls/3767/files

Also for the other sounds

Why do you play the sound twice here? Considering you're playing it from the pos of the player, the first probably should be fine. Also, can you specify max_hear_distance to 16 like in here, please: https://git.minetest.land/MineClone2/MineClone2/pulls/3767/files Also for the other sounds

Niterux commented

2023-06-18 23:03:52 +02:00

I couldn't figure out how to move the sound so I play it twice so it doesn't sound weird if you are walking and it plays

The problem with max_hear_distance is that you can fish up to 33 nodes away so the sounds would be inaudible which would make fishing feel strange

I couldn't figure out how to move the sound so I play it twice so it doesn't sound weird if you are walking and it plays The problem with max_hear_distance is that you can fish up to 33 nodes away so the sounds would be inaudible which would make fishing feel strange

ancientmarinerdev commented

2023-06-18 23:05:40 +02:00

It's very unlikely you'll be walking and catching a fish at the same time. Playing a sound twice potentially could affect performance on a server though, so I'd rather have a weird sound when walking.

Could the sounds not be done at the position of the player to mitigate this? Especially for the reeling. Would you hear the bobber going in from that far away? I cannot see myself fishing more than 10-15 blocks away, so further would be less frequent than you expect.

It's very unlikely you'll be walking and catching a fish at the same time. Playing a sound twice potentially could affect performance on a server though, so I'd rather have a weird sound when walking. Could the sounds not be done at the position of the player to mitigate this? Especially for the reeling. Would you hear the bobber going in from that far away? I cannot see myself fishing more than 10-15 blocks away, so further would be less frequent than you expect.

Niterux commented

2023-06-18 23:20:56 +02:00

Why would playing a sound twice have a noticable impact on the server?
This is how it sounds like without the fix, very funky, would like to know how to move sounds so it doesn't sound like this than leave it sounding like this
https://cdn.discordapp.com/attachments/878939911837847585/1120100746461990932/fishing_without_audio_fix.mp4

Why would playing a sound twice have a noticable impact on the server? This is how it sounds like without the fix, very funky, would like to know how to move sounds so it doesn't sound like this than leave it sounding like this https://cdn.discordapp.com/attachments/878939911837847585/1120100746461990932/fishing_without_audio_fix.mp4

ancientmarinerdev commented

2023-06-19 00:20:07 +02:00

While in isolation, it could be small, most stuff is executed on the server. Playing sounds is sent from the server to the clients. The more clients, the more recipients, and if multiple recipients and it's working out distance etc. or who to send to, it's going to add up. MT isn't too clever and sometimes sends information it shouldn't send. Between, overfrequent updates, many positional sounds, many blocks loading, the amount of packets sent isn't small, and lag does impact our servers. Sending more than we need is best avoided. We already send too much.

ancientmarinerdev commented

2023-06-19 00:24:31 +02:00

Why would playing a sound twice have a noticable impact on the server?
This is how it sounds like without the fix, very funky, would like to know how to move sounds so it doesn't sound like this than leave it sounding like this
https://cdn.discordapp.com/attachments/878939911837847585/1120100746461990932/fishing_without_audio_fix.mp4

Is the funky sound the fact that the reel sound volume decreases as you move?

I'm still trying to understand why you're moving. How many people move when reeling in a fishing rod? Is that likely to come up in normal play?

Have you tried the following:

object = ,
-- Attach the sound to an object.
-- Can't be used together with pos.

https://github.com/minetest/minetest/blob/master/doc/lua_api.md

> Why would playing a sound twice have a noticable impact on the server? > This is how it sounds like without the fix, very funky, would like to know how to move sounds so it doesn't sound like this than leave it sounding like this > https://cdn.discordapp.com/attachments/878939911837847585/1120100746461990932/fishing_without_audio_fix.mp4 Is the funky sound the fact that the reel sound volume decreases as you move? I'm still trying to understand why you're moving. How many people move when reeling in a fishing rod? Is that likely to come up in normal play? Have you tried the following: object = <an ObjectRef>, -- Attach the sound to an object. -- Can't be used together with `pos`. https://github.com/minetest/minetest/blob/master/doc/lua_api.md

ancientmarinerdev reviewed 2023-06-18 20:33:34 +02:00

ancientmarinerdev left a comment

Thanks for the PR. I tried this and like it. It definitely adds to the fishing immersion.

The only thing that bugs me a little is that the xp drop and reel sound play at same time after you catch a fish. I'm not sure if it should play the real sound then drop the xp after a short period of time (0.7s - 1.2s, idk, trial and error could help with that.). I think wrapping the xp_drop in a minetest.after function could do that. Or would it work the other way around, xp drop then reel?

I'd like to hear other opinions on that, but I'm happy with as is also :).

Thanks for the PR. I tried this and like it. It definitely adds to the fishing immersion. The only thing that bugs me a little is that the xp drop and reel sound play at same time after you catch a fish. I'm not sure if it should play the real sound then drop the xp after a short period of time (0.7s - 1.2s, idk, trial and error could help with that.). I think wrapping the xp_drop in a minetest.after function could do that. Or would it work the other way around, xp drop then reel? I'd like to hear other opinions on that, but I'm happy with as is also :).

Niterux commented

2023-06-18 23:05:34 +02:00

How do I use minetest.after I am new to Lua

ancientmarinerdev commented

2023-06-18 23:09:44 +02:00

How do I use minetest.after I am new to Lua

There should be many usages of this in the codebase so worth searching. Documentation has this:

https://github.com/minetest/minetest/blob/master/doc/lua_api.md

Timing

minetest.after(time, func, ...): returns job table to use as below.
    Call the function func after time seconds, may be fractional
    Optional: Variable number of arguments that are passed to func

> How do I use minetest.after I am new to Lua There should be many usages of this in the codebase so worth searching. Documentation has this: https://github.com/minetest/minetest/blob/master/doc/lua_api.md Timing minetest.after(time, func, ...): returns job table to use as below. Call the function func after time seconds, may be fractional Optional: Variable number of arguments that are passed to func

Niterux added 1 commit 2023-06-19 06:02:28 +02:00

d4ac061f46 edited bloop, added maximum distance for reel sound effect, use object for reel sound effect, delay experience orb spawn

Niterux commented

2023-06-19 06:02:52 +02:00

Take a look at it now I think I made things better

Niterux added 1 commit 2023-06-19 13:23:20 +02:00

c4600af0a9 Merge branch 'master' into fishing-sounds

ancientmarinerdev commented

2023-06-19 13:43:28 +02:00

Take a look at it now I think I made things better

Yeah, I'm loving that delay. That little bit of anticipation before the XP drop is so satisfying. I did this more than I needed to, just because I enjoyed it :D.

Thanks for taking the feedback on board. I'm personally happy from a code and quality perspective.

Btw, good job on the credit information in the readme. Providing where you get sounds from makes our lives so much easier. Thank you.

> Take a look at it now I think I made things better Yeah, I'm loving that delay. That little bit of anticipation before the XP drop is so satisfying. I did this more than I needed to, just because I enjoyed it :D. Thanks for taking the feedback on board. I'm personally happy from a code and quality perspective. Btw, good job on the credit information in the readme. Providing where you get sounds from makes our lives so much easier. Thank you.

👍 1

ancientmarinerdev approved these changes 2023-06-19 13:47:50 +02:00

ancientmarinerdev left a comment

Good from a code perspective, good from a gameplay perspective, so approving. Thanks for the PR. Many have been keen to get some sounds on this for a while and now you have done that! Much appreciated.

For future reference, please can you avoid merging master into your branch? It's only needed if there is a confict and rebase is prefered on this project. I cannot rebase a branch when master has been merged as it fantastically breaks it. I can rebase from Gitea UI if it hasn't got a merge commit which can make my life easier. I guess it's on a fork so it's not a big deal as it would depend on permissions.

Good from a code perspective, good from a gameplay perspective, so approving. Thanks for the PR. Many have been keen to get some sounds on this for a while and now you have done that! Much appreciated. For future reference, please can you avoid merging master into your branch? It's only needed if there is a confict and rebase is prefered on this project. I cannot rebase a branch when master has been merged as it fantastically breaks it. I can rebase from Gitea UI if it hasn't got a merge commit which can make my life easier. I guess it's on a fork so it's not a big deal as it would depend on permissions.

👍 1

ancientmarinerdev added the

sounds

label 2023-06-19 13:49:13 +02:00

ancientmarinerdev merged commit 766c9efe33 into master

2023-06-19 13:50:00 +02:00

ancientmarinerdev referenced this issue from a commit

2023-06-19 13:50:01 +02:00

Add more fishing sounds! (#3800)

ancientmarinerdev commented

2023-06-19 13:51:15 +02:00

I merged as a squash commit so remember to take any new branch from master and ensure your master is up to date with ours.

ancientmarinerdev added this to the 0.84.0 - Very Nice milestone 2023-06-19 13:51:33 +02:00

Niterux commented

2023-06-19 13:54:57 +02:00

Good from a code perspective, good from a gameplay perspective, so approving. Thanks for the PR. Many have been keen to get some sounds on this for a while and now you have done that! Much appreciated.

For future reference, please can you avoid merging master into your branch? It's only needed if there is a confict and rebase is prefered on this project. I cannot rebase a branch when master has been merged as it fantastically breaks it. I can rebase from Gitea UI if it hasn't got a merge commit which can make my life easier. I guess it's on a fork so it's not a big deal as it would depend on permissions.

oh okay I clicked on it because there was a button to do that so I thought it would be good if I did that

> Good from a code perspective, good from a gameplay perspective, so approving. Thanks for the PR. Many have been keen to get some sounds on this for a while and now you have done that! Much appreciated. > > For future reference, please can you avoid merging master into your branch? It's only needed if there is a confict and rebase is prefered on this project. I cannot rebase a branch when master has been merged as it fantastically breaks it. I can rebase from Gitea UI if it hasn't got a merge commit which can make my life easier. I guess it's on a fork so it's not a big deal as it would depend on permissions. oh okay I clicked on it because there was a button to do that so I thought it would be good if I did that

👍 1

kneekoo referenced this pull request

2023-06-19 14:20:59 +02:00

Fishing improvements #807

seventeenthShulker referenced this issue from a commit

2023-06-30 16:56:37 +02:00

Add more fishing sounds! (#3800)

Sign in to join this conversation.

No reviewers