-
-
Notifications
You must be signed in to change notification settings - Fork 423
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
Add LookupEntityAttachment & GetEntityAttachment natives #1653
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested for CS:GO Windows, functions properly.
Looks like a great patch, thank you for seeing this through!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice one - thank you for the contribution. Can you add sdk2013 Gamedata outside of the comments? The networkable stuff was a big issue with ents a decade ago.
CBaseEntity* pEntity; | ||
ENTINDEX_TO_CBASEENTITY(params[1], pEntity); | ||
|
||
ServerClass* pClass = ((IServerUnknown*)pEntity)->GetNetworkable()->GetServerClass(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This needs a null check on networkable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The networkable null check should be unnecessary on this. Or at least amount to no extra security.
IServerUnknown is inherited by IServerEntity which itself is inherited by CBaseEntity, and if we look at the function implementation by valve :
inline IServerNetworkable *CBaseEntity::GetNetworkable()
{
return &m_Network;
}
And CBaseEntity definition
private:
// NOTE: Keep this near vtable so it's in cache with vtable.
CServerNetworkProperty m_Network;
The networkable interface should never be null. And valve also doesn't seem to ever null check this in their code base
https://github.com/ValveSoftware/source-sdk-2013/search?q=GetNetworkable
It's also used under CBaseEntity constructor regardless of whether it's server side only or not, this rules out the possibility of it ever being invalid.
CBaseEntity::CBaseEntity( bool bServerOnly )
{
// code
NetworkProp()->Init( this );
// code
}
inline CServerNetworkProperty *CBaseEntity::NetworkProp()
{
return &m_Network;
}
https://github.com/ValveSoftware/source-sdk-2013/blob/0d8dceea4310fde5706b3ce1c70609d72a38efdf/mp/src/game/server/baseentity.cpp#L367
https://github.com/ValveSoftware/source-sdk-2013/blob/0d8dceea4310fde5706b3ce1c70609d72a38efdf/mp/src/game/server/baseentity.h#L2378
CBaseEntity* pEntity; | ||
ENTINDEX_TO_CBASEENTITY(params[1], pEntity); | ||
|
||
ServerClass* pClass = ((IServerUnknown*)pEntity)->GetNetworkable()->GetServerClass(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ditto
ServerClass* pClass = ((IServerUnknown*)pEntity)->GetNetworkable()->GetServerClass(); | ||
if (!FindNestedDataTable(pClass->m_pTable, "DT_BaseAnimating")) | ||
{ | ||
return pContext->ThrowNativeError("Entity %d (%d) is not a CBaseAnimating", gamehelpers->ReferenceToIndex(params[1]), params[1]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you elaborate what you would like changed here?
ENTINDEX_TO_CBASEENTITY(params[1], pEntity); | ||
|
||
ServerClass* pClass = ((IServerUnknown*)pEntity)->GetNetworkable()->GetServerClass(); | ||
if (!FindNestedDataTable(pClass->m_pTable, "DT_BaseAnimating")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@asherkin am I losing it or is this not calling back to the safe nesting datatable code from core?
What exactly do you mean by this? There isn't any gamedata that would make sense to add for sdk2013. |
bca6ec3
to
09974e9
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This all looks good to me, the error handling and datatable bits all seem fine - one tiny comment on how the API is presented but otherwise I'd say the meat of this is all good to go.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
@KyleSanderson please could you re-review this or dismiss your stale review. |
Concerns addressed, no follow up
* Update syntax for SourceMod 1.11 * Remove DHooks dependency * Remove LookupAttachment SDKCall alliedmodders/sourcemod#1653 * Remove deprecated forwards * Rename ShowKeyHintText * Move pragmas up * A few more updates * Switch to AddTargetsToMenu2 * Update default admin flags * Update native return values * Update code and comment style * More style changes * yes * Change weird vector declaration * Re-register vehicle on plugin reload * Rework passenger damage in vehicles * Enforce convars * Use EOS constant * Add semicolons * Comments * Don't use nonexistant define * More headers
I might have an idea how to get attachment points without using signatures, though can't check if that will work: |
#1555
I agree.
Overview
This PR adds two new natives:
Example usage:
Gamedata for CS:GO, CS:S, L4D, L4D2 and TF2 is provided, but has not been tested by me.
Implementation
Using the virtual
CBaseAnimating::GetAttachment(int, matrix3x4_t &)
was a deliberate choice because virtual offsets are generally easier to maintain than signatures. Thematrix3x4_t
is converted to world position and world angles internally.Some of the other overloads are also inlined on a few games, making this the best choice.
Since this call can only be used on classes inheriting
CBaseAnimating
, we check if theDT_BaseAnimating
SendTable exists on the entity, throwing a native error if it doesn't.This safeguard could be greatly improved with a call to
CBaseEntity::GetBaseAnimating
, but would require more gamedata (maybe something to consider for the future?)Footnote
I have not been able to test this for any game other than TF2. I'd be grateful if other people could assist with testing
this on different games and engine versions.
This is my first time actively working with C++, so excuse some rookie mistakes.