-
Notifications
You must be signed in to change notification settings - Fork 1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Sync footstep audio #2177
base: master
Are you sure you want to change the base?
Sync footstep audio #2177
Conversation
…nd(TYSM for the help Jannify) Co-authored-by: Jannify <24827220+Jannify@users.noreply.github.com>
NitroxClient/Communication/Packets/Processors/FootstepPacketProcessor.cs
Outdated
Show resolved
Hide resolved
NitroxClient/Communication/Packets/Processors/FootstepPacketProcessor.cs
Outdated
Show resolved
Hide resolved
NitroxClient/Communication/Packets/Processors/FootstepPacketProcessor.cs
Outdated
Show resolved
Hide resolved
NitroxClient/Communication/Packets/Processors/FootstepPacketProcessor.cs
Outdated
Show resolved
Hide resolved
NitroxClient/Communication/Packets/Processors/FootstepPacketProcessor.cs
Outdated
Show resolved
Hide resolved
NitroxServer/Communication/Packets/Processors/FootstepPacketProcessor.cs
Outdated
Show resolved
Hide resolved
NitroxServer/Communication/Packets/Processors/FootstepPacketProcessor.cs
Outdated
Show resolved
Hide resolved
NitroxServer/Communication/Packets/Processors/FootstepPacketProcessor.cs
Outdated
Show resolved
Hide resolved
NitroxServer/Communication/Packets/Processors/FootstepPacketProcessor.cs
Outdated
Show resolved
Hide resolved
NitroxServer/Communication/Packets/Processors/FootstepPacketProcessor.cs
Outdated
Show resolved
Hide resolved
Co-authored-by: Jannify <23176718+Jannify@users.noreply.github.com>
Co-authored-by: Jannify <23176718+Jannify@users.noreply.github.com>
Co-authored-by: Jannify <23176718+Jannify@users.noreply.github.com>
…implement fetching the value from .csv file
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
Outdated
Show resolved
Hide resolved
NitroxServer/Communication/Packets/Processors/FootstepPacketProcessor.cs
Outdated
Show resolved
Hide resolved
NitroxServer/Communication/Packets/Processors/FootstepPacketProcessor.cs
Outdated
Show resolved
Hide resolved
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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 &&
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Think I got this done actually this time but will keep conversation open until you mark it as resolved
// if both player's don't have SubRootIds | ||
if (!player.SubRootId.HasValue) | ||
{ | ||
if (NitroxVector3.Distance(player.Position, sendingPlayer.Position) <= footstepAudioRange && player != sendingPlayer) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I got this now, but I'll leave this open until you resolve it
player.SendPacket(footstepPacket); | ||
} | ||
} | ||
// If one player doesn't have an id and other does, automatically false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 <23176718+Jannify@users.noreply.github.com>
Co-authored-by: Jannify <23176718+Jannify@users.noreply.github.com>
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
Outdated
Show resolved
Hide resolved
Co-authored-by: Jannify <23176718+Jannify@users.noreply.github.com>
I have tested the PR as of commit 7e56f40, so if code reviews are approving, then this is ready to merge imo |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 ?
Co-authored-by: tornac1234 <tortornac@gmail.com>
Co-authored-by: tornac1234 <tortornac@gmail.com>
Co-authored-by: tornac1234 <tortornac@gmail.com>
Currently we're using a switch case to determine what asset is being played, and the exosuit has a different step sound so if I change the default case to be an early return and add a case for the land step sound, it would ensure that footstep packets are only sent for footsteps and not exosuit steps. I would also move that switch/case higher up so that it triggers before we get too far into the method. Would that be an acceptable solution to your suggestion @tornac1234 ? |
Fixes issue #2164
Does what the title says