[Snippet] UnitLL

[Troll-Brain]

Looks nice for iteration ; ).

Beyond the extra features added over what groups actually offers, that's pretty much the point of it.
Now, this is totally useless with instant enumerated groups which don't need to be kept during time.

But for personal stuff and/or if your snippet already requires it, you still could use the group spelled GROUP for instant enumerations.
Btw i don't really know if i should change the name of its group variable, maybe it's too generic ?!
I like it, but ...

 

[Nestharus]

What I am thinking for the group var is that you should make another library that encapsulates it, like ConstantGroup or something ^)^. It could have various enum methods as well as for/clear/first of/remove. It seems like a lot, but it just appears to me like it should be encapsulated ;p. It being kind of out there just sort of sets off alarms, hehehe ; ).


Furthermore, people could then use ConstantGroup whenever they needed a temp group for enums : ). I know the thing has been contested in the past, but if you properly encapsulate it and so on and make this lib use it, who knows o-o. Ofc, I personally just create/destroy groups nowadays ^)^, but it's probably better to have a single constant group instead. I started the habit with timers ;p. I figured timer recycling wasn't really worth it due to the memory overhead and sometimes strange behavior of paused timers (behavior thing quoted from TKoK).

 

[Troll-Brain]

What I am thinking for the group var is that you should make another library that encapsulates it, like ConstantGroup or something ^)^.

Huge no, one group more or less doesn't have any matter, while one requirement just for that is lame.
I'm just not sure if i should kept the name "GROUP", as it is a real common one, i suppose i should not ...

It could have various enum methods as well as for/clear/first of/remove. It seems like a lot, but it just appears to me like it should be encapsulated ;p.

If it was a module, maybe that would make sense, but it is a struct.
More methods wouldn't hurt, even for the script size.
I could add these methods, i just don't have a concrete usage of it in my mind (and not just a theoric one)

It being kind of out there just sort of sets off alarms, hehehe ; ).

I don't get what you want mean.
Maybe it's because i translate it word by word.

Furthermore, people could then use ConstantGroup whenever they needed a temp group for enums : ). I know the thing has been contested in the past, but if you properly encapsulate it and so on and make this lib use it, who knows o-o.

As already said, one group more or less doesn't matter in jass, it's not like we are lacking of memory.
So a requirement, just for that, no.

Ofc, I personally just create/destroy groups nowadays ^)^

Why ?

, but it's probably better to have a single constant group instead.

For instant enumerations it makes sense.

I started the habit with timers ;p. I figured timer recycling wasn't really worth it due to the memory overhead and sometimes strange behavior of paused timers (behavior thing quoted from TKoK).

Again memory usage is not a problem in jass.
What sort of strange behavior ? Link plz.
So far, i thought that destroying not paused timer could still make the timer expiring (never tried if this bug was fixed), and to link a data and a timer i don't see a best way than timer recycling.

 

[Nestharus]

What sort of strange behavior ? Link plz.
So far, i thought that destroying not paused timer could still make the timer expiring (never tried if this bug was fixed), and to link a data and a timer i don't see a best way than timer recycling.

It was a discussion in a chatroom with the devs of TKoK. They said that when they paused a timer and later started it up again, it would use its previous remaining timeout rather than the new assigned timeout. I have never encountered it. They have, so I am always careful of using paused timers now.

I don't get what you want mean.
Maybe it's because i translate it word by word.

Oh, the alarms were because it isn't currently encapsulated ; P. Giving direct access to the group gives access to things like DestroyGroup ^)^. Yea, it's silly, but encapsulation can really help map dev ;p. It forces you to do things the right way (finding workarounds to your own encapsulation to keep up good design) rather than getting lazy and going for coupling or crazy weirdness ;D. I spent 2 hours trying to solve a problem that came from the very way I designed my map : P. It would have been easy to solve if I had just coupled it together with something else, but I was determined to keep everything separate ^)^.


The whole separate group lib was just a thought if you ended up adding too many features for encapsulating that group into ur lib. After all, if someone just wanted to use that group and not all of the linked list stuff, then it makes sense to keep the group sep as another resource.


Another thought... why not just keep the group private? After all, the group isn't really part of the rest of the lib, it is sort of an extra, and we all know that extras are bad ;D. Yea, you might be thinking that you might as well keep it public, but then that means that your library does Unit Linked Lists + A Constant Group. See that +? In the interest of good design, it'd probably be best to keep it private, even if there ended up being more overhead because of the need to declare more groups in other resources.

I hope that makes sense ; ).

 

[Troll-Brain]

The bug involved can be used as a way to attach data to a timer.
However, even if it's the fastest way (im not saying it is, i consider it as a really bad way, as you would expect to get the real period time of the timer)

I also honestly don't think there is a bug, using correctly TimerUtils, unless you are not starting a timer, pause it and restart it again with no delay (for wtf reason ?!)
If the bug was more general, i think it would be a while since someone had reported it for TimerUtils.

It was a discussion in a chatroom with the devs of TKoK. They said that when they paused a timer and later started it up again, it would use its previous remaining timeout rather than the new assigned timeout. I have never encountered it. They have, so I am always careful of using paused timers now.

I guess they made the error i've described, don't take me wrong this is still a bug, but their code was already bugged, in the sense it doesn't have any logic.
Now, if my guess is wrong and you have a real code, share it plz.

Oh, the alarms were because it isn't currently encapsulated ; P. Giving direct access to the group gives access to things like DestroyGroup ^)^.

That's pretty much why GROUP follows the jass constant convention.

Yea, it's silly, but encapsulation can really help map dev ;p. It forces you to do things the right way (finding workarounds to your own encapsulation to keep up good design) rather than getting lazy and going for coupling or crazy weirdness ;D.

Yes, this is silly, you can't always protect the user for himself, really here if he can't figure that GROUP is supposed to be never destroyed ...

I spent 2 hours trying to solve a problem that came from the very way I designed my map : P. It would have been easy to solve if I had just coupled it together with something else, but I was determined to keep everything separate ^)^.

I also honestly think you are wasting many more time in debug, by using custom struct allocators and such.

The whole separate group lib was just a thought if you ended up adding too many features for encapsulating that group into ur lib. After all, if someone just wanted to use that group and not all of the linked list stuff, then it makes sense to keep the group sep as another resource.

I'm against a library just for that, i've already explained why.

Another thought... why not just keep the group private? After all, the group isn't really part of the rest of the lib, it is sort of an extra, and we all know that extras are bad ;D.

Keep it private is a possibility, not because this extra is lame, but i really don't know how to spell him.

Yea, you might be thinking that you might as well keep it public, but then that means that your library does Unit Linked Lists + A Constant Group.
See that +? In the interest of good design, it'd probably be best to keep it private, even if there ended up being more overhead because of the need to declare more groups in other resources.

It's like you are bothering to add one dollar more to one million, i mean the group overhead is just negligible.
Now it would be lame if the user requires this library just for the group GROUP instead of using its own in a public resource.
But i think that moderators will notice it anyway, so that's not a real problem.

I hope that makes sense ; ).

Most of parts.

 

[Bribe]

That's a crazy trick, I wonder why no one has been doing that? It would be slower for setting timer data but I'd say faster for getting the timer data, but way more hacky and I like that.

 

[Troll-Brain]

Because, really, get the data is not a speed issue in 99.99 % of cases.
Also, what if you use a code which uses the PERIOD of the timer within the timer callback ?
I mean even if i don't use that myself, there is nothing wrong with it.

Even if i love to find bugs and how to exploit them, it's just an hobby, i don't seriously think about use them.
Sometimes bugs are fixed, hell, remember the old return bug, now ofc it's not likely to happen, but there is still a chance, then you map will be broken.
Since we have already other valid ways i've no use of it, not to mention that a benchmark is needed, for example R2I could be slower than expected.

Now i suppose it's find to use it in personal scripts, but it is not in a public resource.

 

[Troll-Brain]

v 1.1

- fixed a bug with the method addGroup : If you used the safety mode it returned 0 instead of the number of units added.

- use the new for loop of Cohadar's jasshelper version, ( the FirstOfGroup/GroupRemoveUnit loop )

- use a decent initializer now, instead of a module one, since the vJass init order makes sense now.

- added the methods removeGroup and removeUnitLL for completness reason.

 

[Bribe]

I like Cohadar's JassHelper and this is gonna get approved now.

 

[Troll-Brain]

I like it too.

 

[Troll-Brain]

I've a problem.

I was so concentrated on highlighting the features than i've forgotten one big thing, related to unit removing.
Actually, if you need to fire code related to an UnitLL on unit deindexing event, you can't, i mean the code will fire but the unit will be already removed of the UnitLL.

Now, i could "hijack" the Event library (without any hook involved, nor editing, just playing with GetTriggeringTrigger() and the Event API).
Indeed i could simply add an action to the deindex trigger, then users would be allowed to simply use UnitIndex.DEINDEX as usual, but it requires to make conditions returns true, else my action will never fire.
Also, playing with other libraries outside the bounds is a terrible practice.

I could also simply use a Timer(0) on deindex event, but there is a minor chance then that you could enum a ghost unit, even if it's very unlikely.

So it seems that i've only one solution, add a method/function, the user will have to use it instead of the classic deindex API for an UnitLL.
But i'm not sure about the name of the method.

registerDeindex ?

 

[Troll-Brain]

v 2.0

- made all the debug stuff only, no more only on debug mode, except of course for debug messages.
Just because really, comparing to the rest of the code it doesn't matter at all.
And just in case you worry about it you can still edit the code for yourself.
Now, there are execptions for the methods isUnitInside and removeThis, because then they can be inlined.

- renamed the private static method onDeindex

- added a method onDeindex (check the first post)

- added a static method getDeindexing (check the first post)

 

[Bribe]

The "onRemove" method should account for recursion imo.

            //I recommend this change, since a new trigger will have no conditions:
            if this.trig_deindex == null then
                set this.trig_deindex = CreateTrigger()
            @else@
                call TriggerClearConditions(this.trig_deindex)
            endif

 

[Troll-Brain]

Yeah i could add this check, but last time i checked calling TriggerClearConditions/TriggerEvaluate was fine on a "null" trigger, so it would just add size to the script (and it is nothing related to speedfreak).
Now, if you consider it as a bad practice i don't mind to add it.
Afaik there are only really few (or even none at all) native functions that will cause a malfunction with a null argument.

The "onRemove" method should account for recursion imo.

I don't get what you want mean.

Also, i'm not sure if i should destroy the eventual trigger when an UnitLL head is destroyed, or keep it like it is actually.
I think it would be better to destroy it.

 

[Bribe]

Well if a unit gets removed from a function which gets called, there will be an issue with recursion with the setup you got here. It would just be to add this highlighted stuff:

        private static method onRemove takes nothing returns boolean
            local LL this = LL(LL_main[GetIndexedUnitId()])
            local LL s
            @local thistype cachedThis = thistype.This@
            if this == 0 then
                return false
            endif
            set s = this.next
            loop
            exitwhen s == this
                set thistype.This = s.x.head
                call TriggerEvaluate(s.x.head.trig_deindex)
                //set thistype.This = 0
                call s.x.removeThis()
            set s = s.next
            endloop
            @set thistype.This = cachedThis@
            return false
        endmethod

Obviously you need to set "static thistype This = 0" from its declaration to avoid crashing threads.

TriggerClearConditions on a null trigger is fine, but I am a big fan of logic and readability so the "else" statements will let people know why you are clearing conditions from the trigger in the first place.

 

[Troll-Brain]

The only drawback i see is that in case that you remove the deindexing unit inside a code registred with my method onDeindex :
s.x.removeThis() will be called for no reason inside the loop (i hardly can avoid that), but won't malfunction.

Now i'm thinking about it, getDeindexing can and will return an incorrect value if you use before add/removeGroup with an other UnitLL inside the registred code (very unlikely but theoritical cases are so fun ).

I used This, just because i already used it before as a temp variable for add/removeGroup (to save one variable), but to prevent this possible bug it's just better to use one variable reserved to the method.

Thx for pointing that, even if that was not exactly that ^^

Ok for the else.

And i will also destroy trigger, because it's not obvious that an UnitLL head instance once destroyed will become again an UnitLL head.
I don't feel it's good to keep these potential useless triggers.

 

[Troll-Brain]

v 2.1

- improved the debug stuff, there is again more stuff only on debug mode, but i've managed to make it work as expected even in not debug mode.
Basically, the debug mode will "just" help the user to find errors in his code by displaying text messages
I also didn't handle some error cases in the previous versions, like the not indexed units and if you used a 0 UnitLL instance, now all cases should be handled.

- getUnit() method added, note that in not debug mode it inline to the readonly member enum

- deprecated the readonly member unit "enum", it still can be used but i recommend to use the method getUnit() instead.

- fixed an unlikely bug described in the post above.

- 2 debug messages were displayed even in not debug mode (in case the user made an error with his script), now it's fixed, all messages are displayed only on debug mode.

- now triggers created internally are destroyed when they are no longer needed instead of got "recycled" (check the post above if you want more details)

 

[Troll-Brain]

Well, for the onDeindex mehod, that would be far better to use boolean functions which return true, then i could use the ForceEnumPlayerCounted trick instead of dynamic triggers.

But if you used return false in such code, you will have to edit to return true, else the event will fire as many times there are players in the map (including computers).
Same, if you make a thread crash or reach the limit op in the evaluated code before the return, it will fire as many times, since it will be considered as false.

This version is quite safety made, i hesitate to switch to this.

If there are users here, plz tell me your opinion about that.

 

[Bribe]

If only Blizzard made their own debug-mode which printed errors on thread crashes.

I am OK with ForceEnumPlayersCounted. The CreateTrigger/TriggerAddCondition/TriggerEvaluate syntax is the worst for me.

 

[Troll-Brain]

That's indeed much cleaner, but i have also an alternative, recycle triggers properly (own stack).
That wouldn't matter that much in both speed and memory usage.
This way i ensure the user won't screw it with a false boolean return, because in jass return false is commonly used in these cases.
You perfectly know it ^^

 

[Bribe]

Well having a clearly-visible disclaimer stating "the function must return true" is enough. People write "return false" out of habit, not because it looks cool.

 

[Troll-Brain]

That's because it is an habbit, that i hesitate.
I mean it's error prone.

 

[Bribe]

It's up to you. I will be using it whenever I am presented with the opportunity, my idea is that people should get used to it. If a map maker will not pay attention to

a disclaimer like this,

I can imagine his/her map will be much more flawed than this simple error.

 

[Troll-Brain]

We only have grey and green colors for comments

But oh well, the ForceEnumPlayerCounted makes the code much less sphaghetti.
And anyway even with conditions used as code, it's fine to use return true.
That even makes more sense, even if there is this strange behavior, i mean even if the previous condition return false, all conditions are still evaluated, while conditons are "and", not "or".

So i will wait few days, if nobody disagree (most likely) i will use it with a disclaimer.

EDIT :

Hmm, you won't be allowed to use functions that returns nothing anymore ...
That's kinda lame, or at least annoying.
I think i will just leave as it, because i'm quite sure (not tested) that trigger recycling would be slower than just create/destroy anyway (because jass is interpreted)

 

[Troll-Brain]

v 2.2

Because everyone wanted it, i've added a new method : getRandomUnit
The name of the method should be enough clear by itself.
At worst the complexity of it will be O(N/2), where N is the number of the units inside the UnitLL.

 

[Troll-Brain]

Well, this new method actually doesn't work, because i've written it like an UnitLL is a double linked list, while it's not the case.
Now, on a second though i think it's better if it would be a circular one, because using a such loop is not that bad.

local UnitLL this = ...

loop
set this = this.next
exitwhen this == this.head
    // your code here
endloop

Comparing to the former one :

local UnitLL this = ...

loop
set this = this.next
exitwhen this == 0
    // your code here
endloop

You can even use a variable if you want to win some "performance", and anyway in the first place if speed is such an issue you wouldn't use it in the very first place, that was never the main purpose of it.

The reason why a double linked list is better is because you have more possibilities like constantly looping through an UnitLL, of course you still can in the current state but it needs more code to do it.
It also reduces the code inside the methods of the UnitLL struct.
Now if i do that it will break the backward compatibility, but apparently nobody uses it, so it shouldn't be an issue, plus the fix is enough easy imho.

Also i've realized that there is a major bug with the onDeindex method, actually it doesn't register the code, this was probably introduced before the version 2.1 by a failed code optimization (i've checked only from this version and not older ones since i didn't had them on my computer while i was checking).

I attach the current unofficial version where this bug is fixed, i've also begin to make UnitLL circular, but the custom spell "LifeDrain" in the demo map doesn't work, it should be a problem with the method addUnit and maybe or/and with the custom spell by itself.
I suppose other methods also need a fix.

Frankly i've quite lost the motivation to fix it right now, i think i will take a break for this summer, so it should be graveyarded until i will come with a fixed version.

 

[Magtheridon96]

Graveyarded by request. (Temporarily)

 

[Magtheridon96]

Recovered by request. (Incoming update)

 

[Troll-Brain]

I was out of my mind when i thought about making UnitLL as a circular linked list, i've thought about all possible implementations, they are all weird and less useful, more verbose, than the current not circular one.
Plus, in almost situations, you need to loop through the loop only one time, and anyway if you need to constantly loop through an UnitLL you can still do it.

So here is the new fixed version, backward compatibility is not broken, UnitLL has still the same implementation than the previous versions, a double not circular linked list.

v 2.25

* fixed the bug with the onDeindex method, the code given in argument was not registred.

* fixed the bug with the method getRandomUnit, it works properly now.

* added a boolean struct member end, it is meant to be used with an exitwhen loop, check the samples.
I've not added it to "win" some performance on an UnitLL iteration, just to be a less verbose alternative than the previous one (exitwhen UnitLLinstance == 0)
Now, if you still prefer to use (exitwhen UnitLLinstance == 0), you don't have to use it, it's just the same.

* improved the demo map in the sense that i've added more samples for the UnitLL api usage.

 

[Troll-Brain]

Ok, since UnitIndexer has been deleted i suppose it should be graveyarded.

 

[Nestharus]

UnitIndexer hasn't been deleted, it's being moved elsewhere. Give me a while >.<. I need to reform the code and do some nice docs -.-. I'm also doing an interesting Lua thing .

 

[Troll-Brain]

So it will be re-submitted there ?

 

[Nestharus]

Yea, for now, I'm working on a huge new way to manage resources through Lua and stuff. I'll be reforming a lot of stuff to go through it . Big changes, but they are good changes.

Don't worry, I wouldn't take down 10 years of work -.-. This'll just be a huge transition into a new way of managing and distributing this stuff ^_^.

edit
look in my sig, temp solution

 

[Troll-Brain]

Ok, you can graveyard this, i don't want to bother with Nestharus, and anyway according to the 0 feedbacks it doesn't seem to be used, so RIP.

 

[Nestharus]

I'll put a backwards compatibility thing up :\

 

[PurgeandFire]

Graveyarding by request