codenamecpp / carnage3d

Reimplementation of Grand Theft Auto 1 [GTA1]
MIT License
470 stars 38 forks source link

Blood sometimes moving away from corpses #47

Closed MiroslavR closed 3 years ago

MiroslavR commented 3 years ago

I haven't found stable reproduction steps, however what usually works is to fire a rocket at a wall, then start killing pedestrians. I think it may have something to do with the smoke effect from explosions because the blood splatters appear to gain the same velocity.

codenamecpp commented 3 years ago

Thanks! It's should be fixed now.

MiroslavR commented 3 years ago

@codenamecpp It does indeed seem to be fixed, thanks for looking into it!

BTW, I'm staring at the commit's changes and can't for the life of me figure out what the problem was. :smile: All of changes (with the possible exception of mAnimationState()) seem redundant to me. Was it Decoration's velocity somehow being left uninitialized? I would expect doing mMoveVelocity() to be redundant for a glm::vec3.

codenamecpp commented 3 years ago

I am not sure for 100%, but sometimes when adding new fields to class, they remains uninitialized for some reason even I do this in header file: class SomeClass { int mNewField = 5; }; When debugging I can see that mNewField has a random value. Only full project rebuild helps. Compiler optimizing compilation times probably?

Initialize fields in constructor in cpp file solves problem.

MiroslavR commented 3 years ago

I don't think it's that. Even after a full rebuild, the problem persists (without the fix of course). It seems glm just does not initialize its members in vec3 constructor. Or rather, the behavior is fenced behind an #if:

#   if GLM_CONFIG_DEFAULTED_FUNCTIONS == GLM_DISABLE
        template<typename T, qualifier Q>
        GLM_FUNC_QUALIFIER GLM_CONSTEXPR vec<3, T, Q>::vec()
#           if GLM_CONFIG_CTOR_INIT != GLM_CTOR_INIT_DISABLE
                : x(0), y(0), z(0)
#           endif
        {}

        template<typename T, qualifier Q>
        GLM_FUNC_QUALIFIER GLM_CONSTEXPR vec<3, T, Q>::vec(vec<3, T, Q> const& v)
            : x(v.x), y(v.y), z(v.z)
        {}
#   endif

But explicitly doing mMoveVelocity() will still value initialize the float members to 0, that's just how C++ works apparently.

MiroslavR commented 3 years ago

I might have worded it in a confusing way. It seems the glm provided by my system by default has these two constructors defaulted. So:

#include <glm/glm.hpp>

int main()
{
    glm::vec3 v1; // Uninitialized
    glm::vec3 v2{}; // 0, 0, 0
}

There is apparently a define GLM_FORCE_CTOR_INIT that can be used to have the following result:

#define GLM_FORCE_CTOR_INIT
#include <glm/glm.hpp>

int main()
{
    glm::vec3 v1; // 0, 0, 0
    glm::vec3 v2{}; // 0, 0, 0
}
codenamecpp commented 3 years ago

It's not obvious to me, why in the first example v1 will be uninitialized, isn't it the same constructor will be called for both v1 and v2 ?

MiroslavR commented 3 years ago

The way I understand it, in the first example v1 will not have any constructor called and v2 will have a compiler-generated constructor called (which will default initialize members). I think this just applies to PODs (plain old data types). In the second example, due to GLM_FORCE_CTOR_INIT being defined the constructor would be user-defined and the structure would not be considered a POD. You can verify that std::is_pod<glm::vec3> returns true in the first example and false in the second example.

As to why it works the way it does, it is not obvious to me either. Maybe this behavior is inherited from C? Same goes for primitive types like float, which are also considered PODs:

float a; // Uninitialized
float b{}; // 0

Take what I say with a grain of salt, though. I am by no means a C++ expert, in fact I've just learned about this behavior.