Skip to content
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

Exploit: Undead orc priest can die from fall damage #224

Merged
merged 5 commits into from May 9, 2021
Merged

Exploit: Undead orc priest can die from fall damage #224

merged 5 commits into from May 9, 2021

Conversation

AmProsius
Copy link
Owner

@AmProsius AmProsius commented May 9, 2021

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.

@AmProsius AmProsius added the validation: required label Apr 3, 2021
@szapp
Copy link
Collaborator

@szapp szapp commented Apr 5, 2021

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
Copy link

@N1kX94 N1kX94 commented Apr 6, 2021

Gothic Speedrun

you can see it in the video.

@szapp
Copy link
Collaborator

@szapp szapp commented Apr 6, 2021

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.

@szapp szapp added compatibility: easy provided fix impl: change obj var type: revert on save labels Apr 7, 2021
@szapp szapp added this to NPC property in Fix templates Apr 7, 2021
@AmProsius AmProsius changed the title Undead orc priest can die from fall damage Exploit: Undead orc priest can die from fall damage Apr 8, 2021
@szapp szapp added provided implementation and removed provided fix labels Apr 12, 2021
@AmProsius AmProsius self-assigned this Apr 17, 2021
@AmProsius AmProsius added this to To Do in v1.2.0 Apr 17, 2021
@AmProsius AmProsius added this to the v1.2.0 milestone Apr 18, 2021
@AmProsius
Copy link
Owner Author

@AmProsius AmProsius commented May 9, 2021

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

@AmProsius AmProsius marked this pull request as draft May 9, 2021
@szapp
Copy link
Collaborator

@szapp szapp commented May 9, 2021

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
Copy link
Owner Author

@AmProsius AmProsius commented May 9, 2021

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.

CHANGELOG.md Outdated Show resolved Hide resolved
@AmProsius AmProsius marked this pull request as ready for review May 9, 2021
@AmProsius AmProsius merged commit 07ddc43 into master May 9, 2021
v1.2.0 automation moved this from To Do to Done May 9, 2021
@AmProsius AmProsius deleted the bug224 branch May 9, 2021
@AmProsius AmProsius removed their assignment May 12, 2021
Copy link
Collaborator

@szapp szapp left a comment

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

};

npc.fallDownDamage = 0;
return TRUE;
Copy link
Collaborator

@szapp szapp May 13, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Owner Author

@AmProsius AmProsius May 14, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@szapp szapp removed the validation: required label May 14, 2021
@szapp szapp moved this from NPC property (volatile) to NPC properties (persistent) in Fix templates Feb 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compatibility: easy impl: change obj var provided implementation type: revert on save
Projects
Fix templates
Change NPC property (non-volatile)
v1.2.0
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

3 participants