Possible problem related to constraints while loading objects

[Hamlet]

This is another report originating from a GCC warning, in this case the comparison of two different enumerators.
This works by converting each of the two enumerators into a numeric value, and comparing them instead.
In general, an enumerator is better used in a way that does not depend from its value at all. If the value is needed, constants are generally better suited.
The relevant one of the two warnings comes from idAF::Load() in game/AF.cpp (line 910), where a variable of enumerator type declAFConstraintType_t from framework/DeclAF.h:

typedef enum {
  DECLAF_CONSTRAINT_INVALID,
  DECLAF_CONSTRAINT_FIXED,
  DECLAF_CONSTRAINT_BALLANDSOCKETJOINT,
  DECLAF_CONSTRAINT_UNIVERSALJOINT,
  DECLAF_CONSTRAINT_HINGE,
  DECLAF_CONSTRAINT_SLIDER,
  DECLAF_CONSTRAINT_SPRING
} declAFConstraintType_t;

is compared with a variable of enumerator type constraintType_t from game/physics/Physics_AF.h:

typedef enum {
        CONSTRAINT_INVALID,
        CONSTRAINT_FIXED,
        CONSTRAINT_BALLANDSOCKETJOINT,
        CONSTRAINT_UNIVERSALJOINT,
        CONSTRAINT_HINGE,
        CONSTRAINT_HINGESTEERING,
        CONSTRAINT_SLIDER,
        CONSTRAINT_CYLINDRICALJOINT,
        CONSTRAINT_LINE,
        CONSTRAINT_PLANE,
        CONSTRAINT_SPRING,
        CONSTRAINT_CONTACT,
        CONSTRAINT_FRICTION,
        CONSTRAINT_CONELIMIT,
        CONSTRAINT_PYRAMIDLIMIT,
        CONSTRAINT_SUSPENSION
} constraintType_t;

The comparison of values from two distinct enumerator types should be avoided because it requires the enumerator modifications to be synchronised, but it's very tempting to add an element to one forgetting about the other.
If this comparison is really required, there should be a single enumerator type.
Judging from the enumerator names, this departure from synchronisation has already happened (I would except a DECLAF_CONSTRAINT_SLIDER to be equivalent to a CONSTRAINT_SLIDER, and instead it's compared to a CONSTRAINT_HINGESTEERING).
The two definitions are in different parts of the code and do not apparently share headers. But more to the point, for what I know these numerical values might be serialised, and changing them would break saved data.
My recommended suggestions would be:

• if possible, manage to have a single enumerator for both uses
• if not, abandon direct comparison and use a slower multiple-choice comparison function.

I provide a patch implementing the latter suggestion, which is expected not to break any serialised data, by not changing the enumerator values. Note that this function relies on the heuristic logic of my brain to match the elements from the two enumerators. Somebody with some understanding should cross check it.
The patch also addresses the other enumerator comparison, which is more subtle and is caused by the fact that enumerators defined in a template class yield one different enumerator type for each template instantiation.

enumCompare.patch.txt

[Obsttorte]

I can see the issue of comparing two distinct, non-numerical variables.

 

However, what issue does it cause?

[Hamlet]

However, what issue does it cause?

This is the kind of question hard for one like me with no experience in anything of The Dark Mod to answer. In fact, I was hoping some other reader could connect more dots.

My contemplation of the code tells me that "AF" is Articulated Figures, which I may think objects which are not rigid. In this sense, it makes sense to have constraints between the rigid parts of the figure, which is what both enumerators enumerate. The issue is shown in idAF::Load(idEntity *ent, const char *fileName), which suggests it's trying to load data from a file. It is in a double loop where constraints are deleted from a physics object. So I guess what could happen is that the wrong constraints are deleted and the object does not react like it should to being dragged around.

[Obsttorte]

Your correct in your interpretation. AF's are non-rigid bodies like corpses or ropes. The constraints are used to limit the rotation of the connected bodyparts. They are, however working as they should. As far as I can tell we don't have any issues there.