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: Remove fingerprint functionality from beacons #1267

Merged
merged 8 commits into from Oct 29, 2016

Conversation

Projects
None yet
5 participants
@Faerachnidendro
Contributor

Faerachnidendro commented Oct 28, 2016

~snipped previous description.

Actual issue stems from planted beacon entity receiving reference to beacon weapon's fingerprints table, instead of a new/cached table of it's own - entity attempts to merge the weapon's fingerprints table, with itself, instead of merging it's own fingerprints with that of the weapon(?)

gamemodes/terrortown/entites/weapons/weapon_ttt_beacon.lua @ lines 99 & 151

Latest update: Can't concoct a viable work around for beacons to utilize fingerprints, recommend removing fingerprint functionality from beacons.

Faerachnidendro added some commits Oct 28, 2016

Fix c-side error which hangs server on beacon pickup.
I may actually be the only person who bothered to test this thing out :P

When picking up a beacon that has been placed, the server will hang at least once per map for up to a few seconds and will always throw an ambiguous blue "[ERROR]" message, I have identified the cause as being incorrect usage of table.Add, this function very much prefers to have the second argument as the desired index, when inserting tables into number-indexed tables. Providing only the table to be inserted, confuses the hell out of things for reasons that I'm not certain of - a better fix may actually be to alter the default behavior of table.Add, I won't make a pull for that though.
Fix error which hangs server on beacon pickup.
I may actually be the only person who bothered to test this thing out :P

When picking up a beacon that has been placed, the server will hang at least once per map for up to a few seconds and will always throw an ambiguous blue "[ERROR]" message, I have identified the cause as being incorrect usage of table.Add, this function very much prefers to have the second argument as the desired index, when inserting tables into number-indexed tables. Providing only the table to be inserted, confuses the hell out of things for reasons that I'm not certain of - a better fix may actually be to alter the default behavior of table.Add, I won't make a pull for that though.
@Faerachnidendro

This comment has been minimized.

Owner

Faerachnidendro commented on cea976c Oct 28, 2016

Erm, sorry about the eroneous edits, didn't realise it would change the pull so much.

@bigdogmat

This comment has been minimized.

bigdogmat commented Oct 28, 2016

Uh, table.Add doesn't have a third argument (source)

@Faerachnidendro

This comment has been minimized.

Contributor

Faerachnidendro commented Oct 28, 2016

Then why is my code working?
Not to be a dick but I can't argue with results.

Edit: rather, why is the error no longer showing? "working" is a bit generous considering the expected behavior of table.Add

Edit 2: Okay derp on me, it's obvious why the error isn't showing - I guess the issue is with the table being added - my bad.

@bigdogmat

This comment has been minimized.

bigdogmat commented Oct 28, 2016

I mean, you can't argue with source either.

Are you sure that part of the code is running, i.e putting a print before the function is ran and after will result in both prints being ran? If they both run fine then I can only assume something is overwriting table.Add, try running this in console lua_run PrintTable(debug.getinfo(table.Add))

@Faerachnidendro

This comment has been minimized.

Contributor

Faerachnidendro commented Oct 28, 2016

See edits to above post :P
Bad troubleshooting on my part, will either edit this pull or replace it with an actual fix.

@bigdogmat

This comment has been minimized.

bigdogmat commented Oct 28, 2016

Ah, ok

@Faerachnidendro

This comment has been minimized.

Contributor

Faerachnidendro commented Oct 28, 2016

AH!
I see where I've gone wrong...

table.Add is misusing table.insert, in the way(or similar) that i thought table.Add was being misused.

I'm new to this git hub business - am I able to entirely change this pull or should I close it and make a new one?

Faerachnidendro added some commits Oct 28, 2016

Fix for anomolous behavior with table.Add
I caught on to this issue when trying to troubleshoot the ttt_beacon entity, in an earlier iteration of this pull/branch.
If you'll excuse the shoddy troubleshooting beforehand, I think this change to table.Add will be perfectly safe and totally avert the issue in the context it was found and many others that it may be rearing it's head.

The issue is not one I can fully explain, except that some tables really don't like the way table.Add works currently, which will cause a blue "[ERROR]" message to be thrown to console, and also results in a server hang on occasion.

@Faerachnidendro Faerachnidendro changed the title from Fix error which hangs server on beacon pickup. to Fix anomolous behavior with table.Add Oct 28, 2016

@willox

This comment has been minimized.

Collaborator

willox commented Oct 28, 2016

table.Add( { 1, 2, 3 }, { 4, 5, 6 } ) is meant to return { 1, 2, 3, 4, 5, 6 }. Your changes would break that. I'd suggest you just try to fix any issues TTT has in TTT specifically rather than changing any behaviour in the libraries every other addon uses.

@Faerachnidendro

This comment has been minimized.

Contributor

Faerachnidendro commented Oct 28, 2016

Yeah I just caught on to that xD
was reading over it now and thinking of different work arounds, as I'm pretty sure the issue isn't with the fingerprints table being added, but how table.Add handles the table - unless of course I DID miss something else in the ttt_beacon entity?

Either way, my bad for the not so useful attempt at a fix here, feel free to close and delete this one until I figure it out :P

@bigdogmat

This comment has been minimized.

bigdogmat commented Oct 28, 2016

The only thing that would cause table.Add to hang the server would be if it was called with a huge table as it's source. Nothings wrong with the way the function itself handles tables.

@Faerachnidendro

This comment has been minimized.

Contributor

Faerachnidendro commented Oct 28, 2016

wait, so, if table.Add does intend for the dest table to remain sequential, shouldn't table.insert be provided with #dest + 1 as arg 2? with v from the source loop as arg3?

@bigdogmat

This comment has been minimized.

bigdogmat commented Oct 28, 2016

If table.insert is not given a third argument then it'll use the second argument as the value and push it onto the end of the table

@Faerachnidendro

This comment has been minimized.

Contributor

Faerachnidendro commented Oct 28, 2016

I understand that this is the expected behavior, but must admit, it has always worked falwlessly when providing the desired index, and errored in some fasion otherwise - personal experience, no modification of either table.Add or table.insert

@bigdogmat

This comment has been minimized.

bigdogmat commented Oct 28, 2016

As I forgot in my earlier comment, table.Add does type checking on both arguments, if source is not a table then it'll return dest.

@Faerachnidendro

This comment has been minimized.

Contributor

Faerachnidendro commented Oct 28, 2016

Yeah I saw that, I'm not saying you're wrong so to speak - even the wiki confirms the expected behavior of table.insert, but in practice(personally), table.insert is unreliable when not given the desired index.

(honestly not just plucking straws there, I do indeed always provide the desired index when using table.insert, because of past errors)

@bigdogmat

This comment has been minimized.

bigdogmat commented Oct 28, 2016

table.insert when used without a index isn't unreliable at all, if it is then you're using it incorrectly. By default it tries to push it into the end of the table, if the table has holes in it then it isn't sequential and as such table.insert has no idea where to put the value. As such it's undefined behavior

@Faerachnidendro

This comment has been minimized.

Contributor

Faerachnidendro commented Oct 28, 2016

If the table wasn't sequential, that would be the error message, no? (genuinely asking)

@bigdogmat

This comment has been minimized.

bigdogmat commented Oct 28, 2016

No, just the behavior of where it places the value would be undefined

@Faerachnidendro

This comment has been minimized.

Contributor

Faerachnidendro commented Oct 28, 2016

Heh, fair enough.
Just recall seeing errors about non sequential tables before( probably from iterator functions ).

Alright so then I guess it goes back to something else being wrong with the ttt_beacon entity.... got me fuckin stumped now though.

Sorry for harping on table.insert, just never ran into a problem with it after i got into the habit of providing the index :P

@Faerachnidendro

This comment has been minimized.

Contributor

Faerachnidendro commented Oct 28, 2016

I'm gonna close this for now until I can figure out what's going on - but, that said, the blue "[ERROR]" message and server hang come back if I revert my changes to the ttt_beacon entity; the call to table.Add on line 58 of gamemodes/terrortown/entities/entities/ttt_beacon.lua, is throwing the error.

@bigdogmat

This comment has been minimized.

bigdogmat commented Oct 28, 2016

Then that means the issues manifests from adding the entities fingerprints table to the weapons fingerprints tables. So there's your thing to go off of

@Faerachnidendro

This comment has been minimized.

Contributor

Faerachnidendro commented Oct 28, 2016

Well yeah, but nothing is out of the ordinary there, unless it has something to do with the source table(ent's fingerprints) being empty?

@Faerachnidendro

This comment has been minimized.

Contributor

Faerachnidendro commented Oct 28, 2016

Oh for the love of... figured it out... it's sending the exact same table as both arg1 and arg2...
As in literally the same object.

@Faerachnidendro

This comment has been minimized.

Contributor

Faerachnidendro commented Oct 28, 2016

gamemodes/terrortown/entities/weapons/weapon_ttt_beacon.lua @ lines 99 & 151

Planted beacon entity recieves reference to weapon's fingerprints table, instead of new/cached fingerprints table of it's own - genius.

@Faerachnidendro Faerachnidendro changed the title from Fix anomolous behavior with table.Add to TTT: Fix inappropriate use of table.Add involving weapon_ttt_beacon and ttt_beacon Oct 28, 2016

@bigdogmat

This comment has been minimized.

bigdogmat commented Oct 28, 2016

I thought about this, though that would result in an out of memory crash, or at least it should.

@Faerachnidendro

This comment has been minimized.

Contributor

Faerachnidendro commented Oct 28, 2016

Well to be fair, you also thought that the only way table.Add would hang the server, is if the supplied tables were overpopulated - indeed, perhaps this explains the hang perfectly?

@willox

This comment has been minimized.

Collaborator

willox commented Oct 28, 2016

What error did you get in your server's console, anyway?

@Faerachnidendro

This comment has been minimized.

Contributor

Faerachnidendro commented Oct 28, 2016

literally "[ERROR]" in 0,255,255 teal, nothing more, nothing less(except the hang)
Which is what made me jump to a few conclusions and confuse myself - I'm not silly, just overtired :P

@bigdogmat

This comment has been minimized.

bigdogmat commented Oct 28, 2016

That's weird, it should've crashed the entire server with an out of memory error, instead it prevents it and prints out a blue [ERROR] in console

srcds_2016-10-28_12-55-12

@willox

This comment has been minimized.

Collaborator

willox commented Oct 28, 2016

I guess if Lua runs out of memory in a protected context it's just going to error and then run the GC afterwards and carry on chugging. I'd expect whatever code is stitching the error message together to actually handle not being able to allocate a buffer for the message though.

@Faerachnidendro

This comment has been minimized.

Contributor

Faerachnidendro commented Oct 28, 2016

I should be able to knock up a fix to the way the beacon works, to avoid this issue resurfacing in this context - as for the "[ERROR]" thing, maybe an if dest == source then return dest end condition being added to table.Add ? (as a means of preventing the error message until the hang is fixed, that is)

@Faerachnidendro

This comment has been minimized.

Contributor

Faerachnidendro commented Oct 28, 2016

At this point, I'm not seeing any reason for beacons to even record fingerprints - let alone an efficient and consistent way to cache the fingerprints of each placed beacon, popping them in and out as required on beacon spawn/pickup.
Mostly due to the way weapon_ttt_beacon removes and re-adds itself to the user's inventory, when exhausting all ammo or when picking up a placed beacon, having had no ammo left - using the weapon's SWEP:Clip1() value as a means for determining which fingerprints to give a beacon, results in inconsistencies which occasionally lead to inappropriate erasing of previously cached fingerprints.

The best course of action to rectify the error being thrown by the ttt_beacon entity, in my opinion, would be to remove all fingerprint related code for beacons, which shouldn't change much of anything about how they're used(if anyone even does use them).

Faerachnidendro added some commits Oct 28, 2016

Prevent weapon_ttt_beacon from using fingerprints.
This will stop placed beacons throwing the blue "[ERROR]" message and server hang when picked up.
Prevent ttt_beacon from using fingerprints
This will stop placed beacons throwing the blue "[ERROR]" message and server hang when picked up.

@Faerachnidendro Faerachnidendro changed the title from TTT: Fix inappropriate use of table.Add involving weapon_ttt_beacon and ttt_beacon to TTT: Remove fingerprint functionality from beacons Oct 28, 2016

@robotboy655 robotboy655 added the TTT label Oct 28, 2016

@svdm

This comment has been minimized.

Collaborator

svdm commented Oct 29, 2016

There may be a fix involving copying the fingerprints to the beacon instead of setting a reference, and removing duplicate prints after merging them back to the weapon on pickup, but whether any of the fingerprint behavior still makes sense after that is another thing.

@svdm svdm merged commit 89b6917 into Facepunch:master Oct 29, 2016

@Faerachnidendro

This comment has been minimized.

Contributor

Faerachnidendro commented Oct 29, 2016

That's what I tried at first, but was too tired to pursue it.
Might have another bash at it again soon.

Even if a consistent way to transfer fingerprints can be achieved - there still remains no real opportunity for a beacon to actually gather fingerprints from any players, except for maybe the beacon owner themself; pointless really.

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