Add ClearFactoryQueue logic, and hook it to INSERT/STOP/REMOVE combos

[saurtron]

Work done

• Add code to efficiently clear factory queue (unit's commandQueue)
• Hook into INSERT(control, alt)+STOP
• Implement REMOVE(meta)
  • New behaviour accepting params(startIdx, endIdx), if either is unset will default to queue ends.
  • Also accepts ctrl to choose queue.

Work yet to be done in this PR

• Remove STOP hook if we don't want (just need to remove all changes from CFactoryCAI.*)
• Refine the clear factory behaviour

Related issues

• CMD_STOP for factory buildqueue #1082
• Commands: stop production queue should clear all items except current, double should clear current Beyond-All-Reason#2190

Also see following "precursor" pr: #2137

Remarks

• REMOVE(meta) working great for this use case, use like this:
  • spGiveOrderToUnit(unitID, CMD.REMOVE, nil, {'meta', 'ctrl'})
  • spGiveOrderToUnit(unitID, CMD.REMOVE, 1, {'meta', 'ctrl'})
  • spGiveOrderToUnit(unitID, CMD.REMOVE, {1, 20}, {'meta', 'ctrl'})
• Seems like INSERT(control, alt)+STOP fits quite naturally for this use case, it also supports clearing from a specific element onwards.
  • Probably best not do it though, since breaks the (unwritten and not totally true) "STOP == noop" rule.
  • Use like this: spGiveOrderToUnit(unitID, CMD.INSERT, {index, CMD.STOP, 0}, {'ctrl', 'alt'})
    • index is the element to start removing from.
• This can be changed, to use REMOVE instead, if we decide on a way to control that command through modifiers.
  • can also be added and support both STOP and REMOVE through the same core code, just changing it slightly.
  • note most of the remarks here probably apply to REMOVE as well
• It's a bit unclear how callins should react
  • I'm making the STOP command FinishCommand(), that makes it generate UnitCmdDone callin for the command, in this way GUI can react.
  • Don't think STOP currently always sends the UnitCmdDone.
  • Not sure if cancelling the current command should UnitCmdDone, again, don't think it's always the case now (at least dequeue will cancel the first element but not generate a UnitCmdDone), and this code doesn't do it atm. In case its a unit being built don't think it should be called, but for other commands it's probable it should.
• Probably needs a bit of special handling of the WAIT command too, if we want this to work conditionally on the WAIT status of the unit (actually the first command in the queue).
• While we could just not support so many special cases, I think it has some value, so we have more efficient path for the "full queue clear" paths of leaving 0 or 1 commands behind. This way we avoid possibly clearing thousands of commands and then looping through them.

[sprunk]

STOP should stay a no-op, add a modifier to REMOVE to allow span removal.

[saurtron]

STOP should stay a no-op, add a modifier to REMOVE to allow span removal.

Yeah, don't worry easy to delete from the PR, just trying different possibilities until this is settled.

Already Implemented the REMOVE(meta) path for range removal.

Seems to work fine, and also allows working with control modifier to select either queue, so pretty much a great solution. (code can still be refactored a little bit probably).

So, with meta atm it accepts up to 2 params, being start, and end. Guess this could still accept more parameters, and then also do the

So using like this to clear:

spGiveOrderToUnit(unitID, CMD.REMOVE, nil, {'meta', 'ctrl'})
spGiveOrderToUnit(unitID, CMD.REMOVE, 1, {'meta', 'ctrl'})

Also tested with a queue having 100k commands and more, and didn't see any issues, works instantly, (thx to the STOP hax ofc), so likely not needing the alternative paths, those would help if not having the hax, but I think it's here to stay, so...

[saurtron]

Already Implemented the REMOVE(meta) path for range removal.

Only thing I'm worried here, or maybe I'm missing something, is how the gui is supposed to be notified?

You can see I added:

commandQue.push_front(c);
FinishCommand();

After my loop, this makes UnitCmdDone called for the CMD_REMOVE here. Problem is this probably has a bunch of other side effects if the unit is actually doing something and we're not removing the first command. Without META it's not doing it, so here I probably also should not do it.

So, the question is, without something like that, how is gui supposed to be notified? Maybe it is and I'm not looking for it properly, but see, the problem I have is otherwise gridmenu doesn't get notified, so everything works, but the numbers in the panel don't update. Not specific about gridmenu, but atm for tests I'm using UnitCmdDone to see if I should refresh.

[saurtron]

So, with meta atm it accepts up to 2 params, being start, and end. Guess this could still accept more parameters, and then also do the

Regarding this, parameters could also be start, nitems. Lemme know what you prefer.

[saurtron]

Added a new PR, #2156, with just the INSERT changes.

Will close this soon, once conversations are settled.

[sprunk]

start/end vs start/nitems

I don't mind either way, your call.

how the gui is supposed to be notified? (...) atm for tests I'm using UnitCmdDone to see if I should refresh.

I think this

spGiveOrderToUnit(unitID, CMD.REMOVE, {1, 3}, {'meta'})

would ideally produce the same events etc as:

local _, _, tag1 = Spring.GetUnitCurrentCommand(unitID, 1)
local _, _, tag2 = Spring.GetUnitCurrentCommand(unitID, 2)
local _, _, tag3 = Spring.GetUnitCurrentCommand(unitID, 3)
spGiveOrderToUnit(unitID, CMD.REMOVE, {tag1, tag2, tag3})

[saurtron]

would ideally produce the same events etc as:

Yeah, without the extra UnitCmdDone it should, since it does exactly the same. But is it actually sending anything? Asking because I'm not sure, in case you know, otherwise I'll find out.

[sprunk]

I don't know. I assume if you start removing from the 1st (but not from 2nd or later) then each command in turn will become active and produce some sort of "command done" event. I dislike this, but I also dislike iterating in different order than what exists so idk. Perhaps it's your call.

[saurtron]

I don't know. I assume if you start removing from the 1st (but not from 2nd or later) then each command in turn will become active and produce some sort of "command done" event. I dislike this, but I also dislike iterating in different order than what exists so idk. Perhaps it's your call.

When it's the first command there might be something, but in the general case, not really getting anything atm.

In BAR, gridmenu atm just listens for CommandNotify:CMD_STOP_PRODUCTION UnitCommand(-cmdID, dequeue flags) to update the number, or updates directly since it's being handled by the same menu.

Problem is currently dequeue cmdID has enough context to know how much to discount from each number, since you get a unitDefID and a count number to remove.

When using the mechanics from this pr, it becomes trickier, since CMD_STOP_PRODUCTION doesn't have context any more (we will get the UnitCommand(REMOVE)), to know how many units to remove. What is worse, there is no clear indication of when the command finished, so you can update the queues, since REMOVE isn't doing any UnitCmdDone. So the gui can be left with needing to introduce some kind of delay, so it can check once the command is done.

That's the situation I noticed that led me to introduce that (push REMOVE in queue + FinishCommand call), still, this could have unexpected side effects when clearing queue from non-factory unit, where pushing remove + FinishCommand will have too many side effects. In this case I believe it would be better to just run the callin directly (UnitCmdDone), so gui can react easily. Otherwise let me know if you can think of some other way to do it.

Complicating things, previous per tag/cmdid behaviour also not doing this, so not sure I should be introducing this callin behaviour in general for Remove, or what'd be the optimal approach.

[sprunk]

REMOVE isn't doing any UnitCmdDone
I believe it would be better to just run the callin directly (UnitCmdDone), so gui can react easily

Sounds good.

previous per tag/cmdid behaviour also not doing this, so not sure I should be introducing this callin behaviour in general for Remove

Sounds good.

[saurtron]

closing since this is superseded by #2156 and all conversations here settled.