[0.11.0] Potion Maker bugs [RESOLVED 0.11.3]

User avatar
pango
Posts: 2917
Joined: Wed Jul 18, 2018 6:14 pm
Location: France
Contact:

[0.11.0] Potion Maker bugs [RESOLVED 0.11.3]

Post by pango »

I've been trying to understand how TalonHatesNames could brew some potions ad libitum:
https://www.twitch.tv/videos/889687250?t=2h38m20s
I haven't found the reason yet; For the record he raided 4 or 5 alchemist shops before, transfering ingredients to his wagon halfway thru, so he must have lots of ingredients in both his inventory and wagon.

But I've found a much simpler case that's already troublesome; Steps to reproduce:
Having some stack of at least two identical ingredients in the inventory,
  • open potion maker interface
  • add the two items into the cauldron
  • close the potion maker interface
And notice there's only one item left in the inventory.

Explanation:
  • when adding the first ingredient, the stack is split by cloning, the stackCount of the original stack being decremented and the stackCount of the clone being one. The clone is put directly into the cauldron, so it''s neither in the ingredients nor, importantly, in player's inventory;
  • when adding the second and last ingredient, the stack of one item is moved from ingredients to the cauldron;
  • when the interface is Popped, ClearCauldron() is called, removing ingredients from the cauldron one by one, in the same order they have been put into the cauldron, so
  • the clone is removed first. Since it's not in ingredients, it's put back there;
  • the original stack is removed second. Since the same ingredient type is found in ingredients (the clone), the stackCount of the clone is incremented.
In the end the player's inventory contains the original stack with its decremented stackCount of 1.
It can sort-of be fixed by clearing ingredients in reverse order, but since the player is free to add and remove items between ingredients and claudron, it's not fool proof.

That seems tricky to fix reliably...
When a measure becomes a target, it ceases to be a good measure.
-- Charles Goodhart

User avatar
pango
Posts: 2917
Joined: Wed Jul 18, 2018 6:14 pm
Location: France
Contact:

Re: Potion Maker bugs

Post by pango »

Okay, I think I understand the bug that can be seen in the stream.
In MixCauldron(), after checking if the ingredients match a recipe (in the correct ingredients order, which seems too strict but that's another issue) all the ingredients in the cauldron must be consumed:
  • if the same kind of ingredient is found in ingredients list, then nothing is done, assuming that adding the ingredient to the cauldron already decremented the stackCount;
  • otherwise the ingredient is removed from both player's inventory and wagon (based on its ID, so it should only be removed from the list that contains that very ingredient).
The problem is that the ingredients list is a simple concatenation of player's inventory ingredients and player's wagon ingredients, so stacks of a kind of ingredient are not guaranteed to be unique in the list.
Say, ingredients of the same kind in the wagon should prevent the last ingredient in the player's inventory to be ever removed.
When a measure becomes a target, it ceases to be a good measure.
-- Charles Goodhart

User avatar
Interkarma
Posts: 6285
Joined: Sun Mar 22, 2015 1:51 am

Re: [0.11.0] Potion Maker bugs

Post by Interkarma »

Thanks Pango. Good catch and appreciate the detailed look at the problem already. Moving this to bug reports for followup.

User avatar
pango
Posts: 2917
Joined: Wed Jul 18, 2018 6:14 pm
Location: France
Contact:

Re: [0.11.0] Potion Maker bugs

Post by pango »

I think a good solution would be to not share items between player's inventory/wagon and ingredients list:
  • make ingredients list an ItemCollection;
  • fill it by Add()ing clones of player's ingredients. That will have the benefit of stacking inventory and wagon ingredients together on display;
  • adding/removing ingredients to the cauldron will be purely visual, in the sense that player's ingredients will now be unchanged;
  • when consuming cauldron's content, remove all ingredients from player's ingredients, say for each one try to remove the ingredient by kind from inventory, and if it fails, from the wagon
That's not as tightly optimized, but should be possible to get to work correctly.
When a measure becomes a target, it ceases to be a good measure.
-- Charles Goodhart

User avatar
Interkarma
Posts: 6285
Joined: Sun Mar 22, 2015 1:51 am

Re: [0.11.0] Potion Maker bugs

Post by Interkarma »

Good solution, I think that'll work nicely also. :)

User avatar
pango
Posts: 2917
Joined: Wed Jul 18, 2018 6:14 pm
Location: France
Contact:

Re: [0.11.0] Potion Maker bugs

Post by pango »

I submitted a PR for stack counts.

What about removing the requirement of ingredients being in recipe order in the cauldron for the mix to succeed?
One way to lift that constraint would be to change PotionRecipe.GetHashCode() (to be compatible with commutativity), but is it safe to do?
When a measure becomes a target, it ceases to be a good measure.
-- Charles Goodhart

User avatar
Hazelnut
Posts: 2698
Joined: Sat Aug 26, 2017 2:46 pm
Contact:

Re: [0.11.0] Potion Maker bugs

Post by Hazelnut »

pango wrote: I think a good solution would be to not share items between player's inventory/wagon and ingredients list:

make ingredients list an ItemCollection;
fill it by Add()ing clones of player's ingredients. That will have the benefit of stacking inventory and wagon ingredients together on display;
adding/removing ingredients to the cauldron will be purely visual, in the sense that player's ingredients will now be unchanged;
when consuming cauldron's content, remove all ingredients from player's ingredients, say for each one try to remove the ingredient by kind from inventory, and if it fails, from the wagon

That's not as tightly optimized, but should be possible to get to work correctly.
---
I submitted a PR for stack counts.

What about removing the requirement of ingredients being in recipe order in the cauldron for the mix to succeed?
One way to lift that constraint would be to change PotionRecipe.GetHashCode() (to be compatible with commutativity), but is it safe to do?
The problem with what I implemented is the difference in stacks and single items, with the former affecting player data as potionmaker is used and the latter not until mixing is done. Thinking about this now, with hindsight, I think your approach above would be a good improvement. So with ingredients list being clones so that stacks don't end up decrementing the player item. Then only adjusting player items when mixing a potion seems a much better approach to me. I should have realised that at the time. As you say it would also combine wagon stacks rather than having duplicate entries.

Your PR doesn't seem to implement what you suggested above, but I admit I've not had time to more than skim it so maybe I'm missing something. It seems to be tracking stacks and I'd prefer your idea above, as it would reduce complexity.

As for removing requirement for recipe order, that's a completely separate matter. I cannot recall if I checked classic for this, but it does seem like you could go either way - that recipes are stating order of ingredients, or that the ordering is irrelavant. I've no strong feelings, so don't really see the need for unordered recipes but won't oppose it either.
See my mod code for examples of how to change various aspects of DFU: https://github.com/ajrb/dfunity-mods

User avatar
pango
Posts: 2917
Joined: Wed Jul 18, 2018 6:14 pm
Location: France
Contact:

Re: [0.11.0] Potion Maker bugs

Post by pango »

Hazelnut wrote: Sun Jan 31, 2021 3:04 pm Your PR doesn't seem to implement what you suggested above, but I admit I've not had time to more than skim it so maybe I'm missing something. It seems to be tracking stacks and I'd prefer your idea above, as it would reduce complexity.
Correct, the PR uses a different approach, I keep track of what stack each ingredient in the cauldron came from, so that I can make exchanges between ingredients and cauldron totally reversible.

That requires less modifications than what I described before, but indeed tends to increase slightly complexity (not by much since it also simplifies RemoveFromCauldron a bit) whereas what I described before would probably make the code significantly easier to understand.
When a measure becomes a target, it ceases to be a good measure.
-- Charles Goodhart

User avatar
pango
Posts: 2917
Joined: Wed Jul 18, 2018 6:14 pm
Location: France
Contact:

Re: [0.11.0] Potion Maker bugs

Post by pango »

{removed duplicate post text - H}
Hazelnut wrote: Sun Jan 31, 2021 3:04 pm As for removing requirement for recipe order, that's a completely separate matter. I cannot recall if I checked classic for this, but it does seem like you could go either way - that recipes are stating order of ingredients, or that the ordering is irrelavant. I've no strong feelings, so don't really see the need for unordered recipes but won't oppose it either.
Well, ordered ingredients makes recipes almost a requirement. My feeling is that brewing potions is already so "punishing" (having to hunt down all ingredients, ingredients lost if mixing fails, only one potion brewed,...) that there's no need to make it harder...
When a measure becomes a target, it ceases to be a good measure.
-- Charles Goodhart

User avatar
Hazelnut
Posts: 2698
Joined: Sat Aug 26, 2017 2:46 pm
Contact:

Re: [0.11.0] Potion Maker bugs

Post by Hazelnut »

I commented on the PR, I would prefer the solution described here.

As for order of ingredients, guess this is an Interkarma call.. or maybe a poll. :)
See my mod code for examples of how to change various aspects of DFU: https://github.com/ajrb/dfunity-mods

Locked