View Single Post
Old 12-13-2010, 11:21 AM   #3
Anitarf
Procrastination Incarnate


Development Director
 
Join Date: Feb 2004
Posts: 8,190

Submissions (19)

Anitarf has a brilliant future (903)Anitarf has a brilliant future (903)Anitarf has a brilliant future (903)Anitarf has a brilliant future (903)Anitarf has a brilliant future (903)Anitarf has a brilliant future (903)Anitarf has a brilliant future (903)Anitarf has a brilliant future (903)

2008 Spell olympics - Fire - SilverApproved Map: Old School Alliance TacticsHero Contest #2 - 3rd PlaceSpell making session 2 winner

Default

Quote:
Originally Posted by BlackRose
Useful. I like it, especially the optional support for AutoIndex, as people also use other indexers.
Since I often see comments that imply that AutoIndex is incompatible with other indexing systems (I'm not saying you think that, but it is one possible interpretation of your words), I would like to take this opportunity to point out that AutoIndex can be configured to use a table instead of unit user data, so there's nothing preventing people from using it even if they are already using another indexing system.

The only reason the AutoIndex requirement is optional is because I don't want libraries to have more requirements than is needed. Since Table can already fulfil the job of attaching data to units, AutoIndex is optionally supported only because it can do the job slightly faster, so there's no sense not to use it if it happens to be in the map already. I suppose I should add that explanation to the documentation.

Quote:
I found a mistake here: "Followin".
Fixed.

Quote:
Although one thing I don't understand is why UnitEntersMap is not placed in UnitEntersMapTrigger, instead it's being called.
UnitEntersMap is needed for the OnUnitIndexed event of AutoIndex. I didn't feel like copying the same code to two functions. Getting rid of the local variable was just a bonus.

Quote:
Also can't GetUnitTypeId(u) be set to a local to avoid double call? Although this is done on initialization, so I guess it's negligible. The same goes for GetUnitId().
Not on initialization, it's done whenever a unit enters the map, which is still negligible. GetUnitTypeId is a native and GetUnitId inlines a native, so they should be quite fast, although I am not familiar with how exactly they compare to a local variable declaration. If they are significantly slower then I guess I could change this.

Quote:
scaleY and scaleZ don't work, so you could save two multiplications. I don't know why Blizzard implemented them.
A minor point, but a good catch nonetheless. Fixed.

Quote:
Or simply: R2I(255 /*(100 for alpha)*/ * I2R(max) * 0.01)? No need to check for max, users shouldn't be setting values to below 0 or above 255 anyways.
They shouldn't, but they might. If they do and I don't include that safety, then the stored ARGB will not match the unit's actual color, but will be all wonky. Note that this is a BJ hook; any user concerned with speed shouldn't be calling BJs in the first place, so this function won't affect his map. Same could be said about your previous point, but since I could change the scale function without any consequences, there was no point in arguing.

Thanks for the review.
__________________
Anitarf is offline   Reply With Quote