Don't cancel recorded macro when running the macro stopped

[Mikolaj]

See the comment "Needed to properly cancel macros that contain apostrophes". This seems to be buggy and triggered not only for nested macros.

Also, the whole macro subsystem needs a review (what is the meaning of k=0? why is k not set to 0 after macro recording is finished), simplification (perhaps increment k after each keypress, not each action? disallow recording macros when inside macros?) and code comments.

[Mikolaj]

Possibly related: when pressing X many times and being interrupted I sometimes (e.g., when an actor is spotted) get "Macro recording aborted".

[rszczers]

Hey,

could you describe briefly how did you imagine the macro system to work? There's a subtle interplay inside humanCommand that I find hard to tell whether it's a bug or there's some clever way to make use of it.

From my experience, as for now, the only use case of macros is to start recording one, keep fingers crossed and bash v to replay what's been recorded already. There's no point in stop recording the macro. From what I get it is so, since we clear currentKeys in slastRecord whenever player moves.
(Also, this is the only part where k gets decremented. For sure k==0 is an implicit state meaning “we are not recording, mind your own business”.)

LambdaHack/engine-src/Game/LambdaHack/Client/UI.hs

Lines 151 to 155 in 6f5cc3b

	LastRecord seqCurrent seqPrevious k <- getsSession slastRecord
	let slastRecord
	| k == 0 = LastRecord [] seqCurrent 0
	| otherwise = LastRecord [] (seqCurrent ++ seqPrevious) (k - 1)
	modifySession $ \sess -> sess {slastRecord}

and then we record last pressed key in currentKeys:

LambdaHack/engine-src/Game/LambdaHack/Client/UI/FrameM.hs

Lines 140 to 142 in 96f10b2

	LastRecord seqCurrent seqPrevious k <- getsSession slastRecord
	let slastRecord = LastRecord (km : seqCurrent) seqPrevious k
	modifySession $ \sess -> sess { slastRecord

Then if we want to replay a macro, e.g. pressing v, repeatHuman 1 fires up and concatenates contents of previousKeys to slastPlay where it gets consumed later on.

LambdaHack/engine-src/Game/LambdaHack/Client/UI/HandleHumanLocalM.hs

Lines 680 to 686 in 17f751b

	repeatHuman :: MonadClientUI m => Int -> m ()
	repeatHuman n = do
	LastRecord _ seqPrevious k <- getsSession slastRecord
	let macro = concat $ replicate n $ reverse seqPrevious
	modifySession $ \sess -> sess {slastPlay = macro ++ slastPlay sess}
	let slastRecord = LastRecord [] [] (if k == 0 then 0 else maxK)
	modifySession $ \sess -> sess {slastRecord}

I've been tinkering with the code here and there, but before I go into details I would like to make sure what were your design choices. Do we want to clear macro buffer whenever we replay the macro? Does constraining macros to maxK=100 actions is necessary?

[Mikolaj]

I remember the system was broken a few times. Perhaps it is broken currently. I will check soon. In any case, it's probably too complex per it's usefulness, if it gets broken so often and the breakage is spotted so late (or it works fine, but it's so hard to use/document, it seems broken), so if at some point you come up with alternative designs, feel free to experiment.

[Mikolaj]

Before I even look at this code: the in-game macro system is an afterthought, "because I can". The real purpose of the system is defining complex commands from simple commands, as seen in the game engine in

https://github.com/LambdaHack/LambdaHack/blob/6f5cc3b7b195d840919e034c8c140bc60b7f1c80/engine-src/Game/LambdaHack/Client/UI/Content/Input.hs

in the game content in

https://github.com/LambdaHack/LambdaHack/blob/6f5cc3b7b195d840919e034c8c140bc60b7f1c80/GameDefinition/game-src/Client/UI/Content/Input.hs

and in user config file in

https://github.com/LambdaHack/LambdaHack/blob/6f5cc3b7b195d840919e034c8c140bc60b7f1c80/GameDefinition/config.ui.default

(though actually there are no macros in the config ATM and the command is expressed algebraically instead; but there could be and the user is rather more likely to be able to write macros that to be able to write algebraic terms with commands in them).

And the main use-case for the in-game macro recording is the functionality from Angband, namely "repeat previous command", easily generalizable to "repeat previous command 100 times" (not sure if that's in Angband).

I hope this sets the boundary conditions or at least the context for the design. In particular, I wouldn't mind forcing the user to write only algebraic terms in config and restricting the in-game macro to the basic cases, if the result is that we never ever need to mix abstract syntax (the algebraic terms, HumanCmd) with concrete syntax (the keypresses) in the code and in the game content. That's the underlying superfluous complexity and it may have something to do with the flakiness of surrounding code. But perhaps you can just fix and document it for good and the complexity will become manageable (I hope this part of the game won't be ever much extended, as opposed to almost all the other parts).

[Mikolaj]

So, the in-game macro system is quite broken indeed. It was supposed to let you record a macro, between a pair of ' keypresses and then, at any time, play it. I think it only works now if you record a number of waiting actions (probably a few other actions might work too, but not movement) and then repeat them. There is probably a macro reset called somewhere in the middle of the movement commands, possibly because they use macros for something (e.g., isn't running with Shift-dir implemented by a macro "run 1 step N times", where running 1 step is almost like moving 1 step, but may change direction if blocked? I don't quite remember). Generally, I think the in-game macros get broken so often, because the in-content and in-engine uses of macros require changes and then they affect the player-accessible macro interface.

I think you are right about the meaning of k==0. I'm not sure, but possibly the ability to end macro with the apostrophe was for the config files, etc., and also for, e.g., a possibly grindy harvesting action that the player wants to repeat for many tiles (cooking food using tiles on fire that get quenched upon use is a recent example, though it's only a couple of keystrokes; I have to try it with a macro after it's fixed).

Do we want to clear macro buffer whenever we replay the macro?

When a macro is replayed, it should be ready to be replayed again. However, it should not be ready to be extended by further keystrokes. Either ' or replaying should stop recording the macro, clearing whatever is responsible for accumulating keystrokes continuously. Otherwise, say, 'v' would be added to the macro and the next 'v' press would repeat the macro 2 times (or more, I'm lost in the recursion).

Does constraining macros to maxK=100 actions is necessary?

Not strictly, but I've had so many cases of the macros run amok, that it was probably just fire safety. :)

[rszczers]

There is probably a macro reset called somewhere in the middle of the movement commands, possibly because they use macros for something

When we're not recording, slastRecord holds two last player's moves as two separate single element lists in currentKeys and previousKeys accordingly (e.g. LastRecord [UP] [DOWN] 0). I guess the problem boils down to that we're holding in-game macro in the same buffer where we hold player's last action. That's how it gets overwritten right after player moves. Maybe we should overwrite it only when macro consists of a single action? That's hypothesis only, let me sprinkle here and there some conditions to check if it holds up.

Also, aren't predefined macros interpreted in macroHuman? This problem seems orthogonal to replaying in-game macros.

LambdaHack/engine-src/Game/LambdaHack/Client/UI/HandleHumanM.hs

Lines 59 to 62 in 17f751b

	cmdAction :: (MonadClient m, MonadClientUI m)
	=> HumanCmd -> m (Either MError ReqUI)
	cmdAction cmd = case cmd of
	Macro kms -> addNoError $ macroHuman kms

Sorry if I mix things up, it's quite dense :-D

[Mikolaj]

What you write reminds me that in the absence of explicitly recorded macros, the last issued command is implicitly regarded as the recorded macro, so that v repeats the last command, by default. That's hacky, but probably convenient and probably explains the one-element lists (only one would be needed if that was all going on).

Yes, we are surely holding the player UI commands and macros inserted from game content/config/engine in the same buffers. That was the idea --- to unify all this. However, quite possibly we've gone overboard and separate input buffers, at least, would be better.

Yes, predefined macros are handled by macroHuman, but it just adds them to a buffer that normally holds the in-game macros (after their recording is finished, I think) and yields control to the standard keyboard input interpretation loop.

[Mikolaj]

Some tests are failing. Possibly I don't understand how this should work. Changing the tests so that they show the intermediate steps of macro unwinding (as some other tests do) would surely clarify the matter. Feel free to do that.

[rszczers]

It's a bug. Some contents of predefined macros are being unnecessarily recorded. Let's take a closer look at two failing test cases.

Case 15: a := x (Output means accumulated output)
In step 4 x gets into the buffer right after a, so x is repeated twice. It's unnecessary, since we use stack buffers only as a placeholders for macros in macros, and that's not the case with x.

The easiest way out is to bypass keys binded to Macro command, i.e. record only the contents of predefined macros (I'm not entirely sure if we won't lose any functionality that way). Otherwise, we have to find a way to track down macro contents in slastPlay (counting down actions perhaps? marking them as introduced by a macro?).

#	smacroStack	slastPlay	Output
1	[Right []]	'a'v
2	[Left []]	a'v
3	[Left [a]]	x'v
4	[Left [x,a]]	'v	x
5	[Right [a,x]]	v
6	[Right [a,x]]	ax
7	[Right [a,x]]	xx
8	[Right [a,x]]	x	xx
9	[Right [a,x]]		xxx

I did by hand case 19. It's essentially the same thing.

Case 19: a := 'x'v

#	smacroStack	slastPlay	Output
1	[Right []]	'a'v
2	[Left []]	a'v
3	[Right [], Left [a]]	'x'v'v
4	[Left [], Left [a]]	x'v'v
5	[Left [x], Left [a]]	'v'v	x
6	[Right [x], Left [a]]	v'v
7	[Right [x], Left [v, a]	x'v	xx
8	[Right [x], Left [x, v, a]	'v
9	[Right [a, v, x]]	v
10	[Right [a, v, x]]	avx
11	[Right [], Right [a, v, x]]	'x'vvx
12	[Left [], Right [a, v, x]]	x'vvx
13	[Left [x], Right [a, v, x]]	'vvx	xxx
14	[Right [x], Right [a, v, x]]	vvx
15	[Right [x], Right [a, v, x]]	vx	xxxx
16	[Right [x], Right [a, v, x]]	x	xxxxx
17	[Right [x], Right [a, v, x]]		xxxxxx
18	[Right [a, v, x]]

[Mikolaj]

Thank you for the analysis. So it's not an error in my test mock, but real bug in my code? BTW, you can tweak the tests to print tables similar to what you provide (but without the formatting) by giving it a dummy expected answer and looking at the error report.

[rszczers]

I've reasoned how it supposed to work with pen & paper (well, more like keyboard & org-mode), then compared that roughly with the real code simply by running the game and printing out intermediate values. Your test assertions seem right, those two failing tests cases gave the same output as I expected doing the thing manually, so implementation is (rather) correct, but the system is flawed.

Passing by macro-keystrokes is a hopeless idea. Just tried that out and broke the whole thing. I'm thinking right now if there's still any point in recording actions by taking them one by one from slastPlay. Why not just handle Macro at once, like, parse them and push what's needed directly onto the stack?

[Mikolaj]

Sure, for a Macro it should work fine, but for keys pressed by the player it needs to be recorded one by one. Could these be treated differently? If yes, that may be the way forward...

[rszczers]

Oh, to clear things out, in previous post I mistook case 18 with case 19. I've edited that post. Case 18 passes fine.

[rszczers]

Hang on, I believe I found the ultimate solution. I think that merging smacroStack, slastAction and slastPlay into a one thing, like sactionPending :: [ActionBuffer] where

data ActionBuffer = ActionBuffer
  { slastPlay    :: KeyMacro
  , smacroBuffer :: Either [K.KM] KeyMacro
  , slastAction  :: Maybe K.KM
  }

gives us exactly what we need. Both slastPlay and slastAction are locally related to macro being repeated. This simplifies a lot the logic of recording a macro (notice that smacroBuffer is now just an Either value). Whenever we repeat a macro, we push onto the stack a new ActionBuffer and we simply pop it if slastPlay is empty.

I've refactored quite a lot already, but this will take a while longer.

[Mikolaj]

This is finally completely resolved, with sugar on top. Closing.