Refund System

[Daffa]

Cool, never thought this can be something of a big idea.

Just some stuff I found:

1. This code:

call AddSpecialEffectTargetUnitBJ("overhead", GetTriggerUnit(), "UI\\Feedback\\GoldCredit\\GoldCredit.mdl")
call DestroyEffectBJ(GetLastCreatedEffectBJ())

can be optimized into:

call DestroyEffectBJ(AddSpecialEffectTargetUnitBJ("overhead", GetTriggerUnit(), "UI\\Feedback\\GoldCredit\\GoldCredit.mdl"))

2. Special effect SFX should be in configuration.

 

[Mayday]

Cool, never thought this can be something of a big idea.

Just some stuff I found:

1. This code:

call AddSpecialEffectTargetUnitBJ("overhead", GetTriggerUnit(), "UI\\Feedback\\GoldCredit\\GoldCredit.mdl")
call DestroyEffectBJ(GetLastCreatedEffectBJ())

can be optimized into:

call DestroyEffectBJ(AddSpecialEffectTargetUnitBJ("overhead", GetTriggerUnit(), "UI\\Feedback\\GoldCredit\\GoldCredit.mdl"))

2. Special effect SFX should be in configuration.

Hi daffa, thank you for your suggestions. Coding is not my forte

 

[MyPad]

Looks rather simple, but probably useful. Needless to say, I'll delve into the code.

Spoiler: Code Review

• This is more on personal taste, but I think the native declarations should be moved inside the library using them.
  
  
• You can make the function return early so that you don't have to deal with nulling the text tags when they don't exist yet.
  A sample of the code rewrite is shown below:
  
  

  if (GetSpellAbilityId() != REFUND_ABIL_ID) then
      set u = null
      return
  endif
  // ===========
  // Rest of the code.
  // ===========
  set tg = null
  set tl = null
  set u = null

  
  
• You can use SetTextTagPosUnit(texttag, unit, real zOffset) as an alternative to SetTextTagPos.
  
  
• While the bundle is called Refund System, the underlying code appears to be closely tied to an ability, making this more of a spell in my book. Hence, you may choose to keep it a library or go with scopes (For spells, I prefer scopes for encapsulation).

Phew, this one was relatively lightweight. I think it may come in handy to some.

Useful

 

[Mayday]

Looks rather simple, but probably useful. Needless to say, I'll delve into the code.

Spoiler: Code Review

• This is more on personal taste, but I think the native declarations should be moved inside the library using them.
  
  
• You can make the function return early so that you don't have to deal with nulling the text tags when they don't exist yet.
  A sample of the code rewrite is shown below:
  
  

  if (GetSpellAbilityId() != REFUND_ABIL_ID) then
      set u = null
      return
  endif
  // ===========
  // Rest of the code.
  // ===========
  set tg = null
  set tl = null
  set u = null

  
  
• You can use SetTextTagPosUnit(texttag, unit, real zOffset) as an alternative to SetTextTagPos.
  
  
• While the bundle is called Refund System, the underlying code appears to be closely tied to an ability, making this more of a spell in my book. Hence, you may choose to keep it a library or go with scopes (For spells, I prefer scopes for encapsulation).

Phew, this one was relatively lightweight. I think it may come in handy to some.

Useful

Hello pad,

thank you for the review. due to a request, i have updated the system to include these changes (except for the SetTextTag change, because I am using an offset) as well as included new configurables to check for required HP amount to refund. Please let me know if you'd like to see any other changes made!

 

[Pyrogasm]

Error in this line: GetUnitLifePercent(u) == HITPOINTS_REQUIRED, should be >= otherwise this check will literally always fail (because you're comparing if a real is exactly equal to another real and good fucking luck with that and floating point precision) if HITPOINTS_REQUIRED is not 100.

Could use a small amount of logic to round non-integer gold/lumber refund remainders. Right now the R2I() call forces it to always truncate and never round. It would only change 1 gold/lumber either way but it might matter if the map this is used in has very low gold/lumber costs for units.

Also there are 3 values that you are recomputing multiple times in the main function, which could simply be computed once each and stored in some locals (or a global if you really don't want to null things): GetUnitX(u), GetUnitY(u), and GetOwningPlayer(u). In PLAYER_UNIT_EVENT_ events you can actually use GetTriggerPlayer() which is the same as GetOwningPlayer(GetTriggerUnit()) with one less call.

 

[Mayday]

thank you so far for all the suggestions, after a quick 2 year reprieve, I have updated this to include a Lua version with additional optimizations.