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

Graham not listed as merchant #203

Merged
merged 1 commit into from Apr 17, 2021
Merged

Graham not listed as merchant #203

merged 1 commit into from Apr 17, 2021

Conversation

AmProsius
Copy link
Owner

@AmProsius AmProsius commented Apr 16, 2021

Describe the bug
Graham is not listed in "Diary/General Info/Merchant in the Old Camp" if the topic wasn't previously created.

Expected behavior
Graham is now listed in "Merchant in the Old Camp" in the journal.

Additional context
Similar to #37.

@AmProsius AmProsius added the validation: required label Mar 26, 2021
@AmProsius
Copy link
Owner Author

@AmProsius AmProsius commented Mar 26, 2021

FUNC VOID DIA_Graham_Hello_Info()
{
AI_Output (other, self,"DIA_Graham_Hello_15_00"); //Hi! I'm new here.
AI_Output (self, other,"DIA_Graham_Hello_02_01"); //I'm Graham. I draw maps. I haven't seen you here before... What do you want?
B_LogEntry( GE_TraderOC,"The digger Graham sells maps left of the main gate.");
};

changed to

FUNC VOID DIA_Graham_Hello_Info()
{
    AI_Output (other, self,"DIA_Graham_Hello_15_00"); //Hi! I'm new here.
    AI_Output (self, other,"DIA_Graham_Hello_02_01"); //I'm Graham. I draw maps. I haven't seen you here before... What do you want?

    Log_CreateTopic(GE_TraderOC, LOG_NOTE);
    B_LogEntry( GE_TraderOC,"The digger Graham sells maps left of the main gate.");
};

@AmProsius AmProsius added compatibility: difficult provided fix impl: modify/analyze script func type: revert on save labels Mar 26, 2021
@AmProsius AmProsius self-assigned this Mar 26, 2021
@AmProsius AmProsius added this to To Do in v1.1.0 via automation Mar 26, 2021
@AmProsius AmProsius added this to Dialog: Info function in Fix templates Mar 26, 2021
@AmProsius AmProsius added this to the v1.1.0 milestone Mar 26, 2021
@szapp
Copy link
Collaborator

@szapp szapp commented Mar 26, 2021

Thanks for adding the issue. For #37 I chose to refer to the "log" as the "journal" (as it is visible to the player, i.e. "New Journal Entry") and simply wrote in the changelog:

Gravo is now correctly listed as merchant in the Old Camp in the journal.

It leaves out some details but keeps it short and maybe more intuitive than writing about "Diary/General Info/Merchant in the Old Camp" and referring to "topics" which are never called that way for the player.

Maybe could put that into quotes, though:

Gravo is now correctly listed in "Merchant in the Old Camp" in the journal.

@AmProsius
Copy link
Owner Author

@AmProsius AmProsius commented Mar 26, 2021

Gravo is now correctly listed in "Merchant in the Old Camp" in the journal.

Thanks, I've updated the other issues as well with this syntax.

@AmProsius
Copy link
Owner Author

@AmProsius AmProsius commented Apr 16, 2021

Both the fix and the test have not been tested ingame yet.

@AmProsius AmProsius marked this pull request as draft Apr 16, 2021
@AmProsius AmProsius requested a review from szapp Apr 16, 2021
@AmProsius AmProsius moved this from To Do to In Progress in v1.1.0 Apr 16, 2021
@AmProsius AmProsius marked this pull request as ready for review Apr 16, 2021
@AmProsius AmProsius removed their assignment Apr 16, 2021
szapp
szapp approved these changes Apr 17, 2021
Copy link
Collaborator

@szapp szapp left a comment

Very nice. I added some comments for later. But it's ready to merge (test passes).

G1CP_SetInfoTold(infoSymbName, toldBak);
r = G1CP_203_LogEntryGrahamMerchant();
G1CP_LogRemoveTopic(topic);
G1CP_LogRenameTopic(tempTopicName, topic);
Copy link
Collaborator

@szapp szapp Apr 17, 2021

Choose a reason for hiding this comment

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

One thing to keep in mind with this test (in case we use it somewhere else in the future), is that it breaks the fix-lookup-table. Applying and reverting the fix like done here, does not update the fix status. In this particular function, the fix status will remain "applied" even the fix is not. The fix-lookup-table will be corrupted from that point on.
Maybe we could backup the current status of the fix in the beginning with G1CP_GetFixStatus and set it in the end with G1CP_SetFixStatus.

Copy link
Collaborator

@szapp szapp Apr 26, 2021

Choose a reason for hiding this comment

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

Since this is just a test, I think this is not so important, but feel free to fix it

var int fixStatusBak; fixStatusBak = G1CP_GetFixStatus(203);

// ...

G1CP_SetFixStatus(203, fixStatusBak);

If you do fix this, there is some more tests where this can be done. But it's only applicable to tests where the fix-status is changed inside the apply function - this seems to only happen for these sorts of log entry fixes.

@szapp szapp merged commit fe1cb80 into master Apr 17, 2021
v1.1.0 automation moved this from In Progress to Done Apr 17, 2021
@szapp szapp deleted the bug203 branch Apr 17, 2021
@szapp szapp self-assigned this Apr 18, 2021
szapp added a commit that referenced this issue Apr 19, 2021
@szapp szapp removed their assignment Apr 24, 2021
@szapp szapp removed the validation: required label May 14, 2021
@szapp szapp moved this from Modify dialog function to Add missing log creation 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: difficult impl: modify/analyze script func provided fix type: revert on save
Projects
Fix templates
Create log topic in dialog function
v1.1.0
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

2 participants