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

TTT: Fixed very old C4 Bug #1271

Merged
merged 3 commits into from Nov 13, 2016

Conversation

Projects
None yet
5 participants
@markusmarkusz
Contributor

markusmarkusz commented Nov 6, 2016

This bug exists since 9 January 2014.
It causes that the following part of the C4 doesn't work:
https://github.com/garrynewman/garrysmod/blob/master/garrysmod/gamemodes/terrortown/entities/entities/ttt_c4/cl_init.lua#L423-L440

@robotboy655 robotboy655 added the TTT label Nov 6, 2016

@svdm

This comment has been minimized.

Collaborator

svdm commented Nov 7, 2016

Nice find. I don't think the writing code is the problem here though. Looking at the client code for that net message, it seem that the problem is simply that the corresponding ReadUInt will read 16 bits instead of 15: https://github.com/garrynewman/garrysmod/blob/master/garrysmod/gamemodes/terrortown/entities/entities/ttt_c4/cl_init.lua#L474

This micro-optimisation was introduced in the PR converting TTT to the net library, and it's a lesson on why such bit-counting optimisations are not worth it when they're used on messages that are rarely sent in the first place.

I think my preferred fix would be replacing this UInt nonsense with WriteEntity and ReadEntity. That's going to do the same thing without the risk of getting the number of bits wrong when writing/reading.

@markusmarkusz

This comment has been minimized.

Contributor

markusmarkusz commented Nov 7, 2016

I'll do that on saturday when I'm back at home.

Am 07.11.2016 3:09 nachm. schrieb "Stefan van der Meer" <
notifications@github.com>:

Nice find. I don't think the writing code is the problem here though.
Looking at the client code for that net message, it seem that the problem
is simply that the corresponding ReadUInt will read 16 bits instead of
15: https://github.com/garrynewman/garrysmod/blob/
master/garrysmod/gamemodes/terrortown/entities/entities/
ttt_c4/cl_init.lua#L474

This micro-optimisation was introduced in the PR converting TTT to the net
library, and it's a lesson on why such bit-counting optimisations are not
worth it when they're used on messages that are rarely sent in the first
place.

I think my preferred fix would be replacing this UInt nonsense with
WriteEntity and ReadEntity. That's going to do the same thing without the
risk of getting the number of bits wrong when writing/reading.


You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#1271 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AHmREM7vvyn0h4jX3l62UPYy_OPSAWmjks5q7zEUgaJpZM4KqkhL
.

@markusmarkusz

This comment has been minimized.

Contributor

markusmarkusz commented Nov 12, 2016

@svdm

I think my preferred fix would be replacing this UInt nonsense with WriteEntity and ReadEntity. That's going to do the same thing without the risk of getting the number of bits wrong when writing/reading.

Done.

When I tested the changes I saw this message when I disarmed the bomb:

Create Stream Failed error 41
Failed to load sound "ttt\wirecut.mp3", file probably missing from disk/repository

Is this problem related to the lua code of the bomb? (I'm sure it isn't but asking is the best thing.)
Btw. the sound files exists.

] lua_run_menu print(file.Exists("gamemodes/terrortown/content/sound/ttt/wirecut.mp3", "GAME"))
true
@@ -457,9 +457,8 @@ end
---- Communication
local function C4ConfigHook()
local idx = net.ReadUInt(16)
local bomb = net.ReadEntity(16)

This comment has been minimized.

@Bo98

Bo98 Nov 12, 2016

Contributor

Don't need that 16 anymore.

This comment has been minimized.

@markusmarkusz

markusmarkusz Nov 12, 2016

Contributor

oops. Removed.

@svdm

This comment has been minimized.

Collaborator

svdm commented Nov 13, 2016

No clue why the sound would not be found. It used to work, of course, but given that this net msg bug has existed for a while something must have changed in the meantime. Possibly in gmod's mounting of a gamemode's content/sound directory.

@svdm svdm merged commit 305f644 into Facepunch:master Nov 13, 2016

@willox

This comment has been minimized.

Collaborator

willox commented Nov 13, 2016

Sound errors suck. Error 41 is BASS_ERROR_FILEFORM which means the file format isn't supported. Your other MP3 files work, so I guess it's something specifically wrong with wirecut.mp3.

@robotboy655

This comment has been minimized.

Collaborator

robotboy655 commented Nov 13, 2016

Should just use .wav for sound effects tbh

@markusmarkusz

This comment has been minimized.

Contributor

markusmarkusz commented Nov 13, 2016

I'll convert them and test it.
If it works then I'll upload them later in a PR.

@Bo98

This comment has been minimized.

Contributor

Bo98 commented Nov 13, 2016

Converting MP3 to WAV is useless unless it fixes the error. You've already lost quality if you're converting from the MP3, so no benefit will be gained.

Re-encoding the MP3 might be enough to fix this though.

@markusmarkusz

This comment has been minimized.

Contributor

markusmarkusz commented Nov 13, 2016

Then can you do that? Or tell me how to do that?

@markusmarkusz markusmarkusz deleted the markusmarkusz:patch-1 branch Nov 15, 2016

@markusmarkusz markusmarkusz restored the markusmarkusz:patch-1 branch Nov 15, 2016

@svdm

This comment has been minimized.

Collaborator

svdm commented Nov 15, 2016

I've converted it to WAV and though I don't have time to test it in-game right now, it at least makes certain audio players happier than the MP3 version. Not sure why because it's a pretty standard MP3 (128kbps CBR mono), maybe the fact that it's less than a second long is somehow an issue.

Either way I'll test the WAV soon and commit it if it works out.

@Bo98

This comment has been minimized.

Contributor

Bo98 commented Nov 15, 2016

So were there other audio players complaining about the MP3 file?

I haven't had a chance to check the file myself.

svdm added a commit that referenced this pull request Nov 20, 2016

TTT: Fix C4 wire cut sound error
The sound for cutting a wire when disarming C4 was failing to load due
to some issue with the mp3 (see #1271). Converting to WAV fixed it (and
given that it's a tiny sound file the filesize is insignificant).

@markusmarkusz markusmarkusz deleted the markusmarkusz:patch-1 branch Feb 26, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment