Undo doesn't refund material

[Former user]

I have witnessed several times materials not refunded after clicking on 'undo' for a craft/purchase. Wondered if it was a bug or an intended behavior?

[Ethaniel Kavelaars]

I confirm that “undo”: 1/ does nothing for Bonfire building purchase (no refund, no removal of the building: If intended, then the button should not appear), 2/ does not exist for Religion and Space buildings (maybe it should if 1/ is a bug? ping @bloodrizer), 3/ works pretty fine for resource crafting, BUT it does not refund consumed resources that was initially above its storage cap (so it's not a real undo in that case).

BTW on a loosely related note, in the Religion tab, Order of the Sun upgrades can be sold, but neither Ziggurats nor Cryptotheology buildings, there is an inconsistency here.

[Ethaniel Kavelaars]

After checking the code and its history, it appears that the triggering of the “undo” button for Bonfire building purchase has been added more than 2 years ago (Sept. 2016) in a pull request that was totally unrelated with that subject, but the actual implementation of what to do when the “undo” button is pressed has never been coded, so it has never worked. So, for now, the more logical thing to do is to remove the useless “undo” button for Bonfire buildings (so nothing to do for 2/ for now). So only 3/ needs a fix.

BTW while checking the code it appears that everything is ready for undoing several crafts (craft A, immediately craft B, undo, B and A should be uncrafted), but actually each new craft overrides the previous one (craft A, immediately craft B, undo, only B is uncrafted) instead of being appended. As everything above has been coded in a single commit with a comment saying “I am too tired to write proper logic”, it really looks like the initial intention was undo chaining and that undo override is a bug.

[Ethaniel Kavelaars]

@bloodrizer In game.js:

/**
 * Undo Change state. Represents a change in one or multiple
 */
dojo.declare("classes.game.UndoChange", null, {

Multiple what? Multiple tabs? Was the initial intention to allow aggregation of undo actions from several tabs (one undo in bld, one in workshop, one in space, etc.), without allowing to chain undo actions of same type (several crafts)?

[Arima B.]

@Ethaniel_Kavelaars A single logical unit of operation should go into a single undochage if possible. Undo changes can and probably should be chained, but jury is still debating on how big margin of error is allowed for a player. Even 1 maximum undo with override is OK at the current state IMO.

[Arima B.]

Fix #150

Triggering of the “undo” button in case of building purchase has been added in c7e45e5d4caeede4604d8cc439e625713ee2a17b but no implementation of the undo method has ever been done, so the button should not be triggered as it does nothing

→ <<cset d02d0e8b0eae>>

[Arima B.]

Merged in Ethaniel_Kavelaars/kittens-game/UNDO_BUILDING (pull request #443)

Fix #150

→ <<cset 122f1bd25f54>>

[Ethaniel Kavelaars]

@bloodrizer Argh, I missed your last week comment, sorry!

Concerning how to manage undo:

O.K., I understand that allowing to undo only the very last action is already a big enough help for players, and I agree with that; Actually I use only F5, so I didn't even realize that the “undo” button didn't work for building purchase before reading the current issue report! But, in that case, there is a lot of useless/YAGNI code which blurs the actual intention, better simplify the code in order to make crystal clear for future readers what is allowed and what will not (I'll do the code cleaning tomorrow I think).

Concerning what to manage with undo:

Currently only workshop crafting can be undone, here are the other things that could be undone (if I don't miss some):

1. stackable purchase (buildings) => I think that no undo must be provided, as it's already possible to sell buildings¹
2. one-time purchase (technologies/upgrades) => I have no opinion at all, I see no reason to allow undo for that, but I see no reason to not allow undo, so maybe I'll implement that some day and the jury will merge or reject the PR
3. conversion (trade/sacrifice/refine/combust) => as it's very similar to workshop crafting (converting some resources into other ones), I think that those conversions should benefit too from undo for consistency, except maybe TC combustion²; I'll see how to code that in the next few days

---

1. Not all the buildings, which is an issue that must be addressed separately ↩

2. Because the code for it will be complex and thus error-prone and thus will require a lot of time: I think that there are more urgent things to fix or implement before undo for TC combustion ↩

[Arima B.]

Undo for buildings is fine, it's time limited anyway. I would probably allow undo anywhere where it makes sense from QOL point of view.

[Ethaniel Kavelaars]

Thanks, noted!