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

Mordrag can be told to "get out" in New Camp #225

Merged
merged 1 commit into from May 11, 2021
Merged

Mordrag can be told to "get out" in New Camp #225

merged 1 commit into from May 11, 2021

Conversation

AmProsius
Copy link
Owner

@AmProsius AmProsius commented May 11, 2021

Describe the bug
Mordrag can be told to "get out of here" even if he guided the player to the New Camp.

Expected behavior
Mordrag can no longer be told to "get out of here" if he guided the player to the New Camp.

Additional context
Bug provided by Blubbler.

Reference for the proposed fix:

FUNC int Org_826_Mordrag_Fight_Condition()
{
if ( (Thorus_MordragKo == LOG_RUNNING) && (!Npc_KnowsInfo(hero,Org_826_Mordrag_GotoNewcamp)) )
{
return 1;
};
};

@AmProsius AmProsius added the validation: required label Apr 4, 2021
@AmProsius AmProsius added this to Dialog: Info condition in Fix templates Apr 4, 2021
@AmProsius AmProsius added compatibility: easy impl: hook script func type: session fix labels Apr 4, 2021
@AmProsius
Copy link
Owner Author

@AmProsius AmProsius commented Apr 4, 2021

FUNC int Org_826_Mordrag_HauAb_Condition()
{
VAR C_NPC Mordrag;
Mordrag = Hlp_GetNpc(Org_826_Mordrag);
if (Mordrag.aivar[AIV_WASDEFEATEDBYSC] >= 1)
{
return 1;
};
};

changed to

FUNC int  Org_826_Mordrag_HauAb_Condition()
{
    VAR C_NPC Mordrag;
    Mordrag = Hlp_GetNpc(Org_826_Mordrag);

    if (Mordrag.aivar[AIV_WASDEFEATEDBYSC] >= 1)
    && (Thorus_MordragKo == LOG_RUNNING)
    && (!Npc_KnowsInfo(hero,Org_826_Mordrag_GotoNewcamp))
    {
        return 1;
    };
};

@AmProsius AmProsius added the provided fix label Apr 4, 2021
@AmProsius AmProsius changed the title Mordrag can be told to "get out" in the New Camp Mordrag can be told to "get out" in New Camp Apr 4, 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 AmProsius merged commit 8ddf052 into master May 11, 2021
v1.2.0 automation moved this from To Do to Done May 11, 2021
@AmProsius AmProsius deleted the bug225 branch May 11, 2021
@AmProsius AmProsius removed their assignment May 12, 2021
Copy link
Collaborator

@szapp szapp left a comment

@AmProsius this will need some fixing.

docs/changelog_de.md Show resolved Hide resolved
src/Ninja/G1CP/Content/Tests/test225.d Show resolved Hide resolved

// Call dialog condition function
G1CP_Testsuite_Call(funcId, npc, hero, FALSE);
var int ret; ret = MEM_PopIntResult();
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.

To cover all ends, this test is incomplete. The fix introduces two new or-conditions for which the dialog condition should fail, but the dialog condition is only tested here once, for both of them at the same time. The test should have two passes (i.e. calling the dialog condition twice), for either of the two conditions. Only that will fully cover testing the fix.

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 fix introduces two new and-conditions, so I don't see the problem here. Or am I mistaken?

Copy link
Collaborator

@szapp szapp May 15, 2021

Choose a reason for hiding this comment

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

Yes, you are right. The dialog condition returns true if both new conditions are met, but the dialog condition returns false if either of the new conditions if false. The test currently confirms that the dialog condition returns false if the dialog "Org_826_Mordrag_GotoNewcamp" was told. What the test should also check is, that the dialog condition also returns false if only the variable "Thorus_MordragKo" is not set to LOG_RUNNING.

Copy link
Owner Author

@AmProsius AmProsius May 15, 2021

Choose a reason for hiding this comment

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

Ah I see. Thanks for the explanation, I will update the test accordingly.

Copy link
Collaborator

@szapp szapp May 15, 2021

Choose a reason for hiding this comment

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

Thanks!

@AmProsius
Copy link
Owner Author

@AmProsius AmProsius commented May 14, 2021

Okay, thanks for the review! I based this fix and test on #153 (fix of #33), so I guess I'll have to update that fix and test as well.

@szapp
Copy link
Collaborator

@szapp szapp commented May 14, 2021

Good catch. The fix of #33 is slightly different. Here we have two independent and-conditions (if either is false return false), in #33 we have two or-conditions (if both are false return false), compare this with this.

It may not hurt to negate the conditions in #33, but also does not seem necessary (there are no double negations). The symbol check in #33 is important and should remain. Unlike here, the existence of the symbol has to be checked again, because it is not guaranteed that it exists:

&& ((G1CP_IsInfoInst("DIA_Gorn_Hut")) || (G1CP_IsIntVar("Gorn_ShrikesHut", 0))) { // Either one must exist

As for the test of #33, it does not need two passes, because there are or-conditions. However, you are right, this line should be adjusted as mentioned above:

@szapp szapp removed the validation: required label May 14, 2021
@AmProsius AmProsius added the validation: validated label May 14, 2021
@szapp szapp moved this from Modify dialog conditions to Add dialog conditions in Fix templates Feb 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compatibility: easy impl: hook script func provided fix type: session fix validation: validated
Projects
Fix templates
Add dialog conditions
v1.2.0
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

2 participants