New REMOVE_RANGE command for range removal

[saurtron]

Work done

• New REMOVE_RANGE command, removing ranges by cmdId or Tag.

Work yet to be done in this PR

• Sort out callin behaviour, if any
  • kind of decided, but can still think about it.
• Maybe refactor to better share code with the cmdid/tag loop
  • not sure it's going to be pretty due to inverse looping and 'active' inside the cmdid/tag loop.

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" prs:

• Add ALTQUEUE command modifier. #2137
• Add ClearFactoryQueue logic, and hook it to INSERT/STOP/REMOVE combos #2154

Remarks

• It's a bit unclear how callins should react
  • how is gui supposed to be notified? I don't see the other loop doing any callins, but gui needs them to be notified of changes, specially for factories, otherwise the gui queue numbers don't get updated without some special gui code.
  • for now decided on actually calling UnitCmdDone for the remove command.

REMOVE_RANGE specs

• ALT: remove by index or tag switch
• CONTROL: alternative queue selection.
  • for factories alternative queue is the factory command queue, default queue is the newUnitCommands queue.
  • for other units no effect.

callins: After successful execution, it will call UnitCmdDone.

Examples:

• spGiveOrderToUnit(unitID, CMD.REMOVE, nil, {'meta', 'ctrl'}) -- delete everything
• spGiveOrderToUnit(unitID, CMD.REMOVE, 2, {'meta', 'ctrl', 'alt'}) -- delete all but the 1st element
• spGiveOrderToUnit(unitID, CMD.REMOVE, {1, 20}, {'meta', 'ctrl', 'alt'}) -- delete from 1st to 2th
• spGiveOrderToUnit(unitID, CMD.REMOVE, {startTag, endTag}, {'meta', 'ctrl'}) -- delete from position of startTag, to endTag

[sprunk]

Looking nice.

[sprunk]

Looks great.

[saurtron]

Ended up implementing tag mode too, it's a bit verbose and who knows if someone needs it, but actually when not providing params it doesn't matter if ALT is used since it defaults to whole queue anyways, otherwise yes, ALT is indexes, no ALT is tags.

If start/end tag are provided, but not found, then it won't do anything.

Let me know if you think it's too much can always remove, but felt its incomplete otherwise, besides, we were not considering the case where no ALT, but no params too. In that case it works just like with ALT.

Anyways I think maybe I'll try with a simple FindTagIndex function, and use that one instead, maybe not so efficient but likely doesn't matter that much here. Also later can be used in other places too. Tried to make the implementation of finding both ends efficient, since the queue can have 100k+ items, but looks a bit overboard tbh XD.

[saurtron]

Another consideration, indexes here are c++ based, so first is 0, last is queue.size()-1. Should we maybe use lua-like indexes instead?

[sprunk]

Should we maybe use lua-like indexes instead?

Good point, should use Lua 1-based.

Ended up implementing tag mode too

Great.

[sprunk]

Looks good, perhaps it's a good moment to undraft.

[sprunk]

I didn't find a good way to perform that flush after the fact, otherwise I'd add it here XD

I'm fairly sure that giving two CMD_WAIT commands works as a flush (they immediately cancel each other out and the unit seems to react a bit faster than when inserting a CMD_STOP). Perhaps check whether it helps in this case and if yes, what exactly happens inside that makes it work.

[badosu]

Ok, I just realized there's #2172 addressing my documentation concern, I removed my previous comments on this post.

(note: is this up to 2nd or up to 20th? unsure if theres a typo or not spGiveOrderToUnit(unitID, CMD.REMOVE, {1, 20}, {'meta', 'ctrl', 'alt'}) -- delete from 1st to 2th)

[saurtron]

Ok, I just realized there's #2172 addressing my documentation concern, I removed my previous comments on this post.

(note: is this up to 2nd or up to 20th? unsure if theres a typo or not spGiveOrderToUnit(unitID, CMD.REMOVE, {1, 20}, {'meta', 'ctrl', 'alt'}) -- delete from 1st to 2th)

Oh yeah, right that's why I wrote 2th originally, but then corrected to 2nd... good catch. Fixed now.

[saurtron]

As noted here, this might be making the REMOVE cmd api a bit too overloaded, and it could be better to create a separate command instead, like CMD_REMOVE_RANGE for example.

Wondering what other ppl here think.

[sprunk]

Splitting sounds good. I wouldn't do CMD_REMOVE_RANGE because there's multiple kinds of range (tags, indexes) and would instead stick to making them accept one type of value. So adding CMD_REMOVE_BY_INDEX, CMD_REMOVE_BY_TAG, and CMD_REMOVE_BY_CMDID sounds good. Remove-multiple (i.e. range) could be a modifier (either alt or meta, idk) for INDEX/TAG, and perhaps CMDID could also remove one by default and remove all with the remove-multiple modifier.

[saurtron]

Splitting sounds good. I wouldn't do CMD_REMOVE_RANGE because there's multiple kinds of range (tags, indexes) and would instead stick to making them accept one type of value. So adding CMD_REMOVE_BY_INDEX, CMD_REMOVE_BY_TAG, and CMD_REMOVE_BY_CMDID sounds good. Remove-multiple (i.e. range) could be a modifier (either alt or meta, idk) for INDEX/TAG, and perhaps CMDID could also remove one by default and remove all with the remove-multiple modifier.

Personally, don't see that path, since then we'd have 3 commands with overlapping uses, ie, CMD_REMOVE, CMD_REMOVE_BY_TAG and CMD_REMOVE_BY_CMDID. People would still be using the old CMD_REMOVE since why would they bother to change it. Then we'd still need the modifier in all commands, to select range or filtering.

Also keep in mind we added the tag option for range just because REMOVE already supported that and otherwise would be incomplete, but don't think that's going to have much use, so if creating CMD_RANGE we could just not add tag support, and wait to see if it's smth needed later on.

I much prefer CMD_REMOVE and CMD_REMOVE_RANGE, then we keep CMD_REMOVE as is, and just add CMD_REMOVE_RANGE doing the range removal, together with the same modifier the other command uses for selecting tag/index. We end with two commands to do everything and nothing is really lost.

The other way we end with 4 commands each with different modifiers and behaviour.

Option A)

• CMD_REMOVE -> uses alt to use tag or id
• CMD_REMOVE_BY_TAG -> uses alt to range or filter
• CMD_REMOVE_BY_INDEX -> uses alt to range or filter
• CMD_REMOVE_BY_CMDID -> filtering only (no range option since doesn't make much sense imo)

Option B)

• CMD_REMOVE -> uses alt to use tag or id
• CMD_REMOVE_RANGE -> maybe no options, since by index is only useful option, or else use alt to use tag or index

I think (B) much better and we don't pollute the command list so much. Also CMD names much nicer imo.

[sprunk]

Also keep in mind we added the tag option for range just because REMOVE already supported that and otherwise would be incomplete, but don't think that's going to have much use

Tag is important for polishing because it avoids latency-based race condition when given from unsynced.

People would still be using the old CMD_REMOVE since why would they bother to change it.

Yes, but that is fine. We can deprecate it and encourage people to use the dedicated ones. There's got to be like 20 uses of cmd.remove total across the whole ecosystem.

together with the same modifier the other command uses for selecting tag/index. We end with two commands to do everything (...) we don't pollute the command list so much

As far as I understand, having complex commands that do everything and change parameter meaning based on modifiers is why @rhys-vdw suggested the split-off in the first place. If we're using an opportunity to have primitive commands then we shouldn't do the bare minimum, in fact I maybe we could even have 7+ commands (existing remove, by tag, by tag range, by index, by index range, by cmdid all, by cmdid single) where none of them take any modifier that modifies the meaning of parameters (only ctrl to choose which queue the removal applies to). Pollution is going to exist either way because there's just so many ways to remove (equivalently, meanings that parameters can have), your method just moves it to modifier options which I think is worse (especially while #1063 exists).

[saurtron]

Well sorry, but I don't see it at all, it's as bad to have too many options as too many commands (pollutes the command space, documentation and code too), I'll do the CMD_REMOVE_RANGE and we can see.

Also it's not like doing it the way you want is saving any options, since those would still need 1 option in general, just like CMD_REMOVE_RANGE.

[saurtron]

Implemented CMD_REMOVE_RANGE and changed PR description and title accordingly.

[sprunk]

The remaining minor changes for regular remove could be in a separate PR since they seem uncontroversial.

[saurtron]

The remaining minor changes for regular remove could be in a separate PR since they seem uncontroversial.

Actually they are controversial XD. If left here would require a bit of a refactor. That's because in some places the cpp code just does uses the REMOVE command to remove a command with a specific tag from the queue, and it shouldnt send the CmdDone there probably.

Wanted to say that but you beat me to it.

[rhys-vdw]

Well sorry, but I don't see it at all, it's as bad to have too many options as too many commands (pollutes the command space, documentation and code too), I'll do the CMD_REMOVE_RANGE and we can see.

I disagree with this because the options don't map to keystrokes in this case. I see no reason to use the options argument which has already been flagged as kind of outdated cruft. I actually think it makes more sense to move away from options in most, if not all cases.

Even when the command does relate to inputs, the use of this pattern has resulted in difficulties remapping inputs such a shift to queue or shift to place a line of buildings. These operations are still not rebindindable because of options being hardcoded into the engine.

When adding unit orders to a factory queue, it makes more sense to check for keybindings and send a different quantity e.g. send ID, 5 or ID, 50 rather than ID, ctrl or ID, ctrl, shift.

Basically everything can be expressed with primitives and parameters, so why muddy things with options that everyone seems to agree are confusing and limiting?

I think @sprunk and I are on the same page here:

If we're using an opportunity to have primitive commands then we shouldn't do the bare minimum, in fact I maybe we could even have 7+ commands (existing remove, by tag, by tag range, by index, by index range, by cmdid all, by cmdid single) where none of them take any modifier that modifies the meaning of parameters (only ctrl to choose which queue the removal applies to).

Even the queue selection could be passed as a param, but any movement away from using options is a good choice IMO.

Obviously I'm new to the codebase so feel free to point out what I'm missing here, but a 1:1 list of clearly named command IDs to command functionalities feels like both the ideal and obvious way to do things.

[saurtron]

I disagree with this because the options don't map to keystrokes in this case. I see no reason to use the options argument which has already been flagged as kind of outdated cruft. I actually think it makes more sense to move away from options in most, if not all cases.

The options are not really flagged as outdated cruft, just their names. If we do go that path of deprecating options, or changing names, then it could be the case but it's not. So I think we should stick to current standards of how things work, instead of creating a new ad-hoc standard that might not even be final.

Keep in mind this PR's scope is not creating a new command option standard.

Even when the command does relate to inputs, the use of this pattern has resulted in difficulties remapping inputs such a shift to queue or shift to place a line of buildings. These operations are still not rebindindable because of options being hardcoded into the engine.

The problem is not the option names, don't think those correspond to the actual keys since they do not. The problem is not about the options that you can't rebind, the problem is the engine ui code is creating those commands in custom parts of the code, but they need to be refactored into actions that ppl will be able to bind. The options can be used all the same so believe me those are really not a problem. (I'm working on fixing those too, and the option 'names' themselves are not a problem)

Basically everything can be expressed with primitives and parameters, so why muddy things with options that everyone seems to agree are confusing and limiting?

As said, I think we should stick to current standards since it's not the scope of this PR to create a new command interface/standard.

I agree maybe for tag/id we could use a parameter, although I think it's a bit awkward, since I believe the common way will be to use index with it, and tag is kind of an extra option, but we already have varargs, so either it has to be the first param or need to provide a nil in some situations, so it would end like one of these:

• (1) Command(CMD_REMOVE_RANGE, unitId, {range_start, range_end, altqueue})
• (2) Command(CMD_REMOVE_RANGE, unitId, {altqueue, range_start, [range_end]})
• (3) Command(CMD_REMOVE_RANGE, unitId, {altqueue, range_start, [range_end]}, [{'ctrl'}])

For example, to remove all but the first element with altqueue would be:

• (1) Command(CMD_REMOVE_RANGE, unitId, {2, nil, true})
• (2) Command(CMD_REMOVE_RANGE, unitId, {true, 2})
• (3) Command(CMD_REMOVE_RANGE, unitId, 2, CMD.OPT_CTRL) -- no tables == better
• (3) Command(CMD_REMOVE_RANGE, unitId, 2, OPTS.REMOVE_RANGE.ALTQUEUE) -- not real example, but just to show how with actual names the options suddently become nice again

If tag would be a parameter then it could be even worse:

• (1) Command(CMD_REMOVE_RANGE, unitId, {2, nil, true})
• (2) Command(CMD_REMOVE_RANGE, unitId, {true, nil/false, 2})
• (3) & (4) stay the same as they are options.

Solving the awkward naming of options is going to be tricky, but could be done by specifying per command maps, where we map the options to their bit codes internally, for example:

CMD_REMOVE_RANGE_OPTIONS = {
  'ctrl' => 'altqueue',
  'alt' => 'byTag',
  shift' => 'append',
}

Then ppl could use as CMD.OPT_ALTQUEUE, or OPTS.REMOVE_RANGE.ALTQUEUE or opts.altqueue, opts.byTag, etc.

This could even be declared from c++ in some way like AddCommandOption(REMOVE_RANGE, ALTQUEUE, CTRL), anyways just trying to show how the options themselves are not bad, it's just about the namings. We could also devise a way to have a deprecation path by automatically converting old names to new ones when passed into the engine, and providing both new and old into opts tables.

[saurtron]

Ok, removed the tag option, since that seems to be controversial, also lua method to GetIndexFromTag or so could make it uneeded, so maybe better to create that one afterwards.

Don't really see creating lots of commands with ugly names, and also don't see creating lots of similarly named methods, I think that also makes the api ugly and confusing to use, not just having options.

Regarding options themselves, above I described a mechanism to make them look on-point from lua, maybe some day we can do that or smth in that direction, personally doesn't bother me too much, since there's more pressing issues tbh.

I don't want to set a new command standard here, just implement a simple Range command. Currently commands have simple names and sometimes several options, that's how it works now pretty much. Trying to come up with a new standard in every PR is a bad idea imo, and when one is implemented, everything should be changed swiftly, otherwise we'd just have several standards coexist and later before one is implemented I'm sure the goals will move again and there'll be a new standard and so on.

I'd be willing to maybe make this a lua method, instead of (or in addition to) a command, that would save some network bytes, but wouldn't be usable from widgets. Other than that I think the feature is Remove(index range).

[rhys-vdw]

Ok, removed the tag option, since that seems to be controversial

So the new API is just REMOVE_RANGE with "ctrl" changing whether it's the main or alt queue?

also lua method to GetIndexFromTag or so could make it uneeded, so maybe better to create that one afterwards.

That makes a lot of sense to me.

I'm still confused what the index of a tag would be. Is it FindFirstTaggedCommandInQueue? Or are tags unique? What if you wanted to find the last tag?

The options are not really flagged as outdated cruft, just their names. [...]

I'm not going to continue this conversation here, and I don't want to block your PR. I didn't think the suggestion would be controversial! So I hope to find time to write up my thoughts in a more structured way for ppl to think about and have a more productive conversation.

[saurtron]

So the new API is just REMOVE_RANGE with "ctrl" changing whether it's the main or alt queue?

Yes, I'll add a PR afterwards with GetCommandTagIndex or smth like that, so we can have the tag version "for free".

I'm still confused what the index of a tag would be. Is it FindFirstTaggedCommandInQueue? Or are tags unique? What if you wanted to find the last tag?

Was noted elsewhere, but the tag for a command is a unique id identifying it in it's queue. So when you know the tag of a command you can query or address it directly through that.