parser.RawPlayers not ensuring unicity

romain-durban commented 9 years ago

I had an exception on var user = parser.RawPlayers.Single(a => a.UserID == (int)data["userid"]);. You are not ensuring the unicity of entities in this list because you are checking only the array index and not the actual object instance.

You need to check already existing players with the same userid before adding any object. For instance var olduser = parser.RawPlayers.SingleOrDefault(a => a.UserID == info.UserID); if(olduser != null) parser.RawPlayers.Remove(olduser); parser.RawPlayers.Add(player); Note that my quick fix is probably not the best. (removing + adding => keeps last occurrence only)

Need to be fixed in all places you add players in this list.

DP/Handler/GameEventHandler.cs ~line 130 (event "player_connect")
DP/Handler/CreateStringTableUserInfoHandler.cs ~line 100
ST/StringTableParser.cs ~line 50

The possible reason of this bug could be players disconnecting during the game (timedout for instance, which was the case of my demo).

moritzuehling commented 9 years ago

ST/StringTableParser.cs ~line 50

At least there the dictionary is cleared at the beginning of the method (see lines 27 and 28).

If after this method there still exist two players with the same user-id things get weird.

I actually thought I tested it with enough demos, but the other parts seemed to have broken. You're right of course. I'll fix this this evening.

0BuRner commented 9 years ago

Hi guys, I rewrite a major part of the code to make it faster and easier to read. I still have to work on it to fix the case of disconnected players but globally its stable. You can have a look at it here : https://github.com/0BuRner/demoinfo-public Moreover I made a quick WPF app to see logs and data.

Romain si tu l'occasion d'y jeter un oeil et de faire des commentaires, je suis preneur :D

moritzuehling commented 9 years ago

Hi @0BuRner ,

I already saw your repository. If you want code to be merged back, please create a pull request. Since we're German we do not want French comments in our code. @main-- and I both are able to read it, but we decided for this project to be commented in English, je suis trés desolée.

The problem is why I can't merge that much stuff back by myself is that I sometimes have a hard time seeing what you have done.

The next thing is, that we have decided to not simply drop events:

var olduser = parser.RawPlayers.SingleOrDefault(a => a.UserID == info.UserID);
if(olduser != null)
    parser.RawPlayers.Remove(olduser);

If there are two players with the same userid, it'll never fix itself, and the event would silently drop. We want to avoid this.

So I am currently investigating the problem with the demo sent by @romain-durban .

@0BuRner : I don't agree with some things you "fixed": One Example.

if (header.NetworkProtocol < NETWORK_PROTOCOL_VERSION_MIN)

This is not correct according to the reference implementation

if ( m_DemoHeader.demoprotocol != DEMO_PROTOCOL )

and

#define DEMO_PROTOCOL       4

Therefore I couldn't accept this in a pull-request.

0BuRner commented 9 years ago

Hi @moritzuehling

There is indeed a lot of modification I made on your project. I'm new to git and I don't work really well with it right now : I should commit a lot often but smaller changes so its easier for everybody to see what changed. I'll try to fix it.

My first goal was not to make these changes public so its why there is still comments in French in my code.

I think your comment about my fix is wrong : you mix up Demo Protocol and Network Protocol. Network Protocol is changed by Valve each time they introduce new things into it. I made this check (which is wrong cause I forget to set NETWORK_PROTOCOL_VERSION_MIN the right value, shoud be at least 13401) as a temporary check, because the parser crash with (maybe not all) older demos. You can try with this one : http://www.hltv.org/interfaces/download.php?demoid=16774 Some other are buggy too but not the same error.

moritzuehling commented 9 years ago

I've been wrong on that one, there are several others I don't understand as well. I've made some comments explaining what I think is weird. An other problem is that you made it impossible to upstream your stuff, so if I change something here, you can't change it as well.

To you "performance-improvements" - what did you improve? I did not run through all 2k lines that you commited in two commits, so what exactly did you change to get faster?

0BuRner commented 9 years ago

I used dotTrace to look the program and improve the performances.

moritzuehling commented 9 years ago

This issue should be fixed in e560b2b.

The players should now be unique, it worked with the test-demo and the other tests. Much thanks for you help, @romain-durban !

adamburgess commented 9 years ago

I'm having problems with this. On a player_disconnect an exception is raised in GameEventHandler.cs line 145, looking for the userid in the Players dictionary. Occurs on NIP v LDLC dust2 DHW: http://www.hltv.org/interfaces/download.php?demoid=17658, tick 5197. It doesn't happen for all disconnects, though...

moritzuehling commented 9 years ago

I'm looking into this.

moritzuehling commented 9 years ago

Okay, this is weird.

I know what happens now. Angeldust disconnects, and reconnects. On all the events, his index-property is set to 12, so he is the entity 13.

During the beginning of the demo, the entity 13 is networked as an normal CCSPlayer.

So far, so cool. When Angeldust first disconnects, we get an "LeavePVS | DeletePVS"-Update.

In the Valve-Reference-Implementation a entity gets deleted as soon as the first bits after the header that are read are 10 or 11. I actually only deleted when it was 11 because it is done that way in the edith-parser.

Here is the thing: The entity "13" leaves, but it never ever ever comes back. In fact during the whole demo there is not a single "EnterPVS" of an CCSPlayer except when "Anders" connects.

Since there is no entity there I don't create an Player-Object (I create it a few lines after that check).

So when I want to set the "disconnected"-property I actually can't. That is sooo weird. Angeldust is the only Player in this demo that hasn't an entity, and that throws me of. Players are supposed to have an entity.

The fun thing is: I don't get updates for entity 13 (that should be Angledust). I never get a single one. If I would there would be Updates for Entity 13, I'd get an exception here, but I don't.

So this is a very weird state, and I don't know how to solve this problem.

We have two options now:

Don't delete Entities. This means the old Angledust-Entity is still there and everything is fine. Disadvantage: I don't know whether this will work for all demos.
Suppress the exception at player_disconnect-events. This is the method I'd favor. It doesn't break our parser right now, and if this ever happens to a real-player (not an spectator) an exception will be thrown as soon as he fires his weapon. Disadvantage: Not a real fix.

Thoughs of you guys?

adamburgess commented 9 years ago

Valve's RemoveEntity removes the entity if it finds it in the entity list, and doesn't create any errors if it can't find the entity, so I would go for suppressing them.

moritzuehling commented 9 years ago

The entity-removal is fine, it is just that I don't have a player because I don't have an entity. Oh well, I'll surpess them.

moritzuehling commented 9 years ago

"Fixed" in 441dab988

StatsHelix / demoinfo

parser.RawPlayers not ensuring unicity #10