Fix #350, Updated CTakeDamageInfo wrapper for CS:GO.

[CookStar]

Referenced from alliedmodders/sourcemod@250886f, alliedmodders/hl2sdk@1d2902b

[jordanbriere]

Nice, thanks! Here are some points:

• I think it would be better to move the m_bNeedInit assignment after m_hHndl so that the object isn't falsely flagged as initialized if the conversion failed (if the given attacker index is invalid, for example).
• That struct should be initialized into CTakeDamageInfo's constructor: https://github.com/Source-Python-Dev-Team/Source.Python/blob/master/src/core/patches/csgo/undefined_symbols.h#L40-L63
• Why not use m_iClientIndex directly into get_attacker?

[CookStar]

• I think it would be better to move the m_bNeedInit assignment after m_hHndl so that the object isn't falsely flagged as initialized if the conversion failed (if the given attacker index is invalid, for example).

In my tests, there has never been a time when m_bNeedInit was true, but I will do so just in case.

• That struct should be initialized into CTakeDamageInfo's constructor: https://github.com/Source-Python-Dev-Team/Source.Python/blob/master/src/core/patches/csgo/undefined_symbols.h#L40-L63

CSGOAttackerInfo's default constructor is already in hl2sdk.

• Why not use m_iClientIndex directly into get_attacker?

In my tests, indeed m_iClientIndex contains the index derived from the attacker's handle, but I have not actually looked at the binary and cannot be sure, so I implemented it this way.

We might try changing it to m_iClientIndex and see how it goes.

[jordanbriere]

Something else I just realized is that m_bIsWorld and m_bIsPlayer should be handled differently. For example, if someone currently does:

info.attacker = player
info.attacker = world

m_bIsPlayer will remains true although the attacker is no longer a player. Both should be set in an else block of your "is player" condition. Other members related to the player (userid, team stuff, etc.) should also ideally be reset in that block so that the object don't carry outdated data if the attacker is changing.

In my tests, there has never been a time when m_bNeedInit was true, but I will do so just in case.

It will before an attacker is set when new instances are created, for example into Entity.take_damage (which we should also ensure still works in case projectiles are no longer handled by the inflictor, etc. now that I think about it).

[CookStar]

m_bIsPlayer will remains true although the attacker is no longer a player. Both should be set in an else block of your "is player" condition. Other members related to the player (userid, team stuff, etc.) should also ideally be reset in that block so that the object don't carry outdated data if the attacker is changing.

You're absolutely right.
This is a question that comes from my lack of experience with the Source SDK, but does PlayerInfoFromIndex fail even if I'm sure the Index is a Player's? In other words, is it possible to determine with confidence if the Entity is a Player with PlayerInfoFromIndex?

It will before an attacker is set when new instances are created, for example into Entity.take_damage

Yeah, but it immediately overrides with set_attacker(0) in CTakeDamageInfo *init().

[jordanbriere]

You're absolutely right.
This is a question that comes from my lack of experience with the Source SDK, but does PlayerInfoFromIndex fail even if I'm sure the Index is a Player's? In other words, is it possible to determine with confidence if the Entity is a Player with PlayerInfoFromIndex?

PlayerInfoFromIndex isn't part of the SDK but part of our conversion utilities (declared into ../src/core/utilities/conversions/playerinfo_from.cpp) and does the necessary checks to ensure the given index is a valid player currently on the server. So yes, you could remove your range check and follow whether the conversion succeeded or not.

Yeah, but it immediately overrides with set_attacker(0) in CTakeDamageInfo *init().

Good point. I forgot a constructor was added to the Python class.

[CookStar]

PlayerInfoFromIndex isn't part of the SDK but part of our conversion utilities (declared into ../src/core/utilities/conversions/playerinfo_from.cpp)

Yeah I know, but I wasn't sure about this part...

Source.Python/src/core/utilities/conversions/playerinfo_from.cpp

Lines 91 to 93 in aeef22f

	IPlayerInfo* pPlayerInfo = playerinfomanager->GetPlayerInfo(pEdict);
	if (!pPlayerInfo)
	return false;

and does the necessary checks to ensure the given index is a valid player currently on the server. So yes, you could remove your range check and follow whether the conversion succeeded or not.

That''s good to hear. Thanks for the advice.

[jordanbriere]

Yeah I know, but I wasn't sure about this part...

Source.Python/src/core/utilities/conversions/playerinfo_from.cpp

Lines 91 to 93 in aeef22f

	IPlayerInfo* pPlayerInfo = playerinfomanager->GetPlayerInfo(pEdict);
	if (!pPlayerInfo)
	return false;

Assuming the SDK is up-to-date when it comes to the player manager, GetPlayerInfo fails if the CBaseEntity pointer can't be cast as a CBasePlayer and I'm sure VALVe overload that cast to make sure it's reliable. I'd say it's pretty safe to assume the server knows what it is doing. 😄

[CookStar]

I will double-check about m_bIsWorld later.

[jordanbriere]

I will double-check about m_bIsWorld later.

Looks good to me at a code level, what else is there to test? Slight note though, the external linkage of gpGlobals is no longer needed as it is not used anymore.

[CookStar]

Slight note though, the external linkage of gpGlobals is no longer needed as it is not used anymore.

I missed that.

what else is there to test?

A check in the game to see if Index(0) and m_bIsWorld are linked, and they seem to be.

(which we should also ensure still works in case projectiles are no longer handled by the inflictor, etc. now that I think about it).

I don't know how it behaved before, but it doesn't look like it changed.