Add more fishing sounds! #3800

Merged
ancientmarinerdev merged 4 commits from :fishing-sounds into master 2023-06-19 13:50:00 +02:00
Contributor

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

<!-- Please follow our contributing guidelines first: https://git.minetest.land/MineClone2/MineClone2/src/branch/master/CONTRIBUTING.md#how-you-can-help-as-a-programmer By submitting this pull request, you agree to follow our Code of Conduct: https://git.minetest.land/MineClone2/MineClone2/src/branch/master/CODE_OF_CONDUCT.md --> 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
Contributor

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
ancientmarinerdev reviewed 2023-06-18 20:31:03 +02:00
@ -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)

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
Author
Contributor

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

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.
Author
Contributor

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

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.

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.

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
Owner

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 :).
Author
Contributor

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

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

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
Author
Contributor

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

Take a look at it now I think I made things better
Niterux added 1 commit 2023-06-19 13:23:20 +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.
ancientmarinerdev approved these changes 2023-06-19 13:47:50 +02:00
ancientmarinerdev left a comment
Owner

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.
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

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.

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
Author
Contributor

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
Sign in to join this conversation.
No reviewers
No project
No Assignees
3 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: VoxeLibre/VoxeLibre#3800
No description provided.