PhilippvK / playforia-minigolf

Client & Server for Minigolf Game known from Playforia/Playray/Appeli. Written in Java.
84 stars 31 forks source link

Fix sound, upgrade to Java 17 #103

Closed StenAL closed 1 year ago

StenAL commented 1 year ago

This restores the satisfying thunk sound that plays on every stroke.

See individual commits for more details.

Highlights:

PhilippvK commented 1 year ago

@StenAL This is awesome. Give me a few days to try it out :)

PhilippvK commented 1 year ago

@StenAL Sound works as expected!

However I found out that the game crashes at the end of a singleplayer round:

>>> Packet{channel=[id: 0x058bf4fc, /127.0.0.1:54884 => /127.0.0.1:4242], type=DATA, count=112, message='game\tendstroke\t0\tt'}
>>> Packet{channel=[id: 0x058bf4fc, /127.0.0.1:54884 => /127.0.0.1:4242], type=DATA, count=113, message='error-debug\t4\tjava.lang.NullPointerException: Cannot load from int array because "outcome" is null\tgame\end\tgame\startturn\0\tgame\endstroke\0\t'}
>>> Packet{channel=[id: 0x058bf4fc, /127.0.0.1:54884 => /127.0.0.1:4242], type=COMMAND, count=0, message='end'}
Unhandled packet: Packet{channel=[id: 0x058bf4fc, /127.0.0.1:54884 :> /127.0.0.1:4242], type=DATA, count=113, message='error-debug\t4\tjava.lang.NullPointerException: Cannot load from int array because "outcome" is null\tgame\end\tgame\startturn\0\tgame\endstroke\0\t'}
Unhandled packet: Packet{channel=[id: 0x058bf4fc, /127.0.0.1:54884 :> /127.0.0.1:4242], type=COMMAND, count=0, message='end'}
Client disconnected: [id: 0x058bf4fc, /127.0.0.1:54884 :> /127.0.0.1:4242]

This does not happen in the master branch, aka. it might be related to the Sound changes or Java upgrade. I hope you can reproduce this...

StenAL commented 1 year ago

Thanks for the thorough testing! I only tested in multiplayer.

I've pushed a fix now, the issue was with trying to play music at the end of a game based on the results, but in a single player game the server does not send any results so there was a null dereference.

PhilippvK commented 1 year ago

That was crazy fast!

I will approve and probably merge in a few hours unless you have anything against that.

PhilippvK commented 1 year ago

@StenAL the CI needs a patch to use Java 17 instead of 8. Should I fix this in a followup or can you have a look?

StenAL commented 1 year ago

I can take a look and I'll also squash the crash fix commit into the original change.

StenAL commented 1 year ago

@PhilippvK Looks like you need to approve every CI run, let's hope the first try works. image

StenAL commented 1 year ago

Alright, looks like CI passed (https://github.com/PhilippvK/playforia-minigolf/actions/runs/4834120524/jobs/8614993225?pr=103).

I've squashed the two fixup commits, the crash fix is now in https://github.com/PhilippvK/playforia-minigolf/pull/103/commits/d7cdeb8d83b8f55414ddd37787814d2a264b409f and CI fix is in https://github.com/PhilippvK/playforia-minigolf/pull/103/commits/dc8b995ff94f73733a450cb8cf97346257431805

I'm ready for merging now.