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

Swiney gives his own pants to the player #181

Merged
merged 2 commits into from Mar 31, 2021
Merged

Swiney gives his own pants to the player #181

merged 2 commits into from Mar 31, 2021

Conversation

szapp
Copy link
Collaborator

@szapp szapp commented Mar 29, 2021

Describe the bug
When Swiney gives a Digger's Dress to the player, he gives him his own equipped Digger's Dress.

Expected behavior
Swiney no longer loses his own Digger's Dress when he gives one to the player.

@AmProsius AmProsius added the validation: required This issue needs validation from one of the validators. label Mar 5, 2021
@AmProsius
Copy link
Owner Author

AmProsius commented Mar 5, 2021

func void Info_Swiney_Schuerfer_Ja()
{
AI_Output (other, self,"Info_Swiney_Schuerfer_Ja_15_00"); //Yes.
AI_Output (self, other,"Info_Swiney_Schuerfer_Ja_09_01"); //Good! Then you know what to watch out for. Here's your protective clothin'. You'll have to find your own pickaxe. Enjoy yourself.
CreateInvItem (self, SFB_ARMOR_L);
B_GiveInvItems(self, hero, SFB_ARMOR_L, 1);
Info_ClearChoices(Info_Swiney_Schuerfer);
};

changed to

func void Info_Swiney_Schuerfer_Ja()
{
    AI_Output (other, self,"Info_Swiney_Schuerfer_Ja_15_00"); //Yes.
    AI_Output (self, other,"Info_Swiney_Schuerfer_Ja_09_01"); //Good! Then you know what to watch out for. Here's your protective clothin'. You'll have to find your own pickaxe. Enjoy yourself.
    CreateInvItem(other, SFB_ARMOR_L);

    CreateInvItem(self, ItAmArrow);
    B_GiveInvItems(self, hero, ItAmArrow, 1);
    Npc_RemoveInvItem(hero, ItAmArrow);

    Info_ClearChoices(Info_Swiney_Schuerfer);
};

func void Info_Swiney_Schuerfer_Nein()
{
AI_Output (other, self,"Info_Swiney_Schuerfer_Nein_15_00"); //No.
AI_Output (self, other,"Info_Swiney_Schuerfer_Nein_09_01"); //Of course you don't! Why's it always me? Okay. Listen carefully. You take this stuff and put it on - here.
AI_Output (self, other,"Info_Swiney_Schuerfer_Nein_09_02"); //If an ore nugget falls on your foot while you're not wearing your protective clothing, you'll be crippled for life and only worth half of what you were before to us.
AI_Output (self, other,"Info_Swiney_Schuerfer_Nein_09_03"); //There'll be a pickaxe lying somewhere around here.
AI_Output (self, other,"Info_Swiney_Schuerfer_Nein_09_04"); //But you won't be able to do much with it - well - so what...
CreateInvItem (self, SFB_ARMOR_L);
B_GiveInvItems(self, hero, SFB_ARMOR_L, 1);
Info_ClearChoices(Info_Swiney_Schuerfer);
};

changed to

func void Info_Swiney_Schuerfer_Nein()
{
    AI_Output (other, self,"Info_Swiney_Schuerfer_Nein_15_00"); //No.
    AI_Output (self, other,"Info_Swiney_Schuerfer_Nein_09_01"); //Of course you don't! Why's it always me? Okay. Listen carefully. You take this stuff and put it on - here.
    AI_Output (self, other,"Info_Swiney_Schuerfer_Nein_09_02"); //If an ore nugget falls on your foot while you're not wearing your protective clothing, you'll be crippled for life and only worth half of what you were before to us.
    AI_Output (self, other,"Info_Swiney_Schuerfer_Nein_09_03"); //There'll be a pickaxe lying somewhere around here.
    AI_Output (self, other,"Info_Swiney_Schuerfer_Nein_09_04"); //But you won't be able to do much with it - well - so what...

    CreateInvItem(other, SFB_ARMOR_L);

    CreateInvItem(self, ItAmArrow);
    B_GiveInvItems(self, hero, ItAmArrow, 1);
    Npc_RemoveInvItem(hero, ItAmArrow);

    Info_ClearChoices(Info_Swiney_Schuerfer);
};

@AmProsius AmProsius added provided fix This issue has a fix provided in the comments. validation: validated This issue is still present even with Systempack/Union. and removed validation: required This issue needs validation from one of the validators. labels Mar 5, 2021
@AmProsius
Copy link
Owner Author

AmProsius commented Mar 5, 2021

An alternative approach would be that B_GiveInvItems ignores equipped items, but I don't know if that might have unforeseen consequences.

@szapp
Copy link
Collaborator

szapp commented Mar 6, 2021

Typically, whenever an NPC gives the PC an armor they are wearing themselves, it is done the following way (there are multiple times that exact thing is done in the scripts). A surrogate item is created, transferred, and then removed for display of the desired text, while the armor itself is created in the inventory of the PC. Optionally, the best armor is equipped subsequently.

CreateInvItem (self, ItAmArrow);
B_GiveInvItems (self, hero, ItAmArrow, 1);
Npc_RemoveInvItem (hero, ItAmArrow);
AI_EquipBestArmor (hero);

I doubt this will be the best approach here, because a mod might have specifically changed the screen print in B_GiveInvItems to reflect the name of the item transferred.

Another way would be to place the armor into the inventory of the PC and supply the screen print manually. However, that is dangerous, because a mod might have changed the way the display works (positioning, font, font size, exact wording, ...).

Best would be to fix the external engine function Npc_RemoveInvItems (the function underlying B_GiveInvItems) to check if the item is in the inventory multiple times and then choose an unequipped one among them (if available). That might be a lot of work and could be in conflict with a similar fix implemented by a third party.

I think the easiest fix might be to use a surrogate NPC to transfer the armor from. This could look like this:

    GetItemHelper();
    CreateInvItem(Item_Helper, SFB_ARMOR_L); 
    B_GiveInvItems(Item_Helper, hero, SFB_ARMOR_L, 1); 

Item_Helper is provided already through GetItemHelper.

@AmProsius
Copy link
Owner Author

Best would be to fix Npc_RemoveInvItems (the function underlying B_GiveInvItems) to check if the item is in the inventory multiple times and then choose an unequipped one among them (if available).

I agree. I will update the issue accordingly.

@szapp
Copy link
Collaborator

szapp commented Mar 6, 2021

I agree. I will update the issue accordingly.

I updated my comment above. The fix might be very easy.

@AmProsius
Copy link
Owner Author

AmProsius commented Mar 6, 2021

If we used the Item_Helper, we would have to fix every item giving issue one by one, whereas fixing Npc_RemoveInvItems might fix all of them at once. I'm not totally convinced yet that this is the best way.

@szapp
Copy link
Collaborator

szapp commented Mar 6, 2021

If we used the Item_Helper, we would have to fix every item giving issue one by one, whereas fixing Npc_RemoveInvItems might fix all of them at once. I'm not totally convinced yet that this is the best way.

But are there any other instances of that issue? The only time transferring an item is a problem is when an item is to be transferred that the NPC has equipped themselves, usually armor, and there seem to only be a few instances where that is relevant - most of them already fixed. Or am I wrong?

@AmProsius
Copy link
Owner Author

I'll have to check that. Let me make a list of armor dealers/givers and see if the issue is present somewhere else. If not, we can gladly use the Item_Helper.

@AmProsius
Copy link
Owner Author

I didn't find any other occasion. In fact, I found comments why PB used the arrow workaround:

CreateInvItem (hero,SLD_ARMOR_H); //SN: ohne B_GiveInvItem, weil sonst Lee nackt dasteht!
// nur wegen Bildschirmausgabe "1 Gegenstand erhalten"
CreateInvItem (self, ItAmArrow);
B_GiveInvItems (self, hero, ItAmArrow, 1);
Npc_RemoveInvItem (hero, ItAmArrow);

So it looks like they forgot to do this with Swiney and I agree on your proposed solution. If it doesn't work, we can still create the arrow workaround.

@szapp szapp added compatibility: difficult This issue is difficult to make compatible. type: session fix The fix for this issues is persistent across a session. labels Mar 6, 2021
@szapp szapp self-assigned this Mar 19, 2021
@szapp szapp added impl: modify engine func This issue requires modifying the machine code of engine functions. impl: modify/analyze script func This issue requires analyzing and/or modifying the bytecode of script functions. labels Mar 19, 2021
@szapp szapp added this to Dialog: Info function in Fix templates Mar 19, 2021
@szapp szapp added compatibility: easy This issue is easy to make compatible. impl: hook script func This issue requires hooking script functions. and removed compatibility: difficult This issue is difficult to make compatible. impl: modify engine func This issue requires modifying the machine code of engine functions. impl: modify/analyze script func This issue requires analyzing and/or modifying the bytecode of script functions. labels Mar 29, 2021
@szapp szapp added this to the v1.1.0 milestone Mar 29, 2021
@szapp szapp added this to In Progress in v1.1.0 via automation Mar 29, 2021
@szapp
Copy link
Collaborator

szapp commented Mar 29, 2021

I fixed the issue differently from discussed here: Instead of changing his "giving behavior", he merely re-equips his best armor right after the execution of the dialog. This should be instant and not visible.

I could not see him take off his armor. @AmProsius, could you test if you spot him taking it off and putting it on? The test is manual (but the dialog will be immediately available).

Update: I accidentally fixed this bug, although it was not part of v1.1.0. I added it there such that we don't forget about it later.

@szapp szapp requested a review from AmProsius March 29, 2021 09:06
@szapp szapp removed their assignment Mar 29, 2021
@AmProsius AmProsius merged commit 69c7ef1 into master Mar 31, 2021
v1.1.0 automation moved this from In Progress to Done Mar 31, 2021
@AmProsius AmProsius deleted the bug181 branch March 31, 2021 10:25
szapp added a commit that referenced this pull request Apr 19, 2021
@szapp szapp moved this from Modify dialog function to Extend dialog function 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 This issue is easy to make compatible. impl: hook script func This issue requires hooking script functions. provided fix This issue has a fix provided in the comments. type: session fix The fix for this issues is persistent across a session. validation: validated This issue is still present even with Systempack/Union.
Projects
Fix templates
Extend dialog function
v1.1.0
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

2 participants