Add Spring.GetUnitCurrentCommandID a cmdID from a units queue.

[saurtron]

Work done

• Add Spring.GetUnitCurrentCommandID to get the cmdID for a specific command at the units queue.

Remarks

• Since most of the implementation is the same as GetUnitCurrentCommand can wait to merge Allow negative index at GetUnitCurrentCommand to index from queue end. #1813 and refactor this so both functions reuse code.
• Could be called Spring.GetUnitCommandID instead maybe, but then the other function would have to be renamed too for consistency.

Rationale

• I seen several widgets looking at the last command in queue, or sometimes first one, just to see if it's a specific cmdID. Since this can usually be done at AllowCommand or UnitCommand callins I think it's worth it to provide a method that's more optimal, since that can run so many times when a command is given for 100s of units against 10s/100s of other units.

Related issues

• Add "last commands" and "command count" getters #1725

[sprunk]

Why? Isn't this just Spring.GetUnitCurrentCommand that fails to push most args? Just do local cmdID = Spring.GetUnitCurrentCommand(unitID, n).

[saurtron]

Why? Isn't this just Spring.GetUnitCurrentCommand that fails to push most args? Just do local cmdID = Spring.GetUnitCurrentCommand(unitID, n).

Yeah I know it can be done like that, but why push them when there's no need?

I think there's value in having this version that's why I propose it. Like I commented at the PR description, this is a pattern used in really critical parts of the gadgets/widgets so it can help to save pushing a few args and looping the params.

[sprunk]

I'm sceptical, my intuition is that pushing 3 numbers or not would be barely measurable.

I made a benchmark where I tried to put the change in the best possible light by making pushing the extra numbers take as much time comparatively as possible and other overhead as cheap as possible:

local function f4()
	return 1, 2, 3, 4
end

local x = os.clock()
for i = 1, 500000000 do
	local j = f4()
	if j == 23 then
		return
	end
end
print (os.clock() - x)

• function is in Lua and not C++
• function does nothing else but push return values
• nothing interesting happens to the returned value
• the algorithm is just to call the function a billion times.

Removing the extra return values nets me a -25% execution time. Given these circumstances it's pretty terrible actually. Especially the last bullet point is important because surely there's better solutions. If you really get to the point where you need to iterate order queues over and over perhaps it's best to introduce some sort of function that iterates in C++ and returns whether and where it found the command, e.g. Spring.IsCommandInQueue(unitID, cmdID[, startIndex = 1]) -> integer? index.

I couldn't find any practical examples. I've heard these two widgets are a problem but neither of them even calls Spring.GetUnitCurrentCommand and neither of them would want to skip the other return values from it anyway because they both need the command tag to remove the command in question.

[saurtron]

If you really get to the point where you need to iterate order queues over and over perhaps it's best to introduce some sort of function that iterates in C++ and returns whether and where it found the command, e.g. Spring.IsCommandInQueue(unitID, cmdID[, startIndex = 1]) -> integer? index.

Good idea if we see the need arises :D, could solve the concerns this command solves too you're right.

I couldn't find any practical examples. I've heard these two widgets are a problem but neither of them even calls Spring.GetUnitCurrentCommand and neither of them would want to skip the other return values from it anyway because they both need the command tag to remove the command in question.

Those two should be calling GetUnitCurrentCommand from now on instead since they're just interested in the last element.

The tag, it can skipped as long as there's no commands matching the ids it's looking for.

Regarding the execution time, it's not just about that, it's about lua memory use too, although maybe those pushed values don't end in the garbage collector queue (not sure about internals there tbh), in that case probably not worth it.

[saurtron]

Spring.IsCommandInQueue(unitID, cmdID[, startIndex = 1]) -> integer? index

I'm thinking to withdraw this PR and implement that method instead. Would solve the same problem and much more powerful. You sure you think that one is ok right? Just to make sure before implementing (not a bit deal but ok).

edit: Or well, actually the problem is sometimes they need to check several ids :P, so actually maybe not a viable strategy in general.

[sprunk]

Sounds ok but mostly because it is something newbie gamedevs might expect to exist, otherwise it feels skippable (usability issues with several IDs, still no evidence of perf either way).

[saurtron]

Sounds ok but mostly because it is something newbie gamedevs might expect to exist, otherwise it feels skippable (usability issues with several IDs, still no evidence of perf either way).

Yep I'll keep this one closed until how it affects memory/perf can be investigated a bit more. There's already too many open PR's anyways XD.