RespawnSystem v1.1 [Zinc]

[Moderator]

18:45, 19th Aug 2013
PurgeandFire: Changes made. Approved.

 

[watermelon_1234]

Even though it may seem obvious, you should still mention that your system is for respawning non-hero units. (It doesn't have ideal behavior for heroes since it creates new units instead of reviving them.)

You should document what FIXED_DELAY and FIXED_SFX do.

Opinion
DEFAULT seems like a better name choice than FIXED.

I'd suggest saving information in a struct instead of abusing TableArray; it's easier to understand and maintain the code rather than having to remember arbitrary values like 4 being the index for a special effect.

Your coding style is rather unappealing. :\
Specifically, doing things like

if (rs[4].string[id] != "") {DestroyEffect(AddSpecialEffectTarget(rs[4].string[id], u, "origin"));}

// Having different indentation for this can also be misleading.
            if (pl==null) {rs[5].player[uID] = GetOwningPlayer(u);}
            else {rs[5].player[uID] = pl;}

and

    public static method remove(unit u)
        {chk.remove(GetHandleId(u));}

Squishing braces like that can be confusing.
The opening library brace and the two ending braces from the struct and library makes the code look confusing.

 

[mckill2009]

forgot to filter out the non-hero thing as this suppose to create only non-hero units...

I'd suggest saving information in a struct instead of abusing TableArray

it'll still need Table for that since I can only save the 'this' to the handle of the dying unit and then pass to the next unit...and it will be limited to 8192, but then again who uses 8192 anyway ...

Your coding style is rather unappealing. :\

sorry for that, its my first time to use zinc, but I think that it's OK to reduce lines...

 

[Almia]

YAY! A Zinc Coder

 

[Magtheridon96]

- Indentation needs work (needs to be consistent and readable)
- Default does actually make a better label here than Fixed because it expresses the intent and purpose of the global constants
- Can we agree to always use one style of indentation in one library?

The accepted styles of indentation here are as follows:

• K&R
  

  if (x) {
      y();
  } else {
      z();
  }

• Allman
  

  if (x)
  {
      y();
  }
  else
  {
      z();
  }

• Stroustrup
  

  if (x) {
      y();
  }
  else {
      z();
  }

GNU style, Whitesmiths style, Pico style and pretty much any other style you can think of, yeah... don't use them. Your colleagues will hate you.

 

[Almia]

@Mag
what da? One-liner funcs or ifs are good! or maybe, not in readability

 

[mckill2009]

K&R is best IMO Mags, and I really do wanna apply it but it's just a one-liner like Almia said, so is it not acceptable?...

 

[Magtheridon96]

I couldn't care less about that part, it's the fact that you're combining Allman and GNU style that's bugging me

If you want to keep some of them one-liners, it's fine, but make sure the spacing is okay

 

[PurgeandFire]

You don't need 6 or so tables/instances. You only need 1 for this.

struct X
    static Table instance = 0
    real x
    real y
    integer id 
    // ... other data

    static method create takes unit u returns thistype 
         local thistype this = thistype.allocate()
         set this.x = GetUnitX(u)
         set this.y = GetUnitY(u) // etc... 
         //... other saving
         set instance[GetUnitTypeId(u)] = this // only need to save this
         return this 
    endmethod

    // move this to module init, though
    private static method onInit takes nothing returns nothing 
         set instance = Table.create()
    endmethod
endstruct

Then when you load it, you just load the instance via:

local thistype this = instance[GetUnitTypeId(GetTriggerUnit())]

Right now, your method works, but it is reading quite a few things from a hashtable. Reading it from the arrays will save a lot of time on both lookup and saving. Of course, this means you are limited to 8192 creeps, but that is an issue that can be ignored for most maps.

By the way, you should save under the unit's type ID. Saving under the handle ID means that you have to save the data all over again each time the unit dies. However, if you save it under the unit's type ID, you only need to save it once (since the next unit rezz'd will have the same type ID).

If you make those changes, it will be approvable.

 

[mckill2009]

That's bad Purge, even though the 'this' is changing it's instance, the UnitType which is the parent id of the hash will be overwritten...

Now instead of GetUnitTypeId save, GetHandleId save works...

//save
set instance[GetHandleId(u)] = this
//load
local thistype this = instance[GetHandleId(GetTriggerUnit())]

 

[PurgeandFire]

Yeah, I'm sorry. I wasn't thinking properly. Use GetHandleId().

 

[Luorax]

@PurgeAndFire:

He could actually do it without having to use Table at all. All you need is a unit indexer (that almost every mapper has - although the same goes to Table as well) and an integer array. It should be faster - not that it'd make that big of a difference for such resource, though I usually stick to that solution.

 

[mckill2009]

I do not like ANY unit indexer that uses other libraries, Imo Bribe's UI is best but it's GUI so I dont want to mix it with vjass, although it can be used in general...