[2.0.72] Undo item has nil entries

[Natha]

Steps to reproduce:
1. Build a belt
2. Super-force build a splitter (belt is now marked for deconstruction and splitter ghost is created)
3. Place the belt again in another direction on the same position (splitter ghost is removed)
4. Print the last undo item. The first entry is nil

Code: Select all

/c game.print(serpent.line(game.player.undo_redo_stack.get_undo_item(1)))

[boskid]

Can you provide a save file created between steps 3 and 4? i am unable to reproduce any issues and i suspect i am misunderstanding which actions are super forced or normal or ghost or blueprint builds.

[Natha]

Can you provide a save file created between steps 3 and 4? i am unable to reproduce any issues and i suspect i am misunderstanding which actions are super forced or normal or ghost or blueprint builds.

Rotate the belt in the cursor so that is is not facing north. Place it where the deconstruciton mark is.

[Rseding91]

So, looking into this - a given undo item can have several actions in that item and one or more may be invalid. When pushing the structure to Lua the push logic will skip invalid entries however it still increments the index that it uses to put the undo item actions into the lua table.

I can "fix" this easily, but are the indexes of the undo item actions important in anyway that they should be kept as-is with nil entries?

It looks like the indexes are possibly used when using "remove_undo_action", so they are still useful to know even with nil entries - otherwise it wouldn't work there.

[Natha]

So, looking into this - a given undo item can have several actions in that item and one or more may be invalid. When pushing the structure to Lua the push logic will skip invalid entries however it still increments the index that it uses to put the undo item actions into the lua table.

I can "fix" this easily, but are the indexes of the undo item actions important in anyway that they should be kept as-is with nil entries?

Knowing this behaviour, I can easily do a nil-check when accessing entries. But this is quite counter-intuitive, when the data type is an array I expect it to not have nil entries.

But I found an inconsistence: despite player.undo_redo_stack.get_undo_item(1) has nil entries, the actions array in on_undo_applied doesn't. This might be a problem if you stored indexes before and access the corresponding entries during the event.

It looks like the indexes are possibly used when using "remove_undo_action", so they are still useful to know even with nil entries - otherwise it wouldn't work there.

I don't see a problem - why would you want to remove actions that don't exist anymore?

[Rseding91]

Those do seem to be conflicting... but the question is what's the more useful behavior? I can easily remove the nil entries from the push if that's what seems most useful.

[Natha]

Whatever is best for UPS. I'd say only a few mods use the undo/redo API at all.
For me, as a programmer, it is more useful (or convenient) if entries of an array of UndoRedoActions really are UndoRedoActions (having a type and some data). If you decide to leave it as-is, please add a hint to the docs. Iterating with pairs() might be better then.

If you consider touching the undo API, I'd really appreciate if you also think about some of the (imo good) modding interface requests:
search.php?keywords=undo&fid%5B0%5D=28