Add widgetHandler:GetSelectedUnits.

[amnykon]

Part 1 of Change.

Part 2 would replace widget calls to Spring.GetSelectedUnits() with widgetHandler:GetSelectedUnits() and Spring.GiveOrderToUnit () with widgetHandler:UnitCommandNotify().

Part 1 should have no effect on current behavior. Contains all changes that cannot be changed via local widgets.

[amnykon]

Unhanded behavior does not call widgetHandler:GetSelectedUnits, which does not allow widgets to modify the selection units.

[amnykon]

Default behavior does not call widgetHandler:UnitCommandNotify, preventing widgets from using this hook.

[amnykon]

calls that use this check for unhanded events and call this anyway. This makes that calls easier as they don't need to check if it is unhanded.

[amnykon]

Can be tested with #4646. Since part 2 is not done, it blocking the attack command will only work with unhanded commands, so no commands that any other widgets handles.

[GoogleFrog]

Can you step back and provide motivation? I don't know what this is parts 1 or 2 of. I'm not keen on a design that will have widgets regularly call widget the widget handler within the code of widgets, and I don't know whether this change is trying to do that or not.

[GoogleFrog]

This callin seems misnamed. Spring.GetSelectedUnits already exists but widget:GetSelectedUnits seems to be doing something different. Also why is a callin a getter?

[amnykon]

Would widgetHandler:filterSelectedUnits(selected, id, params, options) be better. and make callers call Spring.GetSelectedUnits() for selected argument? should widgetHandler call Spring.GetSelectedUnits() if selected is nil, as Spring.GetSelectedUnits() is the expected value for this argument?

[amnykon]

Can you step back and provide motivation? I don't know what this is parts 1 or 2 of. I'm not keen on a design that will have widgets regularly call widget the widget handler within the code of widgets, and I don't know whether this change is trying to do that or not.

Spring.GetSelectedUnits gets the selected units from the engine. widget:GetSelectedUnits gets the selected units from the engine, then allows all widgets a chance to modify the list of units. Without this I would need to add logic in all widgets that call widget:GetSelectedUnits to filter out units that have cmd_disable_attack set when an attack command is issued. Is there another way you feel would be better to accomplish this? I could add and argument to widgetHandler:CommandNotify(id, params, options, selected); however, widgets also use function widget:MouseRelease(mx, my, mButton) instead of CommandNotify.

[amnykon]

Why are you against widgets calling the WidgetHandler?

[amnykon]

Using an include function instead of widget handler.

[amnykon]

Also updated several widgets to use the function. The GetSelectedUnits function calls a function on cmd_disable_attack from another PR, checking for its existence first, so it's not required for this change; however this change should not change behavior without it.

[GoogleFrog]

The thing about this PR is the cost:benefit ratio.

• The intermittence cost is high. The new function GetSelectedUnits shadows Spring.GetSelectedUnits and doesn't actually get selected units. Every widget has to use it, otherwise the feature will stop working. Widgets that want to use the filtering feature have to be manually added to Units.lua, and Units.lua lives in a weird spot and has a non-descriptive name. This is unmoddable. Many widgets will get identical behaviour from Spring.GetSelectedUnits and GetSelectedUnits, until something is manually added to Units.lua which breaks them en-mass.
• In terms of the feature this PR is going to support, the direct benefit to the average player is none because it won't be enabled by default. I don't expect many powerusers to find and use this either. So most of the benefit that I see is in pulling the PR, rather than code that the PR adds.

Basically, I don't want to pay performance or maintenance costs for this feature.

On a technical level I'm doubtful that this PR is the best way to add this functionality. Eg custom formations does something fancy with GiveNotifyingOrder that gives other widgets a chance to chime in on what happens. I'd want to know why something like this can't be done before doing something that is much harder to maintain.

[amnykon]

I have implement this 2.5 times(2nd implement was only partly done)

GiveNotifyingOrder reissues the order to the widgetHandler, giving widgets a chance to chime in; however, it does not change the units the order is being issued to. Should we add units as an optional argument? With this approach, we could also remove widgetHandler:UnitCommandNotify, which is only used in a few places.

What direction do you have in mind for the implementation of this feature?

[sprunk]

GiveNotifyingOrder reissues the order to the widgetHandler, giving widgets a chance to chime in; however, it does not change the units the order is being issued to.

Block the order wholesale but issue individual orders that go through widgetHandler:UnitCommandNotify. Pseudocode:

local function GiveNotifyingOrder(unitID, cmdID, bla)
  if widgetHandler:UnitCommandNotify(unitID, cmdID, bla) then return end
  Spring.GiveOrderToUnit(unitID, cmdID, bla)
end

function widget:CommandNotify(cmdID, bla)
   local units = Spring.GetSelectedUnits()
   for i = 1, #units do
     local unitID = units[i]
     if someCondition(unitID) then -- decide which unit gets which command
       GiveNotifyingOrder(unitID, CMD_DIFFERENT, bla)
     else
       GiveNotifyingOrder(unitID, cmdID, bla) -- manually apply the current command since it's getting blocked below
    end
  end
  return true -- block the command, everything received a command above
end

[GoogleFrog]

CommandNotify and UnitCommandNotify both filter out the command with return true so, as far as I can tell, they are almost entirely sufficient. That is, they have the ability to issue the order to any set of units. Spring.GiveOrder doesn't issue orders to units directly so I wouldn't want CommandNotify to pretend that it does. And I don't see why I'd want to remove UnitCommandNotify.

CommandNotify and UnitCommandNotify cannot handle all commands at the moment because some widgets use them to do UI things such as graphics and sounds. But if these callouts were extended with some sort of internal flag for widgets to set, I don't see why more widgets could notify in an internal way, and then an attack blocking widget could filter them out. Filtering is already possible.

Perhaps another notifying command would be better, and it could be hooked into Spring callins to do the filtering automatically. Then if no widget uses the callin the handler could ignore it, saving performance for everyone. Ideally though, the CommandNotify and UnitCommandNotify changes would be good and performant enough to simplify widgets such as area attack tweak.

I do not want widgets calling some function to get lists of units that they are allowed to issue particular orders to. This sounds like a mess, and lists of units do not necessarily have such narrow uses! Eg what if a widget only decides which type of order to issue after looking at features of the list. And it puts the onus on every widget to anticipate compatibility with future filters, rather than just using the standard widgetHandler interface.

[GoogleFrog]

Have there been any recent advances? My misgivings still stand, I want to know what adding this system that seemingly does something that can already be done reasonably in alternate ways achieves. Adding duplicate functionality under a different paradigm (that of widgets routinely calling functions in the widgethandler, rather than those functions mostly being callins), just seems like a recipe for nasty bugs to me.

[amnykon]

I got distracted by other things and by life. I am working on an implementation that widgets use WG.units and WG.cmdID instead of callins.