• 🏆 Texturing Contest #33 is OPEN! Contestants must re-texture a SD unit model found in-game (Warcraft 3 Classic), recreating the unit into a peaceful NPC version. 🔗Click here to enter!
  • It's time for the first HD Modeling Contest of 2024. Join the theme discussion for Hive's HD Modeling Contest #6! Click here to post your idea!

[General] Pick units in rect bug & Boolexpr leaks

Status
Not open for further replies.
Level 20
Joined
Jul 10, 2009
Messages
479
Hey guys,

I've got 2 questions:
  • Has GroupEnumUnitsInRect(g,r,f) an internal maximum of how many units it can enumerate? I have used it on a rect with 13 units, but only 11 of them were picked. The 2 non-picked units were located at the bottom of the rect and did not have locust. Am I missing something? :p
  • Do conditionfuncs leak, when created from anonymous lua functions? I know that, given a function b taking nothing and returning boolean, you can use Condition(b) as often as you want without creating further leaks. But how about Condition(function() return true end)? I suspect that the anonymous lua function is different on every use and so will the conditionfunc be? Will the conditionfunc be cleaned by Lua's Garbage Collector or should I do it manually?

Thanks for your help and have a nice day,
Eikonium
 
Level 20
Joined
Jul 10, 2009
Messages
479
Rifleman_not_picked.png

Yeah, I knew about the center point being relevant, but that wouldn't explain my observations.
I made the above picture showing the oddness. The right Rifleman doesn't get picked, although his center clearly is within the rect.
Yes, moving him a little bit upwards solves the problem, but I don't understand, why.
Even weirder, I created a second rect directly below this one (i.e. GetRectMaxY(rectBottom)==GetRectMinY(rectTop)) and also picked all units there. The Rifleman is still not showing up, although logically he must be in either of the rects, because there's no space between them.

I attached an example map :)
Typing "a" ingame will pick all units in both rects and show their names. Only one Rifleman will show up.
 

Attachments

  • PickUnits Bug.w3x
    17.5 KB · Views: 16
Level 10
Joined
Jan 13, 2017
Messages
88
Any conditionfunc leaks currently, if its a closure it leaks a LOT, if its not a closure it still leaks but far less.
If you absolutly need to use Filters/Conditions you should wrap everything inside it in "do .. end", otherwise i just reccomend having the condition/filter null and converting the group to an index table (lua array) and discarding the group alltogether using a FirstOfGroup() loop.

There are some more caveats with lua currently and i am semi hard at work trying to figure out how to efficently prevent leakage (thou i primarily use typescript with TypescriptToLua instead of raw lua).
 
Level 20
Joined
Jul 10, 2009
Messages
479
Wow, really useful information, thanks a lot!
Any conditionfunc leaks currently
When you write "currently", are you referring to leaks only occuring, because something is currently not working as intended, which might be solved by a future patch? (I know, betting on a future patch is risky)

Also, as of your current knowledge, wouldn't using DestroyCondition() be a partial solution?
If not, I can absolutely convert a non-filtered group to a lua table and filter there, but that would require rewriting more code.
 
Level 6
Joined
Dec 29, 2019
Messages
82
Wow, really useful information, thanks a lot!

When you write "currently", are you referring to leaks only occuring, because something is currently not working as intended, which might be solved by a future patch? (I know, betting on a future patch is risky)

Also, as of your current knowledge, wouldn't using DestroyCondition() be a partial solution?
If not, I can absolutely convert a non-filtered group to a lua table and filter there, but that would require rewriting more code.
hey man, anonymous functions are kinda mistery, they should definitely be handled by GC as soon as there is no reference left to them, if properly scoped, but nobody knows how exactly wc3 engine cooperates with lua, i guess guys from actizard don't know it either... so we are forced to live in this (try mistake) hell... when it comes to objects like locations, unit groups ect... my advice is to avoid them... in my experiments when there were like 20 locations created every 0.01 seconds it leaked as hell ... even tho they all were properly handled and removed by RemoveLocation... for unit groups i can tell just the same story...

I've changed my approach of handling unit group/location data in my project, locations were replaced fully with X,Y coordinates and unit groups are handled via lua tables, which is pretty simple thanks to super-easy way of hooking natives in lua. Here is some example of what i mean, if you wanna pick units in rect.


Code:
ALL_UNITS = {}

function GetUnitXY(u)
    return GetUnitX(u),GetUnitY(u)
end

function GetRectCoords(r)
    return GetRectMinX(r),GetRectMinY(r),GetRectMaxX(r),GetRectMaxY(r)
end

function IsUnitInRect(u,rect)
    local ux,uy = GetUnitXY(u)
    local rx1,ry1,rx2,ry2 = GetRectCoords(rect)
    return not(ux <= rx1 or uy <= ry1 or ux >= rx2 or uy >= ry2)
end

function RemoveFromArray(value,array)
    for i,v in pairs(array) do
        if v == value then
            return table.remove(array, i)
        end
    end
    return nil
end

-- you need to hook every native which you use for creating units in the map, and if u got some precreated hook BlzCreateUnitWithSkin as well
-- just make sure every unit on map you want to work with somehow is in the global table

oldCreateUnit = CreateUnit
function CreateUnit(id, unitid, x, y, face)
    local u = oldCreateUnit(id, unitid, x, y, face)
    table.insert(ALL_UNITS, u)
    return u
end

oldRemoveUnit = RemoveUnit
function RemoveUnit(whichUnit)
    RemoveFromArray(whichUnit,ALL_UNITS)
    oldRemoveUnit(whichUnit)
end

--depends on your map logic, just make sure u always clear units u wont need anymore in the map from ALL_UNITS table, hooking RemoveUnit native
--also clearing decayed units which can be done for example through catching death event and wait for UNIT_RF_DEATH_TIME + decay_time from gameplay constants until clearing that unit as well
-- in my project i am always removing units manualy when they are not needed anymore so just hooking RemoveUnit will do the trick for me

function GroupUnitsInRect(rect)
    local grp = {}
    for i,u in pairs(ALL_UNITS) do
        if IsUnitInRect(u,rect) then
            table.insert(grp,u)
        end
    end
    return grp
end

MY_RECT = Rect(-15968.0, -16096.0, -12128.0, -13088.0) -- just store your rect here

function printUnitNames()
    for i,u in ipairs(GroupUnitsInRect(MY_RECT)) do
        -- if u need to apply some filter conditions upon group make it here through simple if statements
        print(GetUnitName(u))
    end
end
 
Last edited:
Level 20
Joined
Jul 10, 2009
Messages
479
Perfect! Although it requires more work than I hoped for, your solution seems to fix both the leak issue and the pick-bug I mentioned in the OP.
Thanks for sharing the idea, much appreciated! And good hint on BlzCreateUnitWithSkin(), I would definitely have forgotten that one.

Although having a solution now, I'm still curious on why the Rifleman-not-picked-bug originally occured (see my second post). Is there a known issue I am not aware of?
 
Level 6
Joined
Dec 29, 2019
Messages
82
Okay i've tested your example map and answer is no idea... looks like GetUnitsInRectAll function doesn't work properly. Since by coordinations that rifleman both riflemans are in the rect.

[gg_rct_North] minX,minY,maxX,maxY = -416.0, -128.0, 32.0, 192.0
Rifleman_1 x,y = -347.1, -87.2
Rifleman_2 x,y = -244.6, -80.1

-347.1 > -416.0 AND -347.1 < 32.0 AND -87.2 > -128.0 AND -87.2 < 192.0 = Rifleman_1 is in [gg_rct_North]
-244.6 > -416.0 AND -244.6 < 32.0 AND -80.1 > -128.0 AND -80.1 < 192.0 = Rifleman_2 is in [gg_rct_North]

Conclusion GetUnitsInRectAll Sucks

I've implemented my logic into your example and that works, check attachment.
 

Attachments

  • PickUnits Bug.w3x
    16.7 KB · Views: 17
Level 6
Joined
Dec 29, 2019
Messages
82
I reckon the GetUnitsInRectAll bug came new with Reforged. I've used that function a lot before and never had issues.

As of now, I'm definitely going for the system you presented. Thanks again for sharing and testing! :)
No problem, i've done many researches on memory leaking and WE optimalization using Lua scripting, there was only few resources online and even fewer usable to solve problems in my project, so i know that pain behind it, if i can save someones time then why the hell not.
 
Level 20
Joined
Jul 10, 2009
Messages
479
@Beckx I'm currently implementing your way of handling units into my map.
Units are going to be created via summoning, training and the CreateUnit function.
Am I right that the only way of catching summoned and trained units is via triggers?
In that case, if I have to create a trigger anyway, wouldn't a single "A Unit enters Playable map area" event be the preferable choice over overwriting all natives and catching summoned and trained separately?

Thanks for your help and best regards :)
 
Level 6
Joined
Dec 29, 2019
Messages
82
@Beckx I'm currently implementing your way of handling units into my map.
Units are going to be created via summoning, training and the CreateUnit function.
Am I right that the only way of catching summoned and trained units is via triggers?
In that case, if I have to create a trigger anyway, wouldn't a single "A Unit enters Playable map area" event be the preferable choice over overwriting all natives and catching summoned and trained separately?

Thanks for your help and best regards :)
Hello, i don't use any native abilities in my map... so nothing like summoned units exist there (i don't trust them :X) so if i want to summon units or anything else i just copy paste my custom ability which contains only informations about (casting time/ oprder id/ target type/ range / aoe / manacost / cooldown("actually i don't even use native cooldowns of abilities :X instead created my own system which supports several instances of same ability cooldowns making possible to have stacks on ability usage)) and ability mechanics are always handled by triggers afterwards (i just prefer full control over everything), your solution seems right to me, you can create it that way, so you won't need to hook native functions ... or check for if unit is already in ALL_UNITS before inserting it. You can probably remove hooks on native functions ('CreateUnit' ect... ), but make sure u won't remove BlzCreateUnitWithSkin hook if u are using pre-created unit, or create other way to initialize pre-created units into the table at the start of the map, since they won't be handled by Entering trigger trigger.

Conclusion: Do it however you want :D point is to make sure every unit you want to work with in "unit groups (more like a unit tables now :))" needs to be inserted into your "ALL_UNITS" table otherwise logically it won't be counted while looping through that table.
 
Last edited:
Level 6
Joined
Dec 29, 2019
Messages
82
Btw can you please give me an advice, i've implemented custom timers (inspirated by deadly boss mods/ElvUI from WoW) as u can see on the top right corner next to the Beastmaster Frame, i just feel like it's not best position for them since they should be maybe easier to see for player ? Your primary focus wil go on the center of the screen while playing and when u want to check mechanics times you need to move your eyes up while temporary losing center focus which feels like not ideal solution ... but from aesthetical point of view i guess this looks much better than placing them under that frame or anywhere else ... idk what do u think ?

1616737256375.png
 
Level 20
Joined
Jul 10, 2009
Messages
479
But how to check if the entering unit is the summoned unit?
The plan was to do one of the following to hook new units:
  1. Only use "A unit enters playable map area" event.
  2. Hook all CreateUnit natives and use both "A unit spawns a summoned unit" and "A unit finishes training a unit" events to catch the units not being created via trigger.
I've just been wondering, if the easy choice (option 1) has any disadvantages I don't know about. Especially, because we've just seen bugs with the GroupEnumUnitsInRect native, so I don't trust unit-in-rect natives anymore.

Btw can you please give me an advice, i've implemented custom timers (inspirated by deadly boss mods/ElvUI from WoW) as u can see on the top right corner next to the Beastmaster Frame, i just feel like it's not best position for them since they should be maybe easier to see for player ? Your primary focus wil go on the center of the screen while playing and when u want to check mechanics times you need to move your eyes up while temporary losing center focus which feels like not ideal solution ... but from aesthetical point of view i guess this looks much better than placing them under that frame or anywhere else ... idk what do u think ?
Your customized UI looks great! I do notice once again that I have to get familiar with the UI natives :D
You're right about that the countdown is not in the players main focus. The bottom left corner of the screen would be the preferable choice in my opinion, because it gains more automatic attention, as the remaining UI is also positioned there, so players would not have to switch between bottom and top focus.
Another option would be to let the player choose the position. I personally really like options to customize my game experience.
However you do it, in the end I think it's not going to be a big deal. Because as far as I can see you have implemented another visual indicator displaying the remaining countdown that doesn't even require direct focus to acknowledge - the size-and color changing bars below "Summon Beasts" and "Codo Wave" (good stuff!).
 
Level 14
Joined
Jan 16, 2009
Messages
716
@Eikonium There is a documented case of blizzard making an error by one when adding a rect to a region (at least when using the region for enter/leave events).
The following is pseudo code to correctly add a rect to a region.
Code:
function add_rect(region reg, rect r)
    let min = get_min(r)
    let max = get_max(r)
    resize(r, min, max - v2(32, 32)) 
    RegionAddRect(reg, r)
    resize(r, min, max)

So I thought this bug must be something similar and indeed from my tests it seems to be another error by one but this time on the min point of a rect.
Seems like GetUnitsInRectAll() doesn't operate on the last left and bottom rows of the rect. My theory is that the enumeration goes from top right to bottom left and they made another error by one.

Some pseudo code to fix this issue:
Code:
function enum_units_in_rect(group g, rect r, boolexpr filter)
    let min = get_min(r)
    let max = get_max(r)
    resize(r, min - v2(32, 32), max)
    GroupEnumUnitsInRect(g, r, filter)
    resize(r, min, max)

Here is a picture of your demo map with this applied. As you can see, the second rifleman is picked and the peasants are correctly not picked even though our new rect "should" pick them up.

error_by_one.png
 
Level 20
Joined
Jul 10, 2009
Messages
479
Good insight, thanks! Interesting that you call it "error by one". Does Warcraft internally enumerate units within rects by Y-ranges of 32 each?

I now reported this in the Blizzard bug forums, see here.

I hesitate to implement the fix you described, because Blizzard might actually fix this at some point. If they do, the solution would cause the same bug that it is trying to resolve.
 
Level 39
Joined
Feb 27, 2007
Messages
5,019
If they do, the solution would cause the same bug that it is trying to resolve.
Since the existence of this bug can be checked by trying to find a unit inside a rect that it either explicitly should or shouldn't be able to detect, based on where the unit is, it should be possible for a library to learn if this bug is present in any instance of a map at-runtime.

Then it needs a wrapper for GroupEnumUnitsInRect that checks the rect's current size vs. some information about that rect stored in a hashtable. If the bug is active, resize the rect by +1 in the appropriate dimensions and save information about that in the hashtable so any other time it's called on that rect the wrapper knows that it has already been resized. It could even automatically revert them to their original size after the call. (If the rect's dimensions and the dimensions saved in the hashtable differ by more than +-1 then the rect has been manually resized by something else and needs to be 'fixed' again.)
 
Status
Not open for further replies.
Top