Participants: fix disconnected players being returned (#98)

Sometimes bots and players dont get deleted from GameState.playersByUserID
due to missing player_disconnect events.
Some of these cases seem to be re-connects and some are inexplicable to
me.
This change excludes these disconnected players being returned by
Participants.All() etc.

- also made sendtables.Entity mockable for datatables_test.go

Author: markus-wa

.travis.yml

@@ -52,7 +52,7 @@ script:
   - go test -v -race -run "$race_tests" -timeout 15m
 
   # We run all tests again to get full coverage, technically unncecessary tho
-  - coverpkg_ignore='/msg'
+  - coverpkg_ignore='/(msg|fake)'
   # coverpkg for multiple mains is broken in 1.12, so we need to exclude the examples from coverage
   # https://github.com/golang/go/issues/30374
   - if ! [[ "$TRAVIS_GO_VERSION" =~ ^1\.11 ]]; then coverpkg_ignore="${coverpkg_ignore}|/examples"; fi

common/player.go

@@ -25,7 +25,7 @@ type Player struct {
 	ActiveWeaponID              int                // Used internally to set the active weapon, see ActiveWeapon()
 	RawWeapons                  map[int]*Equipment // All weapons the player is currently carrying
 	AmmoLeft                    [32]int            // Ammo left in the various weapons, index corresponds to key of RawWeapons
-	Entity                      *st.Entity
+	Entity                      st.IEntity
 	AdditionalPlayerInformation *AdditionalPlayerInformation // Mostly scoreboard information such as kills, deaths, etc.
 	ViewDirectionX              float32
 	ViewDirectionY              float32

datatables.go

@@ -178,7 +178,7 @@ func (p *Parser) bindPlayers() {
 	})
 }
 
-func (p *Parser) bindNewPlayer(playerEntity *st.Entity) {
+func (p *Parser) bindNewPlayer(playerEntity st.IEntity) {
 	entityID := playerEntity.ID()
 	rp := p.rawPlayers[entityID-1]
 
@@ -206,6 +206,7 @@ func (p *Parser) bindNewPlayer(playerEntity *st.Entity) {
 
 	playerEntity.OnDestroy(func() {
 		delete(p.gameState.playersByEntityID, entityID)
+		pl.Entity = nil
 	})
 
 	// Position

datatables_test.go

@@ -0,0 +1,86 @@
+package demoinfocs
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/mock"
+
+	"github.com/markus-wa/demoinfocs-golang/events"
+	st "github.com/markus-wa/demoinfocs-golang/sendtables"
+	fakest "github.com/markus-wa/demoinfocs-golang/sendtables/fake"
+)
+
+type DevNullReader struct {
+}
+
+func (DevNullReader) Read(p []byte) (n int, err error) {
+	return len(p), nil
+}
+
+func TestParser_BindNewPlayer_Issue98(t *testing.T) {
+	p := NewParser(new(DevNullReader))
+
+	p.rawPlayers = map[int]*playerInfo{
+		0: {
+			userID: 1,
+			name:   "Zim",
+			guid:   "BOT",
+		},
+		1: {
+			userID: 2,
+			name:   "The Suspect",
+			guid:   "123",
+		},
+	}
+
+	bot := fakePlayerEntity(1)
+	p.bindNewPlayer(bot)
+	bot.Destroy()
+
+	player := fakePlayerEntity(2)
+	p.bindNewPlayer(player)
+
+	assert.Len(t, p.GameState().Participants().All(), 1)
+}
+
+func TestParser_BindNewPlayer_Issue98_Reconnect(t *testing.T) {
+	p := NewParser(new(DevNullReader))
+
+	p.rawPlayers = map[int]*playerInfo{
+		0: {
+			userID: 2,
+			name:   "The Suspect",
+			guid:   "123",
+			xuid:   1,
+		},
+	}
+
+	player := fakePlayerEntity(1)
+	p.bindNewPlayer(player)
+	player.Destroy()
+
+	p.RegisterEventHandler(func(events.PlayerConnect) {
+		t.Error("expected no more PlayerConnect events but got one")
+	})
+	p.bindNewPlayer(player)
+
+	assert.Len(t, p.GameState().Participants().All(), 1)
+
+}
+
+func fakePlayerEntity(id int) st.IEntity {
+	entity := new(fakest.Entity)
+	entity.On("ID").Return(id)
+	var destroyCallback func()
+	entity.On("OnDestroy", mock.Anything).Run(func(args mock.Arguments) {
+		destroyCallback = args.Get(0).(func())
+	})
+	entity.On("OnPositionUpdate", mock.Anything).Return()
+	entity.On("FindProperty", mock.Anything).Return(new(st.Property))
+	entity.On("BindProperty", mock.Anything, mock.Anything, mock.Anything).Return(new(st.Property))
+	entity.On("Destroy").Run(func(mock.Arguments) {
+		destroyCallback()
+	})
+	return entity
+}

game_state.go

@@ -148,7 +148,11 @@ type Participants struct {
 func (ptcp Participants) ByUserID() map[int]*common.Player {
 	res := make(map[int]*common.Player)
 	for k, v := range ptcp.playersByUserID {
-		res[k] = v
+		// We need to check if the player entity hasn't been destroyed yet
+		// See https://github.com/markus-wa/demoinfocs-golang/issues/98
+		if v.Entity != nil {
+			res[k] = v
+		}
 	}
 	return res
 }
@@ -167,8 +171,8 @@ func (ptcp Participants) ByEntityID() map[int]*common.Player {
 // All returns all currently connected players & spectators.
 // The returned slice is a snapshot and is not updated on changes.
 func (ptcp Participants) All() []*common.Player {
-	res := make([]*common.Player, 0, len(ptcp.playersByUserID))
-	for _, p := range ptcp.playersByUserID {
+	res, original := ptcp.initalizeSliceFromByUserID()
+	for _, p := range original {
 		res = append(res, p)
 	}
 	return res
@@ -177,8 +181,8 @@ func (ptcp Participants) All() []*common.Player {
 // Playing returns all players that aren't spectating or unassigned.
 // The returned slice is a snapshot and is not updated on changes.
 func (ptcp Participants) Playing() []*common.Player {
-	res := make([]*common.Player, 0, len(ptcp.playersByUserID))
-	for _, p := range ptcp.playersByUserID {
+	res, original := ptcp.initalizeSliceFromByUserID()
+	for _, p := range original {
 		if p.Team != common.TeamSpectators && p.Team != common.TeamUnassigned {
 			res = append(res, p)
 		}
@@ -189,10 +193,10 @@ func (ptcp Participants) Playing() []*common.Player {
 // TeamMembers returns all players belonging to the requested team at this time.
 // The returned slice is a snapshot and is not updated on changes.
 func (ptcp Participants) TeamMembers(team common.Team) []*common.Player {
-	res := make([]*common.Player, 0, len(ptcp.playersByUserID))
-	for _, ptcp := range ptcp.playersByUserID {
-		if ptcp.Team == team {
-			res = append(res, ptcp)
+	res, original := ptcp.initalizeSliceFromByUserID()
+	for _, p := range original {
+		if p.Team == team {
+			res = append(res, p)
 		}
 	}
 	return res
@@ -209,3 +213,8 @@ func (ptcp Participants) FindByHandle(handle int) *common.Player {
 
 	return ptcp.playersByEntityID[handle&entityHandleIndexMask]
 }
+
+func (ptcp Participants) initalizeSliceFromByUserID() ([]*common.Player, map[int]*common.Player) {
+	byUserID := ptcp.ByUserID()
+	return make([]*common.Player, 0, len(byUserID)), byUserID
+}

game_state_test.go

@@ -6,6 +6,7 @@ import (
 	"github.com/stretchr/testify/assert"
 
 	"github.com/markus-wa/demoinfocs-golang/common"
+	st "github.com/markus-wa/demoinfocs-golang/sendtables"
 )
 
 func TestNewGameState(t *testing.T) {
@@ -41,8 +42,8 @@ func TestGameState_Participants(t *testing.T) {
 	byUserID := ptcp.ByUserID()
 
 	// Should update ptcp as well since it uses the same map
-	gs.playersByEntityID[0] = common.NewPlayer()
-	gs.playersByUserID[0] = common.NewPlayer()
+	gs.playersByEntityID[0] = newPlayer()
+	gs.playersByUserID[0] = newPlayer()
 
 	assert.Equal(t, gs.playersByEntityID, ptcp.ByEntityID())
 	assert.Equal(t, gs.playersByUserID, ptcp.ByUserID())
@@ -54,7 +55,7 @@ func TestGameState_Participants(t *testing.T) {
 
 func TestParticipants_All(t *testing.T) {
 	gs := newGameState()
-	pl := common.NewPlayer()
+	pl := newPlayer()
 	gs.playersByUserID[0] = pl
 
 	allPlayers := gs.Participants().All()
@@ -65,19 +66,19 @@ func TestParticipants_All(t *testing.T) {
 func TestParticipants_Playing(t *testing.T) {
 	gs := newGameState()
 
-	terrorist := common.NewPlayer()
+	terrorist := newPlayer()
 	terrorist.Team = common.TeamTerrorists
 	gs.playersByUserID[0] = terrorist
-	ct := common.NewPlayer()
+	ct := newPlayer()
 	ct.Team = common.TeamCounterTerrorists
 	gs.playersByUserID[1] = ct
-	unassigned := common.NewPlayer()
+	unassigned := newPlayer()
 	unassigned.Team = common.TeamUnassigned
 	gs.playersByUserID[2] = unassigned
-	spectator := common.NewPlayer()
+	spectator := newPlayer()
 	spectator.Team = common.TeamSpectators
 	gs.playersByUserID[3] = spectator
-	def := common.NewPlayer()
+	def := newPlayer()
 	gs.playersByUserID[4] = def
 
 	playing := gs.Participants().Playing()
@@ -89,19 +90,19 @@ func TestParticipants_Playing(t *testing.T) {
 func TestParticipants_TeamMembers(t *testing.T) {
 	gs := newGameState()
 
-	terrorist := common.NewPlayer()
+	terrorist := newPlayer()
 	terrorist.Team = common.TeamTerrorists
 	gs.playersByUserID[0] = terrorist
-	ct := common.NewPlayer()
+	ct := newPlayer()
 	ct.Team = common.TeamCounterTerrorists
 	gs.playersByUserID[1] = ct
-	unassigned := common.NewPlayer()
+	unassigned := newPlayer()
 	unassigned.Team = common.TeamUnassigned
 	gs.playersByUserID[2] = unassigned
-	spectator := common.NewPlayer()
+	spectator := newPlayer()
 	spectator.Team = common.TeamSpectators
 	gs.playersByUserID[3] = spectator
-	def := common.NewPlayer()
+	def := newPlayer()
 	gs.playersByUserID[4] = def
 
 	cts := gs.Participants().TeamMembers(common.TeamCounterTerrorists)
@@ -112,7 +113,7 @@ func TestParticipants_TeamMembers(t *testing.T) {
 func TestParticipants_FindByHandle(t *testing.T) {
 	gs := newGameState()
 
-	pl := common.NewPlayer()
+	pl := newPlayer()
 	pl.Team = common.TeamTerrorists
 	gs.playersByEntityID[3000&entityHandleIndexMask] = pl
 
@@ -124,11 +125,28 @@ func TestParticipants_FindByHandle(t *testing.T) {
 func TestParticipants_FindByHandle_InvalidEntityHandle(t *testing.T) {
 	gs := newGameState()
 
-	pl := common.NewPlayer()
+	pl := newPlayer()
 	pl.Team = common.TeamTerrorists
 	gs.playersByEntityID[invalidEntityHandle&entityHandleIndexMask] = pl
 
 	found := gs.Participants().FindByHandle(invalidEntityHandle)
 
 	assert.Nil(t, found)
 }
+
+func TestParticipants_SuppressNoEntity(t *testing.T) {
+	gs := newGameState()
+	pl := newPlayer()
+	gs.playersByUserID[0] = pl
+	gs.playersByUserID[1] = common.NewPlayer()
+
+	allPlayers := gs.Participants().All()
+
+	assert.ElementsMatch(t, []*common.Player{pl}, allPlayers)
+}
+
+func newPlayer() *common.Player {
+	pl := common.NewPlayer()
+	pl.Entity = new(st.Entity)
+	return pl
+}

sendtables/entity.go

@@ -9,6 +9,8 @@ import (
 	bit "github.com/markus-wa/demoinfocs-golang/bitread"
 )
 
+//go:generate ifacemaker -f entity.go -s Entity -i IEntity -p sendtables -D -y "IEntity is an auto-generated interface for Entity, intended to be used when mockability is needed." -c "DO NOT EDIT: Auto generated" -o entity_interface.go
+
 // Entity stores a entity in the game (e.g. players etc.) with its properties.
 type Entity struct {
 	serverClass *ServerClass
@@ -56,7 +58,7 @@ func (e *Entity) FindProperty(name string) *Property {
 // BindProperty combines FindProperty() & Property.Bind() into one.
 // Essentially binds a property's value to a pointer.
 // See the docs of the two individual functions for more info.
-func (e *Entity) BindProperty(name string, variable interface{}, valueType propertyValueType) {
+func (e *Entity) BindProperty(name string, variable interface{}, valueType PropertyValueType) {
 	e.FindProperty(name).Bind(variable, valueType)
 }
 
@@ -296,12 +298,13 @@ func (pe *Property) Value() PropertyValue {
 	return pe.value
 }
 
-type propertyValueType int
+// PropertyValueType specifies the type of a PropertyValue
+type PropertyValueType int
 
 // Possible types of property values.
 // See Property.Bind()
 const (
-	ValTypeInt propertyValueType = iota
+	ValTypeInt PropertyValueType = iota
 	ValTypeFloat32
 	ValTypeFloat64 // Like ValTypeFloat32 but with additional cast to float64
 	ValTypeString
@@ -339,7 +342,7 @@ This will bind the property's value to i so every time it's updated i is updated
 
 The valueType indicates which field of the PropertyValue to use for the binding.
 */
-func (pe *Property) Bind(variable interface{}, valueType propertyValueType) {
+func (pe *Property) Bind(variable interface{}, valueType PropertyValueType) {
 	var binder PropertyUpdateHandler
 	switch valueType {
 	case ValTypeInt:

sendtables/entity_interface.go

@@ -0,0 +1,55 @@
+// DO NOT EDIT: Auto generated
+
+package sendtables
+
+import (
+	r3 "github.com/golang/geo/r3"
+	bit "github.com/markus-wa/demoinfocs-golang/bitread"
+)
+
+// IEntity is an auto-generated interface for Entity, intended to be used when mockability is needed.
+// Entity stores a entity in the game (e.g. players etc.) with its properties.
+type IEntity interface {
+	// ServerClass returns the entity's server-class.
+	ServerClass() *ServerClass
+	// ID returns the entity's ID.
+	ID() int
+	// Properties returns all properties of the Entity.
+	Properties() []Property
+	// FindProperty finds a property on the Entity by name.
+	//
+	// Returns nil if the property wasn't found.
+	//
+	// Panics if more than one property with the same name was found.
+	FindProperty(name string) *Property
+	// BindProperty combines FindProperty() & Property.Bind() into one.
+	// Essentially binds a property's value to a pointer.
+	// See the docs of the two individual functions for more info.
+	BindProperty(name string, variable interface{}, valueType PropertyValueType)
+	// ApplyUpdate reads an update to an Enitiy's properties and
+	// triggers registered PropertyUpdateHandlers if values changed.
+	//
+	// Intended for internal use only.
+	ApplyUpdate(reader *bit.BitReader)
+	// Position returns the entity's position in world coordinates.
+	Position() r3.Vector
+	// OnPositionUpdate registers a handler for the entity's position update.
+	// The handler is called with the new position every time a position-relevant property is updated.
+	//
+	// See also Position()
+	OnPositionUpdate(h func(pos r3.Vector))
+	// BindPosition binds the entity's position to a pointer variable.
+	// The pointer is updated every time a position-relevant property is updated.
+	//
+	// See also OnPositionUpdate()
+	BindPosition(pos *r3.Vector)
+	// OnDestroy registers a function to be called on the entity's destruction.
+	OnDestroy(delegate func())
+	// Destroy triggers all via OnDestroy() registered functions.
+	//
+	// Intended for internal use only.
+	Destroy()
+	// OnCreateFinished registers a function to be called once the entity is fully created -
+	// i.e. once all property updates have been sent out.
+	OnCreateFinished(delegate func())
+}