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

Add initial filtering implementations #4905

Draft
wants to merge 4 commits into
base: develop
Choose a base branch
from

Conversation

Garanas
Copy link
Member

@Garanas Garanas commented May 7, 2023

Closes #4892 and #4891

@Garanas Garanas added this to the Development iteration III milestone Jun 11, 2023
@Garanas Garanas linked an issue Jun 11, 2023 that may be closed by this pull request
@Garanas Garanas changed the base branch from deploy/fafdevelop to develop June 1, 2024 19:37
@MrRowey MrRowey requested review from clyfordv and lL1l1 June 20, 2024 21:18
@MrRowey MrRowey added feature: selection behavior related to selecting units in some fashion type: testing needed labels Jun 20, 2024
Copy link
Contributor

@lL1l1 lL1l1 left a comment

Choose a reason for hiding this comment

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

Since it's not written here, according to FAForever/FA-Binary-Patches#30, this is on hold (is in draft form) because "It would likely break any UI mod that relies on how the selection works."

The PR format could be improved by splitting it up between implementing various filters and the actual new filtering. Though my biggest pain was that moving the entire OnSelectionChanged function around caused a giant diff with for just a few changes.

unitCount = table.getn(units)

if unitCount > 0 then
return units, unitCount
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be better to include filtered down hover + navy in this part instead of just navy.

ignoreFiltering = false

else
-- do not ignore selections when holding down shift
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
-- do not ignore selections when holding down shift
-- do not filter selections when holding down shift

local selection, count = newSelection, originalCount
selection, count = DeselectLockedOut(selection, count)

if PrefsDeselectByLayer ~= 'off' then
Copy link
Contributor

Choose a reason for hiding this comment

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

Could be simplified to if PrefsDeselectByLayer then by using false for "off" in the option keys.

return
end

-- allows us to ignore the deprioritzation for this selection
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
-- allows us to ignore the deprioritzation for this selection
-- the selection has already been filtered

Comment on lines +1234 to +1239
ForkThread(
function()
IgnoreFiltering()
SelectUnits(selection)
end
)
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems unclear to me why SelectUnits has to be forked.

-- Deselect Selens if necessary. Also do work on Hotbuild labels
local changed = false -- Prevent recursion
if newSelection and not table.empty(newSelection) then
newSelection, changed = DeselectSelens(newSelection)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
newSelection, changed = DeselectSelens(newSelection)
-- Function frequently used by mods to adjust selection
newSelection, changed = DeselectSelens(newSelection)

Copy link
Contributor

Choose a reason for hiding this comment

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

What was this going to be used for compared to observable? I see the difference is that it gets a value upon initialization, and that it uses the class system instead of metatables.

Comment on lines +1075 to +1082
local head = 1
for k = 1, count do
local userUnit = selection[k]
if not userUnit.LockedOutOfSelection then
selection[head] = userUnit
head = head + 1
end
end
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess all of this is to preserve the table as an array? Simply doing if locked then t[i] = nil end return t, table.getn(t) leaves holes and requires a table.getn call.

@MrRowey MrRowey added the area: requires engine patch related to the engine and no effective workaround is known in Lua label Jun 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: requires engine patch related to the engine and no effective workaround is known in Lua feature: selection behavior related to selecting units in some fashion type: testing needed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cybran Land Scout Deselection Scathis Deselection
3 participants