Exploit: Undead orc priest can die from fall damage

[AmProsius]

Describe the bug
Grash-Varrag-Arushat (the last undead orc priest) can die by fall damage rather than by Uriziel damage only.

Expected behavior
Grash-Varrag-Arushat (the last undead orc priest) can now only be killed with Uriziel.

Additional context
Bug provided by Blubbler.

[szapp]

Grash Varrag Arushat (the last undead orc priest) can now only be killed with Uriziel.

Was there already some suggestion on how that can be ensured?

I heard about this "trick". You can somehow make him walk off the cliff. But I don't know how to trigger it. The circumstances might be important to understand the problem.

[N1kX94]

Gothic Speedrun

you can see it in the video.

[szapp]

Thanks for the video. I think the easiest fix would be to make that NPC resistant to fall damage. If the player manages to drive the NPC off the cliff, they will stay alive. The player would then be forced to leave the area and return to find the NPC spawned on their original way point again. The bug could no longer be exploited.

If that works (and we decide that this solution is sufficient), this fix would be a revertible fix where the orc priest NPC is determined on loading, cast to oCNpc and the property oCNpc.fallDownDamage (integer, see Ikarus class definitions) is set to zero. I am not sure if that will have to be reverted, because these values might be read from the guild attributes not from the savegame.

---

This is a draft for the implementation:

/*
 * #224 Undead orc priest can die from fall damage
 */

/*
 * Retrieve the symbol index of the NPC
 */
func int G1CP_224_OrcPriestFallDamage_GetInst() {
    const int npcId = -2; // -1 is reserved for invalid symbols
    if (npcId == -2) {
        npcId = G1CP_GetNpcInstId("ORC_Priest_5");
    };
    return npcId;
};

/*
 * This function applies the changes
 */
func int G1CP_224_OrcPriestFallDamage() {
    var oCNpc npc; npc = Hlp_GetNpc(G1CP_224_OrcPriestFallDamage_GetInst());
    if (Hlp_IsValidNpc(npc) {
        if (npc.fallDownDamage == 10) {
            npc.fallDownDamage = 0;
            return TRUE;
        };
    };
    return FALSE;
};

/*
 * This function reverts the changes. Not necessary here, but for completeness and proper applied-status.
 */
func int G1CP_224_OrcPriestFallDamageRevert() {
    // Only revert if it was applied by the G1CP
    if (!G1CP_IsFixApplied(224)) {
        return FALSE;
    };

    var oCNpc npc; npc = Hlp_GetNpc(G1CP_224_OrcPriestFallDamage_GetInst());
    if (Hlp_IsValidNpc(npc) {
        if (npc.fallDownDamage == 0) {
            npc.fallDownDamage = 10;
            return TRUE;
        };
    };
    return FALSE;
};

As for a test, I don't see any other option than to teleport the player to the waypoint of the orc priest (see other tests on how to auto-trigger a level change to the orc temple beforehand) and to mimic the speedrunner exploit as shown in the video above.

What will have to be tested is, whether the fall damage is reset when the player moves out of spawn distance of the orc priest. Therefore, during testing try a few combinations of level changes, saving and loading and moving into spawn distance, out of spawn distance and back into spawn distance.

[AmProsius]

I wasn't able to reproduce the test case yet, because I lose focus before the undead orc hits the ground.

[szapp]

I wasn't able to reproduce the test case yet, because I lose focus before the undead orc hits the ground.

I think the trick of this exploit is to hold CTRL or LMB before the NPC falls, to maintain the focus.

[AmProsius]

I did that, but after a few moments of him falling, I lose focus nonetheless. I can give you a savegame if you want to test it yourself.

[szapp]

@AmProsius some thoughts on exiting-early for this particular case and fix functions in general.

[szapp]

Although I understand the argument of avoiding too deeply nested if-blocks, I really feel that it is, on the one hand not necessary (it's just one nested if-block), and on the other hand creates three exit points of the function. Most importantly it creates much more code than necessary (and writing such a fix more work), and makes a concise and easily readable function much more complex and more difficult to follow at first glance.

The previous flow of reading was "Set fallDownDamage to zero if all conditions are met", i.e. only do something if certain conditions are true. Now it is "If the Npc is invalid, return. If the fallDownDamage is not zero, return. Set the fallDownDamage to zero." The logical and semantic flow is lost and it takes a few moments to put these separated if-blocks together. By the multiple exit points of the functions it is harder to spot when there is actually something happening.

Semantically, I agree that "exiting early, when expected conditions are not met" makes a lot of sense for test functions (as they are currently mostly implemented). But for fix functions, it makes more sense the other way around: "fix something if all conditions are met". Only in the off-chance of stacking very, very many conditions or checking something general, e.g. if symbols exist, then it makes sense.

[AmProsius]

The more important aspect for me is that the primary action is at the top level of the function, not "hidden" inside nested conditions. But I will most certainly think about this.