Celestial Beam v1.0.5b

[Moderator]

19:06, 25th Sep 2013
PurgeandFire: Approved!

(Old) Review:
http://www.hiveworkshop.com/forums/2422044-post17.html

 

[Daffa]

Use (All Players) player group for Disabling the Research instead of manually doing it for all player, it will shorten the triggers.

 

[Manasurge]

Use (All Players) player group for Disabling the Research instead of manually doing it for all player, it will shorten the triggers.

Heh, fixed x) dunno why I didn't do that from the get go

 

[Maker]

Use variables for the dummy unit type and the ability.

 

[Manasurge]

Use variables for the dummy unit type and the ability.

Fixed, I'm drawing my own conclusion here and assume that it causes an issue when importing triggers if it's not stored? Trigger not finding ability & unit ID until manually refreshed.

 

[Azusa]

don't you need to store this to variable as well?

• ((picked unit) belongs to (owner of CB_Temp_Dummy)) equal to true

 

[Manasurge]

don't you need to store this to variable as well?

• ((picked unit) belongs to (owner of CB_Temp_Dummy)) equal to true

I only that line once, iirc it's more ineffective to store it into a variable if you're gonna use it less than 3 times.
I'm going to store the (picked unit) for the next update version but atm I'm going over the code again to see if I can make it more efficient before updating.

 

[Azusa]

i see..
well cool spell, i like it

 

[deathismyfriend]

Actually Azusa is right owner of matching unit will most likely be used twice or more. I go with the twice or more store it into a variable. If you have 5 units being added to the unit group it will run 5 times. It is better to store those. That being said picked unit / key of picked unit / key of cb unit should be stored.

 

[Manasurge]

Actually Azusa is right owner of matching unit will most likely be used twice or more. I go with the twice or more store it into a variable. If you have 5 units being added to the unit group it will run 5 times. It is better to store those. That being said picked unit / key of picked unit / key of cb unit should be stored.

Ouch so true x( massive failure on my side. Didn't think about it like that only went through the code and saw it was only used once, didn't think about the fact that it's used once per picked unit. Already adjusted in v1.0.2

also, I am storing the key the variable is CB_Key_PickedUnit ^^, sneaky name is sneaky

 

[deathismyfriend]

In your cast trigger it looks like you are using key of cb key unit not storing it into an integer variable and using the integer variable. The key of a unit is an integer.
You custom script would actually look like this.

• Custom script: set udg_key = GetHandleId( udg_unitVariable)

 

[Manasurge]

In your cast trigger it looks like you are using key of cb key unit not storing it into an integer variable and using the integer variable. The key of a unit is an integer.
You custom script would actually look like this.

• Custom script: set udg_key = GetHandleId( udg_unitVariable)

Not really sure what I'm doing with that custom script, I probably should stick with the regular GUI method although that'll use (picked unit) instead of a variable.

anyways, I tried your way and your line returns an error for me. But the line I'm using, picked it up earlier @ hive somewhere, is working afaik.

 

[Magtheridon96]

There's nothing bad about Map Initialization.

You would use the Elapsed Game Time event 0.00 second event if your trigger heavily relies on the initialization of another system that inits with a Map Initialization event.
You're not doing that here, so yeah.

 

[Manasurge]

There's nothing bad about Map Initialization.

You would use the Elapsed Game Time event 0.00 second event if your trigger heavily relies on the initialization of another system that inits with a Map Initialization event.
You're not doing that here, so yeah.

Ah ok Changed it in the next version. I'll upload right away if there's nothing else. Lack of comments from mods either really good or really really really bad

 

[ILH]

* Visual Yummies

Lol, fix that

 

[T. D. W.]

1.0.3 Review

Review

Suggestions

Rating

Judgement

My Words

Set CB_Point_Current = (Position of CB_Temp_TriggU)

You leak a Point here ^

Set CB_Point_Current = (Load 0 of (Key CB_Key_PickedUnit) in Celestial_Beam_Table)

Another Leak here ^
You Should Add Custom Spells to dummies with triggers, since their Raw Code will Change when importing
This spell is in the "easy" category so it's a normal job, just fix what i just mentioned

None

If You want me to review an Update of This Spell, Please Notifiy me this by VM or PM

 

[PurgeandFire]

Review:

• In CB_Spell_Cast, you leak right here:
  
  • Set CB_Temp_Degrees = (Angle from CB_Point_Current to (Target point of ability being cast))
  You must set (Target point of ability being cast) to a variable first.
• In CB_Spell_Loop, you shouldn't really use:
  
  • (Number of units in CB_Dummy_Group) Equal to 0
  It is a somewhat expensive function that will iterate through the entire group to determine if the group is empty. You should instead use a counter. Each time you add a unit to the group, increment:
  
  • Set CB_Group_Counter = CB_Group_Counter + 1
  Each time you remove a unit from the group, decrement:
  
  • Set CB_Group_Counter = CB_Group_Counter - 1
  Then instead of the (Number of units in group...) condition, you can just check if CB_Group_Counter is 0.
• You should store Key CB_Key_PickedUnit into an integer variable since you use it around 5 or so times. Otherwise it calls GetHandleId() each time.

Fix those things and I believe it should be ready for approval.

 

[Manasurge]

[COLOR="White"

[COLOR="White"

[COLOR="White"

[COLOR="White"

[COLOR="White"

Review[/COLOR]]

Set CB_Point_Current = (Position of CB_Temp_TriggU)

You leak a Point here ^

Set CB_Point_Current = (Load 0 of (Key CB_Key_PickedUnit) in Celestial_Beam_Table)

Another Leak here ^
You Should Add Custom Spells to dummies with triggers, since their Raw Code will Change when importing
This spell is in the "easy" category so it's a normal job, just fix what i just mentioned

Suggestions[/COLOR]]None

Rating[/COLOR]]

Judgement[/COLOR]]

My Words[/COLOR]]If You want me to review an Update of This Spell, Please Notifiy me this by VM or PM

It's not leaking, I'm removing that location when I close the loop. It's always just 1 location and I'm not overwriting it before I destroy it.

 

[T. D. W.]

I think it still leaks since you overwrite the variable before removing it

 

[FUJI]

This can be done in Indexed Arrays .

 

[Manasurge]

This can be done in Indexed Arrays .

I can't do Index Arrays =) Atleast not yet, atm it's reading GUI with Indexing makes my brain melt.

 

[Manasurge]

I think it still leaks since you overwrite the variable before removing it

It'd be true if I overwrite it, but I don't. I'm simply pointing at the same location several times.