Log error message

[Spannerbag]

Hi,

Should probably be two posts but I've spent way too long on these issues already.

1 : First the easy and original issue:

Getting a message in logfile, repeated 5 times with exact same timestamp.
Appears in log after story so in prestart/start I guess.
Just wondering if it's significant?

error engine: failed to auto-store $this_unit for player: Greenstock at recall index: 57

The unit in question appears where it should and has the correct experience, unit type etc.

Can I ignore this?


2 : Second issue came about as result of going back a scenario while trying to fix above issue.
Not fully isolated what's going on but I had two recall,recruit events with similar logic, one would fire on one leader's first recruit/recall and the second when either of the other two leaders did the same (all for the same player side).
I've completely rewritten how and when these events trigger for various reasons including gameplay flow (wanted them to fire after objectives were displayed but in turn 1 so ended up ditching recall,recruit and went for enter_hex event instead, which works acceptably well).

When running the original code undo didn't work. When I disabled allow_undo (IIRC, I did a lot of testing and rewriting and am not clear now what changed when) undo worked (for later actions IIRC) but doing several actions, undoing them then redoing them crashed Wesnoth.

After this screen abort and retry crashed straight away, ignore worked for a short time but the game wasn't happy and again it crashed shortly afterwards (a second or so, no further gameplay).
I'm guessing that the undo stack was exhausted but this wasn't handled gracefully?

Here's the original event(s) macro:

Code: Select all

# Credit saved gold
#
# Ask once first time any leader recalls/recruits something
# (Dwarf and elf/wose savings handled separately)
#
#define SPEND_SAVED
#arg SIDE
1#endarg
[event]
  name=recruit,recall
  [filter_condition]
    [have_unit]
      x,y=$x2,$y2
      id=Alacritous,Greenstock
    [/have_unit]
    {VARIABLE_CONDITIONAL saved_gold_ew greater_than 0}
  [/filter_condition]
  [message]
    speaker=second_unit
    message=_"Hmm... should we spend our $saved_gold_ew saved gold now?"
    [option]
      label=_"Yes"
      [command]
        {PLAY_GOLD_SOUND}
        [floating_text]		# Colour is gold :-)
          x,y=$x2,$y2
          text=_"<span color='#FFF35A'+$saved_gold_ew gold</span>"
        [/floating_text]
        {FTDELAY}
        [gold_min]
          side={SIDE}
          amount=$saved_gold_ew
        [/gold_min]
        {CLEAR_VARIABLE saved_gold_ew}
      [/command]
    [/option]
    [option]
      label=_"No, we can win without it."
      [command]
      [/command]
    [/option]
  [/message]
  [allow_undo]
  [/allow_undo]
[/event]
[event]
  name=recruit,recall
  [filter_condition]
    [have_unit]
      x,y=$x2,$y2
      id=Hew
    [/have_unit]
    {VARIABLE_CONDITIONAL saved_gold_d greater_than 0}
  [/filter_condition]
  [message]
    speaker=second_unit
    message=_"Should ah spend oor $saved_gold_d saved gold?"
    [option]
      label=_"Aye"
      [command]
        {PLAY_GOLD_SOUND}
        [floating_text]		# Colour is gold :-)
          x,y=$x2,$y2
          text=_ "<span color='#FFF35A'>+$saved_gold_d gold</span>"
        [/floating_text]
        {FTDELAY}
        [gold_min]
          side={SIDE}
          amount=$saved_gold_d
        [/gold_min]
        {CLEAR_VARIABLE saved_gold_d}
      [/command]
    [/option]
    [option]
      label=_"Nae, we can win wi'oot it."
      [command]
      [/command]
    [/option]
  [/message]
  [allow_undo]
  [/allow_undo]
[/event]#enddef

I wrote this event a looong time ago and the new version is significantly more sane elegant.

I'm guessing it's something in my code - hopefully in the bit posted.
If you want more details feel free to ask.
If possible I'd like to nail it down so other people can avoid my mistake.

Cheers!
-- Spannerbag

[Ravana]

1 means that $this_unit is empty in the statement that caused that error. Whether impact is visible depends on what you try to do with that $this_unit variable.

[gfgtdf]

When running the original code undo didn't work. When I disabled allow_undo (IIRC, I did a lot of testing and rewriting and am not clear now what changed when) undo worked (for later actions IIRC) but doing several actions, undoing them then redoing them crashed Wesnoth.
pre-crash.png

After this screen abort and retry crashed straight away, ignore worked for a short time but the game wasn't happy and again it crashed shortly afterwards (a second or so, no further gameplay).
I'm guessing that the undo stack was exhausted but this wasn't handled gracefully?

Here's the original event(s) macro:

Code: Select all

...

I wrote this event a looong time ago and the new version is significantly more sane elegant.

I'm guessing it's something in my code - hopefully in the bit posted.
If you want more details feel free to ask.
If possible I'd like to nail it down so other people can avoid my mistake.

Cheers!
-- Spannerbag

I think the problem in your code is that when you undo it the engine saves the choice (from the [message] with [option]s) you made and is then confused when redoing it becasue there is suddenly no choice to do anymore when reoding the same action (because the event is removed since it doesnt have first_time_only=no).


That said, even without this issue, your code clearly seems to change the gamestate as it changes the players gold,so it shoudnt be undoable in the first place.

[Spannerbag]

1 means that $this_unit is empty in the statement that caused that error. Whether impact is visible depends on what you try to do with that $this_unit variable.

Thanks - really have no idea where that's coming from but appears in the log (as far as I can tell) between the end of [story] and the first dialogue which occurs in start event after setting [objectives]. It's probably some macro somewhere.

Edit: found the issue, I'd accidentally pasted something in the wrong place.
Didn't break anything but removing the unwanted code also removed the error.

Cheers!
-- Spannerbag

[Spannerbag]

...That said, even without this issue, your code clearly seems to change the gamestate as it changes the players gold,so it shoudnt be undoable in the first place.

Ah. Going back in the mists of time, when I wrote the original version of the code I assumed for some reason that undo only applied to things like moves and recalls. Hence my belief was that [allow_undo] would allow the player to take saved gold (or not) but still undo an erroneous recall.
Thanks for the explanation, very helpful and much appreciated.
So... are all player actions saved in the undo stack until cleared or what? It sounds like the entire event is kept for undo? Just curious.

Thanks again for the reply.

Cheers!
-- Spannerbag

[gfgtdf]

When undoing an action, the game stores the players action in that stack and also all "choic-type" actions (like which [option] the player chose) that were recorded while excuting the action. Then the redo code is basicially the same as the replay code, so it takes these actions and "replays" them which implies exciting all events (which would usually use the choice-type actions saved in the replay of the original action), however in your case the event that was there when the action was originally excuted is no longer there so the replay kinda fails resulting in this error message.

I hope that was somewhat understandable

[Spannerbag]

... however in your case the event that was there when the action was originally excuted is no longer there so the replay kinda fails resulting in this error message.

I hope that was somewhat understandable

Ah (again), that's the bit I missed/didn't appreciate - if I understand correctly.
So, the undo action re-executes events rather than simply as well as remembering outcomes?
In essence am I right in thinking that an undo action consists of re-executing a piece of WML but feeding in the same player input recorded from the original gameplay?
If so, I can see that volatile events (first_time_only=yes) that also change gamestate could therefore cause a problem (missing WML template so gamestate not correctly reconstituted).
Teensy bit surprised tbh that the undo stack can't preserve original outcomes without original logic template and/or can't retain a copy of the original event... but then I haven't dug into replaywml much and am way out of my depth!

Have I grasped the basic concept (even if the details are hazy/wrong)?
Many thanks for taking the trouble to explain, appreciated.
Cheers!
-- Spannerbag

[Celtic_Minstrel]

It doesn't re-execute events. It drives the action "as if" it were playing a replay. Events are just executed as part of the action, if they're defined. The problem in your case is that the event was not defined, so it didn't execute, and because it didn't execute, the choice you made the first time went unused.

gfgtdf said you should not allow undo for events that change the gamestate, which is generally true, but it can probably be done safely if you're careful. If you're going to allow undo, your undo handler needs to reset the gamestate to exactly how it was before the action was performed. But event handlers themselves are also part of the gamestate. So in the case of an event with first_time_only=yes (the default), that means that the undo handler for the event needs to reschedule the event handler, which in WML at least requires an impossible recursive definition. (It's probably doable in Lua somehow. I'm not certain of this though.)

So what that means in general is that only events with first_time_only=no (and which never remove themselves) should be undoable.

[Spannerbag]

It doesn't re-execute events. It drives the action "as if" it were playing a replay. ... The problem in your case is that the event was not defined, so it didn't execute, and because it didn't execute, the choice you made the first time went unused.

...

So what that means in general is that only events with first_time_only=no (and which never remove themselves) should be undoable.

Thanks, I understood most of that.

Just to clarify:
By saying the event was not defined do you mean it had fired and so no longer existed so was thus unavailable (in some way I don't really understand) to any undo action the player might initiate?

Errr... so, for example, if a campaign has a first_time_only=yes) event that changes gamestate (e.g spawns a unit the first time the player does a specific action; moveto, kill, attack whatever) that will work in replays but is not "undoable" (in the sense that the undo action will not remove the spawned unit)?
Or does this only apply to certain gamestate changes like choices as mentioned by gfgtdf?
Have I got that right-ish?
Sheesh, who'd've thought undo'd be so complicated! (or, more likely I'm just thick ).
Thanks for taking the time to explain this, much appreciated.
Cheers,
-- Spannerbag

[gfgtdf]

So what that means in general is that only events with first_time_only=no (and which never remove themselves) should be undoable.

This is only partly true, very simple events that only show messages can be undoable (and mainline does that, thinkof messages like "this sign reads 'swamp ahead' when stepping on a sign)

Errr... so, for example, if a campaign has a first_time_only=yes) event that changes gamestate (e.g spawns a unit the first time the player does a specific action; moveto, kill, attack whatever) that will work in replays but is not "undoable" (in the sense that the undo action will not remove the spawned unit)?
Or does this only apply to certain gamestate changes like choices as mentioned by gfgtdf?
Have I got that right-ish?

No, any event that changes the gamestate (whether it has first_time_only=no set or not) may not be maked as undoable (with [allow_undo]) because whatever your event changes will not be undone by the engine when the playerclicks undo, and this results in OOS-errors in replays and often also ingame errors when trying to redo the action. (of course these issues only happen when the player actualy undoes the actions).

You can make things work by manually adding undo code for your event with the [on_undo] tag but that also has it's own pitfalls.

And its important that "Changing the gamestate" does not only mean things that have a direct effect on the board (like moving units) but also things that indirectly effect these things later, like setting variables that are later used to change the gamestate or removing events that change the gameste
(and this in particular imples events that implicitly remove themselves via lack of first_time_only=no).

In addition to that, special rules apply to using a [message][option]s as those have to be the same when the action is replayed/redone. (on a technical level doing a [message][option] can also be just seen as a change of gamestate, but a change that the engine can undo that one on its own. Still removing an event that does so is also a change of gamestate as explained above)


The rule of thumb is to only use [allow_undo] for two cases:

1) events that only do visual things and don't have an effect on the gamestate

for example this code is okay because it doesn't really do anyting besides showing a simple message:

Code: Select all

    [event]
        name=moveto

        [filter]
            x=14
            y=11
            side=1
            id=Inky
        [/filter]

        [message]
            speaker=Kai Krellis
            message= _ "Inky has found something. Someone should check it out."
        [/message]

        [allow_undo][/allow_undo]
    [/event]

2) Branches of code that don't do anything , for example in this code [allow_undo] is only used in case that no action was performed, note that is has first_time_only=no becase oterweise even the "no" branch woud also do something (removing the event)

Code: Select all

            [event]
                name=moveto
                first_time_only=no
                [filter]
                    side=1
                    x,y=1,1
                [/filter]

                [if]
                    [not]
                        {CHECK_VARIABLE book_taken yes}
                    [/not]
                    [then]
                        [message]
                            speaker=$unit.id|
                            caption=""
                            image="wesnoth-icon.png"
                            male_message= _ "Should $unit.name| read the book?"
                            female_message= _ "female^Should $unit.name| read the book?"
                            [option]
                                label= _ "Yes"
                                [command]
                                    [object]
                                        id=wose_lore
                                        name= _ "Wose Lore"
                                        image="items/book4.png"
                                        duration=forever
                                        description= _ "This massive book is filled with the Lore and Legends of the Wose. Any who read it learn deeply of the ways of the twig and leaf, the tree and the forest."
                                        cannot_use_message= _ "The book is filled with meaningless tales of sticks and leaves, and childish stories about trees and forest creatures."
                                        [filter]
                                            x,y=1,1
                                            [not] # Elvish Rangers already know this
                                                type_adv_tree=Elvish Ranger
                                            [/not]
                                        [/filter]
                                        [then]
                                            {VARIABLE book_taken yes}
                                            [remove_item]
                                                x,y=1,1
                                            [/remove_item]
                                            [message]
                                                speaker=$unit.id|
                                                caption=""
                                                image="wesnoth-icon.png"
                                                male_message= _ "$unit.name| becomes entranced and greatly gains affinity with the forest, its nature and ways, and all the creatures within it. He feels he will quickly become more experienced once reaching the surface. Unfortunately, as the last page was turned, the book crumbled to dust."
                                                female_message= _ "female^$unit.name| becomes entranced and greatly gains affinity with the forest, its nature and ways, and all the creatures within it. She feels she will quickly become more experienced once reaching the surface. Unfortunately, as the last page was turned, the book crumbled to dust."
                                            [/message]
                                        [/then]
                                        [effect]
                                            apply_to=new_ability
                                            [abilities]
                                                {ABILITY_AMBUSH}
                                            [/abilities]
                                        [/effect]
                                        [effect]
                                            apply_to=defense
                                            replace=yes
                                            [defense]
                                                forest=30
                                            [/defense]
                                        [/effect]
                                        [effect]
                                            apply_to=movement_costs
                                            replace=yes
                                            [movement_costs]
                                                forest=1
                                            [/movement_costs]
                                        [/effect]
                                        [effect]
                                            apply_to=max_experience
                                            increase=-20%
                                        [/effect]
                                    [/object]
                                [/command]
                            [/option]
                            [option]
                                label= _ "No"
                                [command]
                                    [allow_undo][/allow_undo]
                                [/command]
                            [/option]
                        [/message]
                    [/then]
                    [else]
                        [allow_undo][/allow_undo]
                    [/else]
                [/if]
            [/event]

For everything other than that you probably need [on_undo] to manually undo the changes that you did. (and as i said before it has it own pitifalls, that means i have multiple times seen people doing it wrong)

[Celtic_Minstrel]

By saying the event was not defined do you mean it had fired and so no longer existed so was thus unavailable (in some way I don't really understand) to any undo action the player might initiate?

I think this is correct. The event handler was not defined because it had been removed due to firing once.

Errr... so, for example, if a campaign has a first_time_only=yes) event that changes gamestate

Rather, you should think of first_time_only=yes as being a gamestate change in and of itself. Though as gfgtdf notes you can get away with it if that's the only change to the gamestate that your event does; but in any more complicated case, first_time_only=yes should be considered as a gamestate change. That means that if you use [on_undo] in such an event, the undo handler would need to reschedule the event. This isn't possible in WML because the code would need to be self-referential, but it might be possible in Lua… I'm not entirely sure though. (In nearly every case it would be better to instead use [first_time_only=no along with maybe [remove_event] at an appropriate place if you need the event to be undoable.)

(e.g spawns a unit the first time the player does a specific action; moveto, kill, attack whatever) that will work in replays but is not "undoable" (in the sense that the undo action will not remove the spawned unit)?

Well, yes, if you make a visible kind of change and force your event to be "undoable", then the undo won't really work. That's what [on_undo] is for – it's a way to tell the engine what changes you made to the gamestate, or rather, tell the engine what it needs to do to undo the changes.

However, in the case of spawning a unit, there's one major pitfall related to that: spawning a unit with random traits or a name cannot be undone no matter what. The reason for that is that generating a random number is also a gamestate change, and it's one that there's no way to undo. So if you wanted to be able to undo spawning a unit, you would need to use generate_name=no (or an explicit name) and random_gender=no (or an explicit gender) and random_traits=no (or explicit traits) in addition to using an [on_undo] handler that deletes the unit.

That's not unique to spawning a unit, of course – any process that involves random numbers will have the same issue. I believe it's possible to use an [unsynced] block to get around that and generate a random number without changing the gamestate, but I'm pretty sure that has its own pitfalls (I've never tried it myself).

[Spannerbag]

Whoah, thanks to gfgtdf and Celtic_Minstrel for all your input, really helpful.

Had a play with this event:

Code: Select all

  [event]
    name=moveto
    first_time_only=no
    [filter]
      id=Goody
      x,y=1,3
    [/filter]
    {MOVE_UNIT id=Goody 1 4}
    [allow_undo]
    [/allow_undo]
  [/event]

Regardless of the value of first_time_only and presence or absence of [allow_undo] this event was resolutely undoable, probably because the [move_unit] command inside {MOVE_UNIT ... clears the undo stack?

Sanity checks: this was undoable and redo issued the [message] every time (as expected)...

Code: Select all

  [event]
    name=moveto
    first_time_only=no
    [filter]
      id=Goody
      x,y=1,3
    [/filter]
#    {MOVE_UNIT id=Goody 1 4}
    [message]
      speaker=Goody
      message=_"Hi!"
    [/message]
    [allow_undo]
    [/allow_undo]
  [/event]

...but this wasn't undoable (also as expected)...

Code: Select all

  [event]
    name=moveto
    first_time_only=no
    [filter]
      id=Goody
      x,y=1,3
    [/filter]
#    {MOVE_UNIT id=Goody 1 4}
    [message]
      speaker=Goody
      message=_"Hi!"
    [/message]
#    [allow_undo]
#    [/allow_undo]
  [/event]

... finally this also behaved as expected (moves undoable/redoable but [message] only issued first time):

Code: Select all

  [event]
    name=moveto
#    first_time_only=no
    [filter]
      id=Goody
      x,y=1,3
    [/filter]
#    {MOVE_UNIT id=Goody 1 4}
    [message]
      speaker=Goody
      message=_"Hi!"
    [/message]
    [allow_undo]
    [/allow_undo]
  [/event]

I think it's finally been pounded into my head.

A few thoughts/comments on the wiki allow_undo text:

The types of actions that can be undone are movement, recalling, and dismissing a unit from the recall list. If an action is undone, only the position (or existence) of the involved unit will be restored; any altered variables or changes to the game will remain changed after the action is undone. It is up to the scenario designer to avoid abusing this command.

I think this should be given more prominence in the text and also (maybe) reworded to reinforce the scope of what is undoable plus (maybe), tie in a reference to [on_undo] into the usage advice? A very rough example:

Only changes to the position or existence of a unit can be undone (specifically movement and recalling/dismissing a unit from the recall list). Any other changes (altering/clearing variables, removing events etc.) persist after the action is undone.
Although on_undo enables undoing of additional actions, it generally introduces additional complexities and scenario designers should avoid using [allow_undo] where it is not essential.

My wording isn't great but hopefully gets the idea across.
There's nothing wrong with the current wording as a reference (which is the primary purpose to be fair) but readers using this as learning tool rather than as a reference might appreciate a little more advice on how the command works and what it can (and can't) do?

I'm also wondering if there's any mileage in nuking the redo command?
I use undo from time to time but very rarely use redo and could, personally, live without it if that would make developers' lives any easier?

Thanks again for your time and patience, much appreciated!
Cheers!
-- Spannerbag

[gfgtdf]

Regardless of the value of first_time_only and presence or absence of [allow_undo] this event was resolutely undoable, probably because the [move_unit] command inside {MOVE_UNIT ... clears the undo stack?

I dont think move_unit cleaers the undo stack, its imo much more likeley that since the movememt went to position (1,3) the engine tries to place the unit from (1,3) back to its original position but fails to do so because it cannot find the unit on (1,3)

[Spannerbag]

Regardless of the value of first_time_only and presence or absence of [allow_undo] this event was resolutely undoable, probably because the [move_unit] command inside {MOVE_UNIT ... clears the undo stack?

I dont think move_unit cleaers the undo stack, its imo much more likeley that since the movememt went to position (1,3) the engine tries to place the unit from (1,3) back to its original position but fails to do so because it cannot find the unit on (1,3)

Ah, that makes sense.
Am I right to think that because [move_unit] is done in code (and is thus not a direct action by the player) that it is not added to the undo stack?
Sorry to keep asking questions.

Cheers!
-- Spannerbag

[gfgtdf]

yes that correct, only the "orignal" action is put on the unit stack

(actually i see little reason why it shoul dbe that way, i think we could drather easily make for examle [move_unit] call [on_undo] internally to add itself to the undo stack)