New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Anti-bait gadget for things like Artemis #3886
Anti-bait gadget for things like Artemis #3886
Conversation
@@ -416,7 +488,10 @@ end | |||
|
|||
function gadget:AllowCommand(unitID, unitDefID, teamID, cmdID, cmdParams, cmdOptions) | |||
if (cmdID ~= CMD_PREVENT_OVERKILL) then | |||
return true -- command was not used |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
weird nesting, how about
if cmdID == CMD_PREVENT_OVERKILL then
return PreventOverkillToggleCommand(unitID, cmdParams, cmdOptions)
elseif cmdID == CMD_METAL_DESIRED_TO_PREVENT_OVERKILL then
return PreventMetalOverkillToggleCommand(unitID, unitDefID, cmdParams, cmdOptions)
else
return true
end
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, though pretty much everything I've written is horrible for this...
If it barely interacts with OKP then it does not need to be in the OKP gadget. I just thought you might need to use BlockShot and all the relevant units (should) already call an OKP function in BlockShot. When making the new gadget make sure to look at other gadgets with gadget:AllowWeaponTarget and set the layer in a way that makes sense. |
params = {0, 0, 100, 300, 1000} | ||
} | ||
|
||
function GG.ChaffShootingPrevention_CheckMinMetal(unitID, targetID, damage) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this GG?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps because you want to call GG.ChaffShootingPrevention_CheckMinMetal in Unit Priority to make such targets lower priority? Is that required with this interaction?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think it's needed. Removed.
end | ||
return true, defPriority | ||
end | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should AllowWeaponTargetCheck or AllowUnitTargetRange also be used here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm unclear on what the former does and can't find the latter in https://springrts.com/wiki/Lua:Callins
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Experiment with both. See the gadget handler for AllowUnitTargetRange.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AllowUnitTargetRange is not a Spring callin. Spring overloads gadget:AllowWeaponTarget to do (at least) two distinct things by setting magic numbers in its parameters. I split one of the other uses off into AllowUnitTargetRange in the gadget handler.
|
||
if spValidUnitID(unitID) and spValidUnitID(targetID) then | ||
local gameFrame = spGetGameFrame() | ||
local unitDefIDAsKnown = CallAsTeam(Spring.GetUnitTeam(unitID), spGetUnitDefID, targetID) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I prefer
local states = spGetUnitLosState(unitID, allyTeamID, true)
nowdays.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed.
preventChaffShootingCmdDesc.params = newParams | ||
spEditUnitCmdDesc(unitID, cmdDescID, {params = preventChaffShootingCmdDesc.params}) | ||
end | ||
Spring.SendLuaRulesMsg(minMetalMsg .. " ".. tostring(unitID) .. " " .. tostring(MetalHandledUnitDefIDs[unitDefID][state + 2])) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is a gadget sending a LuaRulesMsg?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So that people can write their own widgets that call in to the gadget and change a unit's minimum metal targetting threshhold.
I haven't split this into the UI widget and the underlying gadget yet.
e09f416
to
ce24b2c
Compare
Ready for review @GoogleFrog |
-- Value is the default state of the command | ||
local metalThreshholdsByUnitDefIDs = { | ||
[UnitDefNames["hoverarty"].id] = {0, 0, 100, 300, 1000}, | ||
[UnitDefNames["cloaksnipe"].id] = {0, 0, 100, 300, 1000}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this supposed to be the same table for every unit? In that case could do
-- Value is the default state of the command | |
local metalThreshholdsByUnitDefIDs = { | |
[UnitDefNames["hoverarty"].id] = {0, 0, 100, 300, 1000}, | |
[UnitDefNames["cloaksnipe"].id] = {0, 0, 100, 300, 1000}, | |
local STATES = {0, 0, 100, 300, 1000} -- first value is the default state of the command | |
local metalThreshholdsByUnitDefIDs = { | |
[UnitDefNames["hoverarty"].id] = STATES, | |
[UnitDefNames["cloaksnipe"].id] = STATES, |
etc
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's due to me waffling back and forth over whether or not this should vary between units.
Eventually decided on simplicity.
Fixed now
preventChaffShootingCmdDesc.params = newParams | ||
spEditUnitCmdDesc(unitID, cmdDescID, {params = preventChaffShootingCmdDesc.params}) | ||
end | ||
Spring.SendLuaRulesMsg(minMetalMsg .. " ".. tostring(unitID) .. " " .. tostring(metalThreshholdsByUnitDefIDs[unitDefID][state + 2])) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks suspicious, why not just call the relevant thing directly?
Spring.SendLuaRulesMsg(minMetalMsg .. " ".. tostring(unitID) .. " " .. tostring(metalThreshholdsByUnitDefIDs[unitDefID][state + 2])) | |
unitMetalMin[unitID] = metalThreshholdsByUnitDefIDs[unitDefID][state + 2] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can't remember what this was about...
Changed to your suggestion
end | ||
|
||
function gadget:RecvLuaMsg (msg, playerID) | ||
if msg:sub(1, #minMetalMsg) ~= minMetalMsg then |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm unsure if the #
operator works on string length reliably. But this whole func may not be needed, see above.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed function.
if canHandleUnit[unitID] then | ||
if unitMetalMin[unitID] then | ||
unitMetalMin[unitID] = nil | ||
end | ||
canHandleUnit[unitID] = nil | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking the value up takes just as much time as setting it
if canHandleUnit[unitID] then | |
if unitMetalMin[unitID] then | |
unitMetalMin[unitID] = nil | |
end | |
canHandleUnit[unitID] = nil | |
end | |
unitMetalMin[unitID] = nil | |
canHandleUnit[unitID] = nil |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doesn't that bloat canHandleUnit unnecessarily though? Taking up a (small) amount of extra memory on units that don't use metal threshhold targetting?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As far as I know nil
takes up no space.
Whatever came of the idea of also adding a set of bad targets? Felon wants to shoot Ronin but not Razor. AA wants to shoot Raven but not Raptor, and both units cost the same. This gadget doesn't seem able to do either of these things. I'm also wondering how to integrate it with the default UI. Maybe it should distinguish between manual commands and automatic target acquisition (don't fire at radar could have a mode where it does the same)? I'm not all that keen on adding a state toggle to the default UI, and it would be a shame for people to have to be told this exists to use it. I will most likely look closer on the weekend. |
Just updated the name for searchability. |
A Phantom with Fight shoots Glaives. |
Which is a bad thing. |
Maybe it's not bad, but it is certainly an oversight. Solving this was the big task in Don't Fire At Radar, and I dislike my solution there. It messes with command queues a lot and possibly isn't even scalable. I was hoping for a solution here, but I guess this at least works perfectly for turrets (as far as I can tell) and units without Fight. I'll pull it on that basis, although I am going to have to rename everything so I can search it years later. I consider this type of thing 'bait', and don't think it should be tied exactly to metal. |
Redone, tis the anti-chaff gadget is now ready for review.