TF2-DMB / CBaseNPC

Provides a friendly interface for plugins to use in order to create custom NPCs on the game Team Fortress 2
37 stars 6 forks source link

Implementation of CBaseEntity::SetAbsAngles is wrong #13

Closed Kenzzer closed 2 years ago

Kenzzer commented 2 years ago

https://github.com/TF2-DMB/CBaseNPC/blob/7c983f90478a15ff6f25c1d65500571e96fb4f40/extension/sourcesdk/baseentity.cpp#L360 Differs from https://github.com/ValveSoftware/source-sdk-2013/blob/0d8dceea4310fde5706b3ce1c70609d72a38efdf/mp/src/game/server/baseentity.cpp#L5944

The entity's matrix should be passed by reference, but instead a local matrix is.

KitRifty commented 2 years ago

I don't think there's an issue here. Dereferencing a pointer and passing it to a function that takes a reference parameter will affect the variable stored in the pointer variable's address, not a local copy. SetAbsAngles is functioning as it should normally. If it wasn't, then by extension SetAbsOrigin would introduce undefined behavior as it uses MatrixSetColumn and that takes a matrix by-reference parameter as well, but SetAbsOrigin is working normally.

Testing the native in SP:

public void OnMapStart()
{
    CBaseEntity ent = CBaseEntity(CreateEntityByName("prop_dynamic_override"));
    ent.SetModel("models/player/pyro.mdl");
    ent.Spawn();
    ent.Activate();

    float ang[3] = { 0.0, 0.0, 0.0 };
    ang[1] = 46.0;
    PrintToServer("prop_dynamic");
    ent.SetAbsAngles(ang);
    ent.GetAbsAngles(ang);
    PrintToServer("angles: %0.2f %0.2f %0.2f", ang[0], ang[1], ang[2]);
    ang[1] = 92.0;
    ent.SetAbsAngles(ang);
    ent.GetAbsAngles(ang);
    PrintToServer("angles: %0.2f %0.2f %0.2f", ang[0], ang[1], ang[2]);
    ent.AcceptInput("Kill");

    CBaseNPC npc = new CBaseNPC();
    ent = CBaseCombatCharacter(npc.GetEntity());
    ent.SetModel("models/player/pyro.mdl");
    ent.Spawn();
    ent.Activate();

    ang[1] = 46.0;
    PrintToServer("CBaseNPC");
    ent.SetAbsAngles(ang);
    ent.GetAbsAngles(ang);
    PrintToServer("angles: %0.2f %0.2f %0.2f", ang[0], ang[1], ang[2]);
    ang[1] = 92.0;
    ent.SetAbsAngles(ang);
    ent.GetAbsAngles(ang);
    PrintToServer("angles: %0.2f %0.2f %0.2f", ang[0], ang[1], ang[2]);
    ent.AcceptInput("Kill");
}

Output:

prop_dynamic
angles: 0.00 46.00 0.00
angles: 0.00 92.00 0.00
CBaseNPC
angles: 0.00 46.00 0.00
angles: 0.00 92.00 0.00

Test case in C++:

#define DECLAREVAR(type, var) \
    static int32_t offset_##var; \
    inline type* var() const \
    { \
        return (type*)((uint8_t*)this + offset_##var); \
    } 

#define DEFINEVAR(classname, var) \
int32_t (classname::classname::offset_##var) = 0

class MyClass
{
public:
    DECLAREVAR(matrix3x4_t, m_rgflCoordinateFrame)

    matrix3x4_t m_matrix;
};

DEFINEVAR(MyClass, m_rgflCoordinateFrame);

void EditMatrix(matrix3x4_t& matrix)
{
    matrix[3][3] = 67.0;
}

void EditVar(float& var)
{
    var = 56.0;
}

int main() {
    float* var = new float;
    *var = 12.0;
    EditVar(*var);

    std::cout << *var << '\n';

    MyClass::offset_m_rgflCoordinateFrame = offsetof(MyClass, m_matrix);

    MyClass* pEnt = new MyClass;
    EditMatrix(*(pEnt->m_rgflCoordinateFrame()));

    std::cout << (*(pEnt->m_rgflCoordinateFrame()))[3][3] << '\n';

    return 0;
}

Output:

56
67
Kenzzer commented 2 years ago

I opened that issue following the talk on this discord regarding issues with m_angAbsRotation, you might be right but this might also just be compiler optimization, I was waiting to read the C++ standard tonight and ensure this is intended behaviour, because there's nothing that prevents a compiler implementation to sugar syntax this func(*ptr) into

int a = *(int *)ptr
func(a)
Kenzzer commented 2 years ago

Finally got around it. And I was wrong in my assumption, this is well in bound behavior.

However this means I've to clean up the line right under it https://github.com/TF2-DMB/CBaseNPC/blob/7c983f90478a15ff6f25c1d65500571e96fb4f40/extension/sourcesdk/baseentity.cpp#L361 Made myself a note. Closed.