Update unit_dgun_stall_assist.lua [NEEDS TESTING] #2856
Conversation
refactored
There was a problem hiding this comment.
Firstly, thank you for making this upgrade, it's long overdue. And congrats on your first PR!🙂
To warn you upfront, this is a pretty strict review, this widget has a long history of breaking so I want it to be extra safe, and it's a good example case for what to look for in terms of bugs, and how to optimize potentially expensive tasks.
Some high-level notes before we get to the specifics:
- It is convention in this codebase to use
camelCaseinstead ofsnake_casefor both function names and variable names - If a Spring engine call is made more than once per game frame, it should probably be cached (this just means putting something like this at the top of the file:
local spGetUnitCurrentCommand = Spring.GetUnitCurrentCommand)
If you would prefer, I can just push the changes I made during testing/review, but if you feel like doing more widget work this could be a good way to get more familiar 🙂
If you want a full reference for the changes I made, the entire file is here: https://github.com/thehobojoe/Beyond-All-Reason/blob/dgun-stall-refactor/luaui/Widgets/unit_dgun_stall_assist.lua
| function widget:GetInfo() | ||
| return { | ||
| name = "DGun Stall Assist", | ||
| name = "DGun Stall Assist v2", |
There was a problem hiding this comment.
This will be an upgrade to the existing widget, so no need to change the name
| local function getWaitAbleUnits() | ||
| local myWaitableUnits = {} | ||
| local myUnits = Spring.GetTeamUnits(Spring.GetMyTeamID()) | ||
| for i=1, #myUnits do | ||
| local unitCmd = Spring.GetUnitCurrentCommand(myUnits[i]) | ||
| local isEnergyConsumingCommand = not (unitCmd == CMD.WAIT or unitCmd == CMD.MOVE or unitCmd == CMD.RECLAIM) | ||
| if isEnergyConsumingCommand then | ||
| local unitDefId = Spring.GetUnitDefID(myUnits[i]) | ||
| if waitableUnits[unitDefId] then myWaitableUnits[#myWaitableUnits + 1] = myUnits[i] end | ||
| end | ||
| end | ||
| return myWaitableUnits | ||
| end |
There was a problem hiding this comment.
This can be rewritten. There are a few issues here
- Units that are not consuming energy will get paused
- Rezzing is not identified
- It does more iteration than it needs to
This is how I've rewritten it:
local function getEnergyConsumingBuilders()
local needsWait = {}
local myUnits = Spring.GetTeamUnitsSorted(Spring.GetMyTeamID())
for unitDefID, units in pairs(myUnits) do
if waitableUnits[unitDefID] then
for i, unit in pairs(units) do
local metalMake, metalUse, energyMake, energyUse = spGetUnitResources(unit)
if energyUse > 0 then
needsWait[#needsWait + 1] = unit
end
end
end
end
return needsWait
endGetTeamUnitsSortedwill return an array of arrays, where each unitdef is its own array. We can completely skip any analysis of unitDefID's that we don't care aboutGetUnitResourceswill let us skip any order analysis and just check if the units are draining energy. There's a theoretical chance that this can give false-positives during hard stalls, however during my testing, units still try to consume a lot of energy even during hard stalls. I consider it good enough. We also use a cached version here since it will be called many times per frame.
| local function doWeHaveLowEnergy() | ||
| local currentEnergy, energyStorage = Spring.GetTeamResources(Spring.GetMyTeamID(), "energy") | ||
| if energyStorage < dgun_cost then return false end | ||
| if currentEnergy < target_energy then return true end | ||
| return false | ||
| end |
There was a problem hiding this comment.
It's only possible to have less than 500 energy storage if you don't have a commander. This can be simplified to
local function doWeHaveLowEnergy()
local currentEnergy, energyStorage = Spring.GetTeamResources(Spring.GetMyTeamID(), "energy")
return currentEnergy < targetEnergy
end
There was a problem hiding this comment.
It's only possible to have less than 500 energy storage if you don't have a commander
what are tweakdefs
what are mods
| local function wait_units() | ||
| local time_now = millis() | ||
| if next_call_when > time_now then return end | ||
| next_call_when = time_now + time_in_ms_befor_unwaiting | ||
| UNITS_WAITING = true | ||
| for _, unitID in pairs(getWaitAbleUnits()) do | ||
| waitingUnits[#waitingUnits +1] = unitID | ||
| end | ||
| Spring.GiveOrderToUnitArray(waitingUnits, CMD.WAIT, {}, 0) | ||
| end |
There was a problem hiding this comment.
More of a style thing, but conditional timing shouldn't take place in functions with such self-explanatory names like this.
Also it's very important that we check current commands, so we don't erroneously unwait units that already have an active wait. This is an unlikely case given that waited units shouldn't be consuming energy, but I think it's better safe than sorry.
local function waitUnits()
waitingUnits = getEnergyConsumingBuilders()
local toWait = {}
for i = 1, #waitingUnits do
local uCmd = spGetUnitCurrentCommand(waitingUnits[i], 1)
if uCmd ~= CMD_WAIT then
toWait[#toWait + 1] = waitingUnits[i]
end
end
Spring.GiveOrderToUnitArray(waitingUnits, CMD_WAIT, {}, 0)
end| local function unwait_units() | ||
| Spring.GiveOrderToUnitArray(waitingUnits, CMD.WAIT, {}, 0) | ||
| UNITS_WAITING = false | ||
| waitingUnits = {} | ||
| end |
There was a problem hiding this comment.
It's even more important here to check for an existing wait command, so that we don't accidently wait units.
local function unwaitUnits()
local toUnwait = {}
for i = 1, #waitingUnits do
local uCmd = spGetUnitCurrentCommand(waitingUnits[i], 1)
Spring.Echo("ucmd", uCmd)
if not uCmd or uCmd == CMD_WAIT then
toUnwait[#toUnwait + 1] = waitingUnits[i]
end
end
Spring.GiveOrderToUnitArray(toUnwait, CMD_WAIT, {}, 0)
waitingUnits = {}
lastDgunTime = 0
end| local function buildWaitableUnitsTable() | ||
| for uDefID, uDef in pairs(UnitDefs) do | ||
| if (uDef.buildSpeed > 0) and uDef.canAssist and (not uDef.canManualFire) then | ||
| waitableUnits[uDefID] = true | ||
| end | ||
| end | ||
| end |
There was a problem hiding this comment.
Again more of a style thing, but it's generally convention to put this caching process outside of a function near the top of the widget. It will run when the widget loads. Also this code will not catch factories or rezbots
for uDefID, uDef in pairs(UnitDefs) do
if (uDef.buildSpeed > 0) and (uDef.canAssist or uDef.canResurrect or uDef.isFactory) and (not uDef.canManualFire) then
waitableUnits[uDefID] = true
end
end| local spGetUnitDefID = Spring.GetUnitDefID | ||
| local spGetSpectatingState = Spring.GetSpectatingState | ||
| local spGetUnitIsBeingBuilt = Spring.GetUnitIsBeingBuilt | ||
| function widget:GameFrame() |
There was a problem hiding this comment.
We do not want to do this in GameFrame, it'll run way too often. Do it in the update loop. The timing here felt very overcomplicated to me, I have simplified it to this:
function widget:Update()
timer = timer + Spring.GetLastUpdateSeconds()
local lowEnergy = doWeHaveLowEnergy()
local _, activeCmdID = Spring.GetActiveCommand()
dgunActive = activeCmdID == CMD.DGUN
if dgunActive then
lastDgunTime = timer
end
if dgunActive and lowEnergy and #waitingUnits == 0 then
waitUnits()
return
end
if
not dgunActive and
#waitingUnits > 0 and
lastDgunTime > 0 and
lastDgunTime < timer - unwaitThreshold
then
unwaitUnits()
end
end|
|
||
| function maybeRemoveSelf() | ||
| local function maybeRemoveSelf() |
There was a problem hiding this comment.
Add Spring.IsReplay() to this
| spGiveOrderToUnitArray(waitedUnits, CMD_WAIT, {}, 0) | ||
| end | ||
| end | ||
| if Spring.IsReplay() then widgetHandler:RemoveWidget() end |
There was a problem hiding this comment.
With unitdef caching taking place at the top of the file, this can be
function widget:Initialize()
maybeRemoveSelf()
end
|
I highly recommend this as a DO NOT MERGE until it has received live game testing, with or without any of my recommend changes. |
thehobojoe
changed the title
|
I updated my reference widget, it was missing special handling for labs which is required to unwait them properly. |
Hi, the refferance file here seems to link to the original file thats in the base game currently, I was just looking for the current version of the new one with the changes you've made to do a bit of work testing and see if I can implement rez functionality |
|
Gah, sorry! Clipboard mixup, here is the correct link https://github.com/thehobojoe/Beyond-All-Reason/blob/dgun-stall-refactor/luaui/Widgets/unit_dgun_stall_assist.lua |
Work done
rewrote entire script to a functional paradigm
Addresses Issue(s)
has fixed an issue of not un-waiting units
To do
currently script doesn't filter out units that are rezing so if you have a lot of active rez it wont work