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

Fix a few issues with sound #5561

Merged
merged 8 commits into from
Jul 20, 2019
Merged

Fix a few issues with sound #5561

merged 8 commits into from
Jul 20, 2019

Conversation

pupnewfster
Copy link
Member

@pupnewfster pupnewfster commented Jul 10, 2019

Changes proposed in this pull request:

  • Adds some null safety to PlayerSounds and only get the player object when needed
  • Fix flame thrower not playing sounds (it still doesn't play a sound when first equipped but unused for other players)
  • Cleaned up various logic and method readability in ClientTickHandler and CommonPlayerTickHandler
  • Keep track of which sounds we have for a player by UUID instead of EntityPlayer reference. Also use a WeakReference in PlayerSound instead of directly saving the EntityPlayer object (gracefully falls back to last known x, y, z position if the reference gets cleaned up while a reference to PlayerSound is still stored). This should allow the GC to do its job a lot better.
  • Updated PacketFlamethrowerData to be like the Jetpack and GasMask ones in terms of MODE_CHANGE, UPDATE, and FULL options

I did do some testing, though it would probably not hurt to double check some of the logic/test more thoroughly on multiplayer.

One other thing to note I believe that at least one case where the player looked up is null may have to do with the fact I believe it syncs data for clients in all dimensions not just the one that is the same as our player. Not sure if/where we should catch that if we decide to but for the most part I believe these changes improve the handling of the various player sounds.

@pupnewfster pupnewfster added this to the 9.8.0 milestone Jul 10, 2019
@thiakil
Copy link
Member

thiakil commented Jul 11, 2019

The null issue I think is to do with it not checking if it actually got a player reference when it receives the packet with the uuid, it just adds it to the map regardless. If we put a Objects.requireNonNull in there I bet that's where it'd crash

@pupnewfster
Copy link
Member Author

The null issue I think is to do with it not checking if it actually got a player reference when it receives the packet with the uuid, it just adds it to the map regardless. If we put a Objects.requireNonNull in there I bet that's where it'd crash

I will look at it again tomorrow, for now I am just having it not do anything if the player is null, but I may actually be able to catch it earlier to save a bit more on memory.

@pupnewfster
Copy link
Member Author

Actually upon further investigation, I think this needs a bigger overhaul as our SoundHandler (and PlayerSound) are keeping track of a reference to EntityPlayer which from my understanding is not a good thing to do for GC interacting with things. The only reason it even works currently I believe is given it keeps track of it in an IdentityHashMap then after dimension changes the object is different.

@pupnewfster pupnewfster requested a review from thiakil July 11, 2019 19:15
@pupnewfster
Copy link
Member Author

Went ahead and rebased this onto the latest 1.12, because even if it didn't have any merge conflicts with it, for testing it still is probably better to have it against the latest version. (Especially given how many changed happened due to the merging of the Rendering PR)

Copy link
Member

@thiakil thiakil left a comment

Choose a reason for hiding this comment

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

Looks good code wise, but I can't seem to get the active sound to work in my dev environment. Does it work for you?

@pupnewfster
Copy link
Member Author

Looks good code wise, but I can't seem to get the active sound to work in my dev environment. Does it work for you?

Ya tested again and it still works fine so going to go ahead and merge this.

@pupnewfster pupnewfster merged commit c5fbabe into 1.12 Jul 20, 2019
@pupnewfster pupnewfster deleted the sound_fix branch July 20, 2019 14:31
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.

None yet

2 participants