Skip to content
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

Open
wants to merge 41 commits into
base: master
Choose a base branch
from

Conversation

OhmV-IR
Copy link
Contributor

@OhmV-IR OhmV-IR commented Sep 7, 2024

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

@Measurity Measurity linked an issue Sep 10, 2024 that may be closed by this pull request
@Measurity Measurity added this to the 1.8 milestone Sep 10, 2024
OhmV-IR and others added 6 commits September 18, 2024 16:47
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>
@OhmV-IR
Copy link
Contributor Author

OhmV-IR commented Sep 21, 2024

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

OhmV-IR and others added 3 commits September 24, 2024 15:25
Co-authored-by: Jannify <23176718+Jannify@users.noreply.github.com>
Co-authored-by: Jannify <23176718+Jannify@users.noreply.github.com>
@OhmV-IR
Copy link
Contributor Author

OhmV-IR commented Sep 24, 2024

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

@OhmV-IR
Copy link
Contributor Author

OhmV-IR commented Sep 28, 2024

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

Copy link
Collaborator

@tornac1234 tornac1234 left a 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 ?

NitroxModel/Packets/FootstepPacket.cs Show resolved Hide resolved
NitroxModel/Packets/FootstepPacket.cs Outdated Show resolved Hide resolved
OhmV-IR and others added 3 commits September 30, 2024 18:48
Co-authored-by: tornac1234 <tortornac@gmail.com>
Co-authored-by: tornac1234 <tortornac@gmail.com>
Co-authored-by: tornac1234 <tortornac@gmail.com>
@OhmV-IR
Copy link
Contributor Author

OhmV-IR commented Oct 1, 2024

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 ?

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 ?

Co-authored-by: tornac1234 <tortornac@gmail.com>
OhmV-IR and others added 2 commits October 15, 2024 15:38
…bstraction

Co-authored-by: Jannify <23176718+Jannify@users.noreply.github.com>
Co-authored-by: Jannify <23176718+Jannify@users.noreply.github.com>
Copy link
Member

@Jannify Jannify left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM CW

Co-authored-by: gussandst <160942469+gussandst@users.noreply.github.com>
Co-authored-by: WerewolfsX <160942469+WerewolfsX@users.noreply.github.com>
@OhmV-IR
Copy link
Contributor Author

OhmV-IR commented Oct 19, 2024

Test went well, @tornac1234 needs to review again before merging though

Copy link
Collaborator

@tornac1234 tornac1234 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will be good for merge after this change (squash and merge)

Co-authored-by: tornac1234 <tortornac@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Footsteps sounds not synced
4 participants