weatherfactory / cultistsimulator-visible

Read-only preview builds of Cultist Simulator for the convenience of modders and the curious.
https://weatherfactory.biz/cultist-simulator/
37 stars 5 forks source link

Mutation values are incorrect for stacks #154

Closed Chelnoque closed 2 years ago

Chelnoque commented 2 years ago

I don't remember, maybe I've already send you this and maybe you said that it should work like that, or that it's too delicate matter to fix at this point, but I've recently encountered this problem again - believe me or not, I've broke the lore research too! (even thought that these Steam threads were my fault at first!) So, the problem is as follows:

Say, you have an Element that has "lantern": 1. Say, you have a stack of 9 cards of that Element. The stack will return 1 * 9 = 9 of "lantern" for all purposes - aspect display, requirements and so on and so on. (I'm not actually sure there are other purposes, but that's beside the point).

Then, say, we set the "lantern" mutation for the whole stack to -1. Reasonably, it now should return (1 9) + (-1 9) = 0, right? Nope, it returns (1 * 9) + (-1) = 8, because the stack's quantity isn't accounted for mutations.

Furthermore, if we separate a single card from the stack, it's now (1 8) + (-1) = -7 for the remaining stack and (1 1) + -1 = 0 for new one, so the overall value has changed. And we can split and split and split the stack, up until the point when each token is has a quantity of 1, and at this point each card returns (1* 1) + (-1) and it's lantern net value is zero, and there's overall zero of lantern in the context.

The problem is in ElementStack.GetAspects(), in all cases where .ApplyMutations(this._mutations) happens. The best way to fix this, if one'd actually want to fix this, is probably to create an overload of AspectsDictionary.ApplyMutations() with an additional parameter "forQuantity", so it iterates through the mutated values and multiplies them by the target quantity, and then just calls a normal ApplyMutations() with the new values.

Upd: After a second glance, it appears there's already a method for multiplying quantities in aspect dictionary, who'd figure! In this case, MultiplyByQuantity() and ApplyMutations() should just switch their places inside ElementStack.GetAspects(), so you just get the overall aspect amount for a single card in the stack, including all the mutations, and then multiply the resulting value by the stack amount.

alexiskennedy commented 2 years ago

I think, when I was working through use cases, I decided: this will never be relevant, because cards with mutations don't stack*.

Yup, your update sounds like a good approach. I am happier assigning this to you than doing it myself because I expect you'll do a better job of testing edge cases after the change!