Skip to content
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

Lua Select API, Lua based KeySelect & Customisable Smart Select #3757

Open
wants to merge 40 commits into
base: master
Choose a base branch
from

Conversation

whossname
Copy link

Work done

Created a Lua based replacement for the existing hotkey selection code and added functionality to SmartSelect to utilise the same API to modify which units are selected when using drag select.

There were two main motivations for this:

  • the existing C++ API can be inconsistent and buggy in places. As far as I can tell the NameContains and Category rules are broken, and the select count/one/part conclusions includes units that are already selected, which can be counter intuitive.

  • writing it in Lua allows the code to be updated, maintained and improved more easily. Far more people should be able to work on the Lua code, and it should be easier to apply the changes.

An additional driver was being able to use the select syntax for SmartSelect commands in uikeys.txt.

The following filter was not implemented, I have no idea what it is meant to do: RulesParamEquals_<string>_<integer>

I was advised that no one uses it anyway. Also an extra rule AntiAir was added for selecting Anti-air units.

The relevant files are:

  • select_api.lua the common code used in both SmartSelect and KeySelect to decode selection command strings and cache the resulting select function
  • unit_smart_select.lua this widget was modified to use the select api for selectbox commands in uikeys.txt
  • unit_key_select.lua intended as a replacement for the existing select commands. This widget is disabled by default as we want users to test it before it is fully rolled out as a replacement.
  • compare_to_spring.lua a test script written to identify differences between the existing C++ select implementation and the new Lua select implementation. It's not a proper unit/integration test as the results require a lot of interpretation.

See discord thread for more details: https://discord.com/channels/549281623154229250/1266331814440730677

Setup

The changes to SmartSelect are merged in directly. It will require modification to the uikeys.txt file to use. Something like this:

bind          Any+shift  selectbox Not_Aircraft
bind           Any+ctrl  selectbox Buildoptions
bind            Any+alt  selectbox Aircraft
bind          Any+space  selectbox Builder_Aircraft

To test the KeySelect open widgets and enable the KeySelect widget.

Test steps

SmartSelect

  • Copy the above code into uikeys.txt and run /keyreload
  • Create a variety of air/land units
  • The shift, ctrl, alt and space modifiers should change which units are selected
  • Check that the existing SmartSelect drag selection key binds still work as expected

KeySelect

  • Run /wigetselector and enable KeySelect
  • Check that all of your keybinds still work

Known issues

There seems to be an issue where the NameContain filter doesn't work as expected if you have ClearSelection in the conclusion. It will centre on the unit, but not actually select it. I suspect it also happens for the Category filter. No idea why. Seeing as the NameContain and Category filters don't seem to work correctly in the existing version anyway, I don't see it as a deal breaker.

includes a refactor of the rule api - now receives udef, udefid, uid instead of just udefid
instead of parsing the rules on each button press, now it does it once and stores the rules in a lookup table
@whossname
Copy link
Author

The changes to luamocks/SpringLuaApi.lua are unintentional. It's mostly removing white-space at the end of lines. I must have accidentally run a formatter at some stage.

author = "woss (Tyson Buzza)",
date = "Aug 24, 2024",
license = "Public Domain",
layer = -999999,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why -999999?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No idea. I think I was copying the smart select widget. I don't know what that number does.

Copy link
Contributor

@badosu badosu Oct 11, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can replace it with 1 or 0, the layer is the loading order of the widget in relation to other widgets. We don't expect any conflicts to exist unless a game or user already declares another select action in a widget.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I made the changes but I can't see them in the discussions. Sorry, I don't use pull requests at work. I've always worked at small startups where it's easier to just review the changes with a local git client and discuss the code verbally. I'm probably missing some basic feature of pull requests here.

@WatchTheFort WatchTheFort self-requested a review October 10, 2024 19:46
@badosu
Copy link
Contributor

badosu commented Oct 11, 2024

Just for context, the key addition here is the select api and possibility of replacing engine-level select action with an own lua managed one.

Reason is so that select keys can be customized to extend the select api in the future so the game does not depend on engine for it. If, after inclusion and continued use, we verify that all current engine features are satisfied with expected performance profile - this will be eligible for inclusion as engine basecontent and a proposal for engine select removal in the future.

I don't have any particular opinion on the smart select changes, a very small set of players actually even use the current smart select capabilities anyway, I leave that up to the team members to consider.

I'd be very glad if we could include the select api engine and select widget, even if enabled false - to at some point and basic testing enabling to true, and at some point to actually prove this api is a worthy replacement of the engine select command in basecontent.

Thank you.

@@ -70,6 +70,7 @@ Engine = {
}

---@param path string
--- @return any
function VFS.Include(path)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you revert this file? I understand this file is autogenerated.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reverting this line is easy. I don't know where the other changes even came from. It's a few dozen instances of removing the white space at the end of the line.

Out of curiosity, is there a way of setting sensible return types in that file? I think (because this was 2 months ago at this stage) the IDE was yelling at me a lot for mismatching type specs.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I reverted the white space changes as well.

@WatchTheFort WatchTheFort marked this pull request as draft October 11, 2024 19:46
@WatchTheFort
Copy link
Member

I need to take a deeper look at the code, but from a first glance it looks like it is just duplicating the engine functionality, i.e. implementing a keybind syntax then using string parsing to turn a text file into Lua select commands.

This seems like the wrong approach, as it is sacrificing the flexibility of Lua. Wouldn't this make more sense as a widget, with the caveat that we need better modding support for custom selection keybinds?

@badosu
Copy link
Contributor

badosu commented Oct 11, 2024

Wouldn't this make more sense as a widget...?

Yes, that's what this PR does.

This seems like the wrong approach, as it is sacrificing the flexibility of Lua.

That's the opposite, we're using the flexibility of Lua to have the ability to extend engine functionality. It is duplicate, but the plan is to nuke engine select command anyway, this should be a basecontent widget instead sufficed it works satisfiably.

If the argument is to then, "if we can do anything then we dont need the old engine select constraints", that's a decent argument. But keep in mind, by first supporting base engine select syntax we are able to have this a first step into what will eventually be included in engine basecontent (the testing is priceless for me as an engine developer) and have a painless transition.

If the game deems this not fruitful for game concerns then sure, that's another argument, I just ask to give this a chance so we can improve our engine using this testbed. Proposal is to include at least the select widget and the api, it can come in as enabled false, that's fine as long as I can ask keybind savvy users to try it out in a first iteration of this process.

The smartselect changes I leave it up for the game maintainers to deliberate about.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the purpose of this file?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Separating config and the widget proper lets people change the former without overriding the latter (which would make them miss out on functional improvements)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is it disabled?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To let players do a wider live test before becoming the default

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It separates the functionality from the interface, allowing other widgets to use the functionality. I mostly did it to share common code between smart_select and key_select, but there should be other applications as well.

luaui/Widgets/Include/select_api.lua Outdated Show resolved Hide resolved
luaui/Widgets/Include/select_api.lua Show resolved Hide resolved
luaui/Widgets/Include/select_api.lua Outdated Show resolved Hide resolved
luaui/Widgets/Include/select_api.lua Outdated Show resolved Hide resolved
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

error is being used when an error is detected. Does this really warrant throwing an exception? Better to log an error and remove the widget, or just skip the individual bad filter if possible.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought the first was what this did (based on how it behaved during debugging), but I can see the second is a better option. What is the error logging function for BAR?

local myTeamId = Spring.GetMyTeamID()
return Spring.GetVisibleUnits(myTeamId)
end
elseif sourceDef == "PrevSelection" then
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
elseif sourceDef == "PrevSelection" then
elseif sourceDef == "PreviousSelection" then

luaui/Widgets/Include/select_api.lua Outdated Show resolved Hide resolved
return {}
end
end)
elseif startsWith(sourceDef, "FromMouseC_") then
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
elseif startsWith(sourceDef, "FromMouseC_") then
elseif startsWith(sourceDef, "FromMouseCylinder_") then

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For numeric filters, do they handle non-integers and negative numbers?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They all use this pattern:

			local minRange = tonumber(getNextToken())
			if not minRange then
				break
			end

I'm pretty sure that means it can be a negative float, which doesn't seem like an issue to me. Negative numbers aren't useful, but they don't break the logic. Any non-numbers will just break the parsing.

@whossname
Copy link
Author

I'll have a look this weekend. Could someone please clarify the workflow here? I haven't used PRs much. Am I meant to make the change and then mark the comment as resolved, or just reply to the comment?

- use global `CMD` and `UnitDefNames` lookups
- use `split` string function
- use localized `spGetSelectedUnits`
- handle abbreviations
- remove dead code
- `getAlreadySelectedSet` as a local function
- removed unused parameter from `parseConclusion`
- fix hard coded 50% in `SelectPart_` conclusion
- fix `x` not validated in `FromMouse` conclusion
@whossname whossname marked this pull request as ready for review October 19, 2024 02:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants