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

Guard has duplicated END dialog #128

Merged
merged 4 commits into from Jul 27, 2021
Merged

Guard has duplicated END dialog #128

merged 4 commits into from Jul 27, 2021

Conversation

AmProsius
Copy link
Owner

@AmProsius AmProsius commented Jul 17, 2021

Describe the bug
The guard right next to Glen in the old mine has two ending dialogue options.

Expected behavior
There should be just one option to end the dialogue.

Additional context
Instance Grd_264_Gardist.

@szapp
Copy link
Collaborator

@szapp szapp commented Feb 16, 2021

Similar to #42.

@szapp szapp added the type: session fix label Feb 16, 2021
@AmProsius AmProsius changed the title Grd_264_Gardist has a second ending option Guard has duplicated END dialog Feb 18, 2021
@AmProsius AmProsius added the validation: required label Feb 20, 2021
@szapp szapp added compatibility: easy impl: hook script func impl: modify/analyze script func labels Apr 5, 2021
@szapp szapp added this to Dialog: Info condition in Fix templates Apr 5, 2021
@szapp szapp added the provided fix label Apr 12, 2021
@AmProsius AmProsius added this to the v1.2.0 milestone May 7, 2021
@AmProsius AmProsius added this to To Do in v1.2.0 via automation May 7, 2021
@AmProsius AmProsius self-assigned this May 7, 2021
@AmProsius AmProsius moved this from To Do to In Progress in v1.2.0 Jul 17, 2021
@AmProsius AmProsius requested a review from szapp Jul 17, 2021
@AmProsius AmProsius removed their assignment Jul 17, 2021
Copy link
Collaborator

@szapp szapp left a comment

I think it's quite elegant to piggyback #42!

However, the fix status of #42 will mark as failed if the two dialogs in #42 are in fact fixed but the one from #128 is not. Same for the opposite. That is highly misleading and could cause a lot of headaches during testing.

I propose to rather copy and adjust the function G1CP_042_GuardExitDialog while re-using the function G1CP_042_ConfirmByteCode (no need to copy that one).

@AmProsius
Copy link
Owner

@AmProsius AmProsius commented Jul 19, 2021

Wouldn't it be even better then to split the original #42 in two separate fixes as well?

@szapp
Copy link
Collaborator

@szapp szapp commented Jul 19, 2021

Wouldn't it be even better then to split the original #42 in two separate fixes as well?

Yes, I agree. This would allow for best inspection during testing. But I fear that it would convolute the fix list / changelog, with many identical entries. An alternative is to collect all of them in one fix, as done in #42 so far.

Both options have their pros and cons, I will leave this decision up to you.

@AmProsius AmProsius requested a review from szapp Jul 22, 2021
@szapp
Copy link
Collaborator

@szapp szapp commented Jul 22, 2021

Thanks for taking care of it. There are a few things that may not work with the new changes. I don't have the time to get into it right now, but hold out before merging this PR.

docs/changelog_de.md Outdated Show resolved Hide resolved
src/Ninja/G1CP/Content/Misc/npc.d Outdated Show resolved Hide resolved
src/Ninja/G1CP/Content/Bytecode/return.d Outdated Show resolved Hide resolved
src/Ninja/G1CP/Content_G1.src Outdated Show resolved Hide resolved
@AmProsius AmProsius requested a review from szapp Jul 27, 2021
szapp
szapp approved these changes Jul 27, 2021
AmProsius added a commit that referenced this issue Jul 27, 2021
AmProsius added a commit that referenced this issue Jul 27, 2021
@AmProsius AmProsius merged commit 5a05caa into master Jul 27, 2021
v1.2.0 automation moved this from In Progress to Done Jul 27, 2021
@AmProsius AmProsius deleted the bug128 branch Jul 27, 2021
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 impl: modify/analyze script func provided fix type: session fix validation: required
Projects
Fix templates
Modify dialog conditions
v1.2.0
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

2 participants