[System] Unit Indexer

[Troll-Brain]

You can't use a (v)jass constant in an object merger call ?
I think we could, i'm wrong ?

I was also pretty sure that object merger had options to overwrite or not an existing object (and creating a new one if necessary).

But i haven't really played with it so i could be wrong.

 

[Nestharus]

You can't use a vjass constant

And it always overwrites

 

[Troll-Brain]

I've just read the grimex documemtation (html included in the JNGP) :

To merge object data there are three different modes available how collisions can be handled. Each of them is available from the menu as well as from vJASS external calls.

* m merge changes for objects with the same rawcode, this is the default (Extensions\Merge Object Editor Data)
* r replace existing objects with the imported objects in case of equal rawcodes (Extensions\Replace Object Editor Data)
* i a clean import, that generates new ids for imported objects if their id is already used (Extensions\Import Object Editor Data)

For constants i suppose it's because jasshelper process macro and external commands before constants.

According to the documentation it should always overwrite by default but not if you are using the right option (i).

Now i'm not aware if these options are working properly or not.

 

[Nestharus]

Those options can't be set from the call. It would depend on the map user having it set up correctly.

 

[Troll-Brain]

Hmm the documentation say it's possible but the samples don't work.
Ever asked it to Pitzermike ?

 

[Nestharus]

Pitzermike is MIA

 

[Troll-Brain]

You mean missing in action ?
But, i'm wondering if it's not jasshelper fault, even if Vexorian is also MIA.

Anyway again you are exaggerating a problem which is not likely to happen, and even if the user already uses this custom rawcode (for wtf reason ?, if it happens he shouldn't play to lucky games ...) that won't be the end of the world.
Just make sure you're using an obscure rawcode like 'N%_"'.
Hell, if you care so much about it you can comment the objectmerger call and add a description above it.

Let's make it even more friendly to use (at least for tests since an unit indexer is almost always used when we deal about units)
What about making WorldBounds optional ?
The few lines of vJass code needed are worth it against a library requirement.
In fact this kind of library (WorldBounds) should be already a part of a jass preprocessor but that's an other story.

In more general these kind of library are good as a personal use but for public resources their requirements are lame when they can be reasonably inlined.

 

[Magtheridon96]

MIA = Mostly Inactive (I think)

edit
WorldBounds isn't so bad. I'm using it in a spell of mine and no one's complaining.

 

[Troll-Brain]

MIA = Mostly Inactive (I think)

edit
WorldBounds isn't so bad. I'm using it in a spell of mine and no one's complaining.

It's just because no one is using it

Missing in action was the first result in my language using goo...

Seriously a spell is not a test, unless you want to test the spell ...
I've already stated that an unit indexer is really used, even for quick tests, and Nestharus agreed with this fact.

For me WorldBounds is just a lil better than the dead boolexpr "utils" library, in the sense that the requirement is worse than the profit.

 

[Nestharus]

For me WorldBounds is just a lil better than the dead boolexpr "utils" library, in the sense that the requirement is worse than the profit.

For me, it's better to use it than not to use it as it cuts some of the code I have to write >.>. It also reduces the number of handles that go through create/destroy.


And for the record, MIA = Missing in Action. It's a military term.

 

[Troll-Brain]

For me, it's better to use it than not to use it as it cuts some of the code I have to write >.>. It also reduces the number of handles that go through create/destroy.


And for the record, MIA = Missing in Action. It's a military term.

The number of handles really doesn't matter, or else just don't use vJass at all. (even with struct arrays and such)

A code is more often read than write, sure it's more annoying to you but more friendly for other users.
There is a reason why such common libraries are not submitted.

 

[Nestharus]

A code is more often read than write, sure it's more annoying to you but more friendly for other users.
There is a reason why such common libraries are not submitted.

I respond with, "identical code should be abstracted" ;p.

 

[Troll-Brain]

And if i need to make a short test where i don't even use regions ?

 

[Nestharus]

Do you want me to add a boolean that makes the region/rect optional?

 

[Troll-Brain]

No, an optional library requirement :

library UnitIndexer uses Event, optional WorldBounds

 

[Magtheridon96]

WorldBounds has no cons at all..
-26 lines of code (18 lines after compiling)
-Reduces the amount of code to write when dealing with Map Boundaries

I don't see why it should be a problem for anyone to implement it.

 

[Troll-Brain]

That's just one more requirement, i think i've clearly said why, i can't argue more ...

 

[Magtheridon96]

Having more requirements isn't a bad thing.

 

[Troll-Brain]

Having more requirements isn't a bad thing.

When you can reasonably avoid them, it is.

Again, think about a quick test.
That will be my last word on it, i'm pretty sure Nestharus won't change his mind about it anyway and i don't expect any approval from other users, but i've done all i could.

 

[Magtheridon96]

I get what you mean bro, but since this only has 2 requirements, it shouldn't take that much time to implement it into a small testmap

 

[Troll-Brain]

Sure, but one requirement is better than 2 (meh that wasn't my last words about it ...)

 

[Bribe]

I definitely prefer the original API of OnUnitIndexed/Deindexed and it also gets rid of the requirement for the Event library.

Nestharus complains when someone makes a resource that actually takes advantage of Event's useful functions like dynamic registry and event data, so might as well go back to basic boolexpr evaluations.

The Init should check if LIBRARY_WorldBounds is found and if not create its own rect/region and of course remove them right afterward. That way you get the best of both worlds.

 

[Magtheridon96]

He can't change the API now, countless resources on the Hive and TheHelper would have to be updated :/

 

[Bribe]

Does anyone on TheHelper use Nestharus' UnitIndexer? I haven't seen anything there.

Either way, it wouldn't be hard to add backwards compatibility just to make the actual API cleaner.

 

[Magtheridon96]

Yeah, you're right

 

[Troll-Brain]

I definitely prefer the original API of OnUnitIndexed/Deindexed and it also gets rid of the requirement for the Event library.

Nestharus complains when someone makes a resource that actually takes advantage of Event's useful functions like dynamic registry and event data, so might as well go back to basic boolexpr evaluations.

The Init should check if LIBRARY_WorldBounds is found and if not create its own rect/region and of course remove them right afterward. That way you get the best of both worlds.

+1

I didn't take in consideration the event library because Nestharus is such a pain in the ass when we argue about (his) resources, one point at a time is the way to go with him.
But sure 0 requirement is much better.

 

[baassee]

TheHelper is obsessed with AIDS...

 

[Nestharus]

I actually think the API is cleaner as it is right now using Event. It also follow more closely to JASS convention-

RegisterUnitIndexEvent(thingToBeRegistered, event)

; )


Also I wrote WorldBounds because I was tired of retrieving the extrema coords and doing all of the rect crap. I don't get why you guys dislike more requirements >.>.


More requirements = more modularity = better coding practices.


Furthermore there are links to the requirements... it's not that much effort to click on a link and cnp >.>.


You should know by now that I don't code all in one packages and that I am thoroughly against all in one packages ; P. If a lib has something that can be abstracted to another library, then I'm all for abstracting it, even if it is something as simple as retrieving the world extrema.

 

[Troll-Brain]

Just because we are lazy ass.

And the inlining of these libraries doesn't really matter in the resulted jass code (at very least WorldBounds), and since a code is more often used/read than written it's always better to privilege the user against the writer (if it's reasonable and here it is).

Also, have i already mentioned quick tests ?

 

[Bribe]

AIDS backwards compatibility:

library AIDS uses UnitIndexer
    
    function GetIndexUnit takes integer id returns unit
        return GetUnitById(id)
    endfunction
    
    // A delegate struct is needed because a lot of scripts have the
    //textmacro at the top of the struct.    
    private struct Dummy extends array
        static method AIDS_filter takes unit u returns boolean
            return true
        endmethod
        static method AIDS_onInit takes nothing returns nothing
        endmethod
        method AIDS_onCreate takes nothing returns nothing
        endmethod
        method AIDS_onDestroy takes nothing returns nothing
        endmethod
    endstruct
    
    module AIDS
        private static delegate Dummy d = 0
        
        method AIDS_addLock takes nothing returns nothing
            call UnitIndex(this).lock()
        endmethod
        
        method AIDS_removeLock takes nothing returns nothing
            call UnitIndex(this).unlock()
        endmethod
        
        private static method filter takes unit u returns boolean
            return thistype.AIDS_filter(u)
        endmethod
        
        private method index takes nothing returns nothing
            call this.AIDS_onCreate()
        endmethod
        
        private method deindex takes nothing returns nothing
            call this.AIDS_onDestroy()
        endmethod
        
        implement UnitIndexStruct
    endmodule
    
    //! textmacro AIDS
        private static method onInit takes nothing returns nothing
            call thistype.AIDS_onInit()
        endmethod
        
        implement AIDS
    //! endtextmacro
    
endlibrary

 

[jim7777]

Bribe is expert at making Backwards Compatibility scripts eh?
this will be useful >

 

[Magtheridon96]

Yup, Good Job Bribe : )

 

[jim7777]

ohh wait

static method AIDS_filter takes unit u returns boolean
says that it should return something
/thanks to jasshelper

 

[Nestharus]

Bribe is expert at making Backwards Compatibility scripts eh?
this will be useful >

UnitIndexer originally had scripts for AIDS and AutoIndex, but I deleted them ; P

 

[Bribe]

ohh wait

static method AIDS_filter takes unit u returns boolean
says that it should return something
/thanks to jasshelper

Thanks for the spot. I freehanded that when I was here at work between calls.

Fixed: http://www.hiveworkshop.com/forums/2010615-post180.html

 

[baassee]

Great work Bribe, finally I don't have to use my manipulated AIDS version... which still sucks more than UnitIndexer but better than normal AIDS.

 

[jim7777]

working now!! thanks

 

[Bribe]

Rather than making a boolean to enable/disable the indexer, you could
make it a method operator (to keep backwards compat) and do this:

static method operator enabled= takes boolean flag returns nothing
    if flag then
        call EnableTrigger(trig)
    else
        call DisableTrigger(trig)
    endif
endmethod

Of course the trigger here would be the one you assign to the enters region event.

Also, when removing a lock, I know it's rare but what if the final lock is removed from the same thread it was deindexed in? The unit won't be null it will just be missing its ability. So I suppose you could change the check to make sure it's not null into checking if it has the ability or not.

Adding back in the overall system filter and the IndexUnit function would also be a good comeback. Then you could also mirror every feature of AIDS.

 

[Nestharus]

The big filter was removed as the UnitIndexer.enabled should always be used.

IndexUnit was removed as it is a useless function ; P.


Also, I have been meaning to change the boolean to a method operator, I just haven't gotten around to it ;D.


For the idea on final locks, that's good =).

 

[Bribe]

The filter was the only way to keep certain naturally-occurring units like illusions, for example, from being indexed.

IndexUnit would be used in that case to assign such a unit an index. It's usefulness is obviously arguable but apparently Romek convinced Jesus4Lyf long ago that this is useful, so I am not sure if there is some hidden meaning behind it.

The filter has pretty much no overhead so it's good to have it for completeness.

 

[Nestharus]

I suppose, but I still don't see the point of IndexUnit. If a unit isn't indexed at the start, then it is like it doesn't exist. If you ever have a need for the unit to be indexed, then you should always have it indexed at the start. IndexUnit is a bad practice, thus it will not be implemented at all. You'd have checks to see if the unit was already indexed and etc, it just turns into a mess.


However, the filter idea I can see now -> if (not IsUnitType(whichUnit, UNIT_TYPE_SUMMONED)) then.


The only problem with filtering out any unit is effects. Effects that depend on a unit type id are still supposed to effect summoned units. If they are supposed to effect summoned units in a different way, then IsUnitType is used. This means that any unit created naturally should be indexed >.>. The only units that shouldn't be indexed are temporary units, hence why there is the UnitIndexed.enabled thing.


If you have a regular unit that is not indexed, then you are just asking for problems in your map.

 

[Bribe]

I have done some thinking and found it is easier to filter all units of a type, and then on the relatively more rare occasion where you need a dummy like that to be indexed you'd use IndexUnit.

It would help with compatibility as well, for example with many spells that don't have "static if UnitIndexer then disable unit indexer, etc."

 

[Nestharus]

The thing I don't like about your idea is that the user then has to configure the system to work with the other systems rather than configuring the other systems to take this system into account.

When a user has to make like a unit indexer work with a myriad of spells, that just becomes a real mess. The spells should be modified to work with Unit Indexer. This was actually the initial reasoning I had when I decided to take the global filter out.


I took IndexUnit out because it was just extra overhead. There is never an occasion where you will index a unit that was previously not indexed. If this occasion arises, you designed something very poorly.


It's all about easier managing. It's easier to account for the various systems inside of the various systems, not inside of some other third party system.

 

[Bribe]

Maybe then it just gives the user a false sense of extra control

I see the point. So I'd say it's not a necessity until someone else comes in and brings up a better argument.

 

[Dirac]

Why do structs that have no index method don't index any units at all?

struct Test extends array
  private static method filter takes unit u returns boolean
    call BJDebugMsg("this never displays")
    return false
  endmethod
endstruct

 

[Nestharus]

Because if there is no index and no deindex method, it expects that you don't want any units to be indexed for that struct

 

[Bribe]

Why do structs that have no index method don't index any units at all?

struct Test extends array
  private static method filter takes unit u returns boolean
    call BJDebugMsg("this never displays")
    return false
  endmethod
endstruct

Because your indentation is terrible.

 

[Nestharus]

Actually, the allocated portion of the module could use a little work ; P. I'll probably get to updating it tomorrow ; ).

 

[jim7777]

err the system is bugging with preplaced units, i tried debugging it and it shows me the lock unlock blah blah errors with null unit or unlocked unit

 

[Nestharus]

Please provide more information as I can't reproduce the error you are experiencing. I placed two units on the map and ran this script and there were 0 problems. If I can't reproduce the error, there is no way that I can help you.


Are you sure that your code is correct? Are you sure that you have things configured correctly? I know that one person didn't have undefend in their map, which was causing them to have indexing errors.


All recent bugs with this system have been a direct result of the user doing something wrong lately, so make sure that you are using everything correctly first =).

struct tester extends array
    private static unit array u
    private static integer c = 0
    private static timer t = CreateTimer()
    private static method r takes nothing returns nothing
        loop
            exitwhen 0 == c
            set c = c - 1
            call RemoveUnit(u[c])
            set u[c] = null
        endloop
    endmethod
    private method index takes nothing returns nothing
        call DisplayTimedTextToPlayer(GetLocalPlayer(),0,0,60,GetUnitName(GetIndexedUnit()))
        set u[c] = GetIndexedUnit()
        set c = c + 1
        call TimerStart(t,3,false,function thistype.r)
    endmethod
    private method deindex takes nothing returns nothing
        call DisplayTimedTextToPlayer(GetLocalPlayer(),0,0,60,GetUnitName(GetIndexedUnit()))
    endmethod
    implement UnitIndexStruct
endstruct