Sync footstep audio #2177

OhmV-IR · 2024-09-07T14:39:03Z

Fixes issue #2164
Does what the title says

Footstep audio radius is currently set to 20m feel free to change it depending on your preference
Footstep max audio multiplier is 0.5f, once again feel free to change it depending on your preference
Server checks for same structure and will not send footstep packets to players who are outside of the sending player's structure
Footstep sound volume scales with distance
Uses reflection to create and send footstep packet when local game plays footstep sound

…l yet)

…range(20f)

…nd(TYSM for the help Jannify) Co-authored-by: Jannify <[email protected]>

…ther player

NitroxClient/Communication/Packets/Processors/FootstepPacketProcessor.cs

NitroxServer/Communication/Packets/Processors/FootstepPacketProcessor.cs

Co-authored-by: Jannify <[email protected]>

…implement fetching the value from .csv file

OhmV-IR · 2024-09-21T14:39:59Z

Did another test with the help of kookoo, CSV sound range works, tested everything else and it was still fine so review away

NitroxClient/Communication/Packets/Processors/FootstepPacketProcessor.cs

NitroxModel/Packets/FootstepPacket.cs

NitroxServer/Communication/Packets/Processors/FootstepPacketProcessor.cs

Jannify · 2024-09-24T13:33:18Z

NitroxServer/Communication/Packets/Processors/FootstepPacketProcessor.cs

+                if (player.SubRootId.HasValue)
+                {
+                    // If both players have id's, check if they are the same
+                    if (NitroxVector3.Distance(player.Position, sendingPlayer.Position) <= footstepAudioRange && player != sendingPlayer && player.SubRootId.Value == sendingPlayer.SubRootId.Value)


Line is too long imo

Still not fixed or commented why keeping that way.

These two ifs could be merged

think I did that

With merging I mean that two nested ifs with no else can be merged into one statement, meaning you chain both conditions together with an &&

Think I got this done actually this time but will keep conversation open until you mark it as resolved

Jannify · 2024-09-24T13:38:07Z

NitroxServer/Communication/Packets/Processors/FootstepPacketProcessor.cs

+                // if both player's don't have SubRootIds
+                if (!player.SubRootId.HasValue)
+                {
+                    if (NitroxVector3.Distance(player.Position, sendingPlayer.Position) <= footstepAudioRange && player != sendingPlayer)


The first if in this brnaching tree should be the most common. In this case the NitroxVector3.Distance [...] && player != sendingPlayer bc it's in both branches

Done

Not done.

Also if you apply this I would recommend inverting the if so you have an early return for better clarity. Visit this for more information

I think I got this now, but I'll leave this open until you resolve it

Jannify · 2024-09-24T13:41:30Z

NitroxServer/Communication/Packets/Processors/FootstepPacketProcessor.cs

+                        player.SendPacket(footstepPacket);
+                    }
+                }
+                // If one player doesn't have an id and other does, automatically false


Here are many comments. Just keep in mind that comments should explain the important external things needed to understand the code (line), not explain the code itself. For example: No need to write that "both ids should be the same" if the if-statement below does exactly that check.

Removed the comments, don't think there is enough of a reason to have them. Leaving this open in case you think otherwise

Co-authored-by: Jannify <[email protected]>

…oxModdedUnofficial into sync-footstep-audio

OhmV-IR · 2024-09-24T21:34:24Z

Will do another test after approving reviews, so don't merge until that happens(will post here again when it does)

NitroxServer/Communication/Packets/Processors/FootstepPacketProcessor.cs

Co-authored-by: Jannify <[email protected]>

OhmV-IR · 2024-09-28T15:38:32Z

I have tested the PR as of commit 7e56f40, so if code reviews are approving, then this is ready to merge imo

tornac1234

You need to update FootstepSounds_OnStep_PatchTest (change the number of instructions added by the transpiler). Also please use "Remove and Sort usings" before committing changes to a file.
It works fine IG but for some reason, the local player also sends packet for exosuits even if they're not walking those.
Can we have some sort of check that the FootstepSounds is for sure emitted by the local player or by an exosuit controlled by the local player ?

tornac1234 · 2024-09-29T13:54:02Z

NitroxModel/Packets/FootstepPacket.cs

+        PRECURSOR_STEP_SOUND,
+        METAL_STEP_SOUND,
+        LAND_STEP_SOUND
+    }


I feel like we're repeating twice "step sound" while writing StepSounds.X_STEP_SOUND while it could probably be better as StepSounds.X

tornac1234 · 2024-09-29T13:55:21Z

NitroxModel/Packets/FootstepPacket.cs

+
+    public FootstepPacket(ushort playerID, StepSounds assetIndex)
+    {
+        this.PlayerID = playerID;


No need to use this if PlayerID and playerID are not the same word

OhmV-IR added 13 commits August 28, 2024 00:17

Created packet and processors for footstep audio(not functional at al…

d4e3ff2

…l yet)

prototype client-side footstep packet processing

7e0c624

Switch to using player's AnimationController to find velocity

380d9ba

Footstep packets are forwarded from server to players within hearing …

e07a5c3

…range(20f)

Fixed FootstepPacket class to fit format(was crashing)

180bb35

Removed duplicate var

26db96c

cleanup

a772c16

Added transpiler to send packets when local client plays footstep sou…

eaf1122

…nd(TYSM for the help Jannify) Co-authored-by: Jannify <[email protected]>

Lower volume of other players' footsteps(suggested by NinjaPedroX)

a879656

Added same structure/submarine check for sending footstep packet to o…

8ef9842

…ther player

Removed logs

092af16

Formatting changes

c6814b3

Formatting changes

737028e

Measurity linked an issue Sep 10, 2024 that may be closed by this pull request

Footsteps sounds not synced #2164

Open

Measurity added this to the 1.8 milestone Sep 10, 2024

OhmV-IR added 2 commits September 18, 2024 15:37

Newline changes

103c14e

newline changes

5a5fc18

Jannify requested changes Sep 18, 2024

View reviewed changes

OhmV-IR and others added 6 commits September 18, 2024 16:47

Split long line into shorter lines

91936ec

Co-authored-by: Jannify <[email protected]>

Remove unneeded this keyword

6545583

Co-authored-by: Jannify <[email protected]>

Applying suggestions from Jannify's review

33d249c

Co-authored-by: Jannify <[email protected]>

Merge

8cc0956

Added comment in packet processors to keep values updated, trying to …

c1d0ef3

…implement fetching the value from .csv file

Pull sound range from .csv file(UNTESTED)

c4c851f

OhmV-IR requested a review from Jannify September 21, 2024 14:38

Jannify requested changes Sep 24, 2024

View reviewed changes

OhmV-IR and others added 3 commits September 24, 2024 15:25

Add whitespace

756bece

Co-authored-by: Jannify <[email protected]>

Add whitespace

30055b8

Co-authored-by: Jannify <[email protected]>

Merged ifs and removed var type usage

d26a186

OhmV-IR added 3 commits September 24, 2024 17:13

Merge branch 'sync-footstep-audio' of https://github.com/OhmV-IR/Nitr…

5c6f971

…oxModdedUnofficial into sync-footstep-audio

Merged else + if into else if

38c6161

Inverted if with early return condition

9a5927b

Jannify reviewed Sep 25, 2024

View reviewed changes

NitroxServer/Communication/Packets/Processors/FootstepPacketProcessor.cs Outdated Show resolved Hide resolved

OhmV-IR added 4 commits September 25, 2024 22:18

Compare subroot IDs using .Equals instead of 2 if statements

38c1aa7

Overriding .Equals in Optional<> for consistency and performance

6d4dcbf

Co-authored-by: Jannify <[email protected]>

Merged 2 if statements & fixed bad clause

bef5717

Fixed bug(was getting all players instead of only connected ones)

7e56f40

tornac1234 requested changes Sep 29, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Sync footstep audio #2177

Sync footstep audio #2177

OhmV-IR commented Sep 7, 2024

OhmV-IR commented Sep 21, 2024

Jannify Sep 24, 2024

OhmV-IR Sep 24, 2024

Jannify Sep 24, 2024

OhmV-IR Sep 24, 2024

Jannify Sep 24, 2024

OhmV-IR Sep 24, 2024

OhmV-IR commented Sep 24, 2024

OhmV-IR commented Sep 28, 2024

tornac1234 left a comment

tornac1234 Sep 29, 2024

tornac1234 Sep 29, 2024

Sync footstep audio #2177

Are you sure you want to change the base?

Sync footstep audio #2177

Conversation

OhmV-IR commented Sep 7, 2024

OhmV-IR commented Sep 21, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

OhmV-IR commented Sep 24, 2024

OhmV-IR commented Sep 28, 2024

tornac1234 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment