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 GetFocusArmyUnits() #86

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

Conversation

4z0t
Copy link
Member

@4z0t 4z0t commented Sep 18, 2024

This function returns UserUnit array of currentely focused army. Being observer returns all units of all armies. This opens lots of possibilities for observer UI and also fixes some workarounds for UI mods.

Testing

UI_Lua reprsl(GetFocusArmyUnits())
UI_Lua for i=1,1000000 do GetFocusArmyUnits() end
UI_Lua for _,unit in GetFocusArmyUnits() do LOG(unit:GetArmy()) end

Nothing special for testing, just running a command in console. However I'd recommend for someone to test it for leaks because I'm not sure if it really leaks.
Thanks @Hdt80bro for providing definitions for some structures used in this PR.

@4z0t 4z0t requested a review from Hdt80bro September 18, 2024 15:35
@Garanas
Copy link
Member

Garanas commented Sep 19, 2024

Retrieving all the units just to determine how many extractors there are seems a little expensive to me. It may be interesting to add some safe guarding to the function to preserve performance. Especially since this is a UI function - it may run many times each second. My primary concern is about memory, specifically the size of the table that it returns.

We can either solve this in Lua, or we can solve it in the engine. I don't know how difficult these suggestions are.

(1) Would it be possible, for example, to add an argument to pre-filter the list of units? Similar to the brain function aiBrain:GetCurrentUnits or aiBrain:GetListOfUnits where the first argument is the categories that the units must meet. And even better - extend it with the arguments of GetListOfUnits to determine idle units too.

(2) Assuming that (1) succeeds, would it be possible to have a second function that just returns the number of units that meet the predicate instead of the unit instances? Say GetFocusArmyUnitCount. This would prevent the table allocation all together and is often the only information that a user may be interested in.

(3) Separately from (1) and (2), one major issue with the current engine API is that it always allocates a new table. Often this is not required - one can easily re-use a table. But this requires you to be able to pass a table to the engine function to re-use.

Given the frequency that this function may be called, would it be interesting to add an argument at the end that if provided, re-uses the provided table instead of allocating a brand new table? It would fix a lot performance concerns. And it would also make doing (1) and (2) in Lua instead a lot more viable.

@Garanas
Copy link
Member

Garanas commented Sep 19, 2024

And on a separate note - I am not sure whether the current behavior for an observer makes sense. I understand it, but there's no way to filter the units based on their army index except by iterating over the list. It would be better if you could retrieve the units of a specific army.

And before I sound all negative - the idea of this function is very promising and would fix a major hiccup in the UI where we do a 'hack' with the selection to get units that we're interested in.

@4z0t
Copy link
Member Author

4z0t commented Sep 19, 2024

@Garanas yeah, it can be done, I'll look into it a bit later

@4z0t
Copy link
Member Author

4z0t commented Sep 19, 2024

Given the frequency that this function may be called, would it be interesting to add an argument at the end that if provided, re-uses the provided table instead of allocating a brand new table? It would fix a lot performance concerns. And it would also make doing (1) and (2) in Lua instead a lot more viable.

There is no point in such complexity. Just make a wrapper in lua that checks for tick to be updated.

@Garanas
Copy link
Member

Garanas commented Sep 19, 2024

Given the frequency that this function may be called, would it be interesting to add an argument at the end that if provided, re-uses the provided table instead of allocating a brand new table? It would fix a lot performance concerns. And it would also make doing (1) and (2) in Lua instead a lot more viable.

There is no point in such complexity. Just make a wrapper in lua that checks for tick to be updated.

That means every call would share the same table instance. Which is volatile - if some UI mod changes the table directly then you get all sorts of weird behavior that is difficult to debug. Either the source supports it, or we won't have it in my opinion.

It's fine to not have it - it is just a suggestion.

@4z0t 4z0t marked this pull request as draft September 19, 2024 21:19
@4z0t
Copy link
Member Author

4z0t commented Sep 21, 2024

Continuation of this PR requires a new patcher. Should I close this one then?

@Garanas
Copy link
Member

Garanas commented Sep 23, 2024

Let it open for now - until we have a new pull request to refer to.

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.

2 participants