[General] Flux's Buff System -> Transfer variable to onApply

[maxodors]

I am struggling to understand what is the problem with the variable transfer. I am using [vJASS] - Buff System. The problem is that a variable assigned during buff application is not transferred to the Buff to onApply function. To clarity, here is an example:

struct TestBuff extends Buff
  requirements...
  integer test

 method onRemove takes nothing returns nothing
      debug call(return test)
  endmethod

  method onApply takes nothing returns nothing
      debug call(return test)
  endmethod

endstruct

struct Spell
    method buff takes nothing returns nothing
          local TestBuff b = TestBuff.add ...
          set b.duration = ..
          set b.test = 1
    
    endmethod
endstruct

In this example, test returns 0 onApply, but 1 onRemove. What is the problem and how can I fix this? This is a huge problem for me now, and I would really appreciate any help.

 

[Elprede]

When you call the add method, onApply runs automatically. Therefore, test will never be equal to one on onApply, since test is set to one after onApply is called.

To do what you want to do, split the method add under the module BuffApply into two separate methods, one of which just setups data and the other one calls the check method, which calls onApply.

 

[maxodors]

When you call the add method, onApply runs automatically. Therefore, test will never be equal to one on onApply, since test is set to one after onApply is called.

To do what you want to do, split the method add under the module BuffApply into two separate methods, one of which just setups data and the other one calls the check method, which calls onApply.

This didn't solve the problem.

 

[Elprede]

static method createEx takes nothing returns thistype
    local thistype this = thistype.create()
    //Write into readonly attributes
    set s__Buff_rawcode[this] = thistype.RAWCODE
    set s__Buff_stackType[this] = thistype.STACK_TYPE
    set s__Buff_dispelType[this] = thistype.DISPEL_TYPE
    //set this = this.check(source, target)  /* Remove any kind of checking in add */
    return this
endmethod
      
method add takes unit source, unit target returns thistype
    set this = this.check(source, target)
    return this
endmethod

method buff takes nothing returns nothing
    local TestBuff b = TestBuff.createEx()
    set b.test = 1
    set b.duration = ...
    call b.add(GetTriggerUnit(), GetSpellTargetUnit())
endmethod

 

[maxodors]

static method createEx takes nothing returns thistype
    local thistype this = thistype.create()
    //Write into readonly attributes
    set s__Buff_rawcode[this] = thistype.RAWCODE
    set s__Buff_stackType[this] = thistype.STACK_TYPE
    set s__Buff_dispelType[this] = thistype.DISPEL_TYPE
    //set this = this.check(source, target)  /* Remove any kind of checking in add */
    return this
endmethod
 
method add takes unit source, unit target returns thistype
    set this = this.check(source, target)
    return this
endmethod

method buff takes nothing returns nothing
    local TestBuff b = TestBuff.createEx()
    set b.test = 1
    set b.duration = ...
    call b.add(GetTriggerUnit(), GetSpellTargetUnit())
endmethod

Thanks! What do you mean by removing any kind of checking in add? There is no checking in add. Will it break anything in the system? Sorry, I really have trouble into understanding this system for now.

 

[maxodors]

static method createEx takes nothing returns thistype
    local thistype this = thistype.create()
    //Write into readonly attributes
    set s__Buff_rawcode[this] = thistype.RAWCODE
    set s__Buff_stackType[this] = thistype.STACK_TYPE
    set s__Buff_dispelType[this] = thistype.DISPEL_TYPE
    //set this = this.check(source, target)  /* Remove any kind of checking in add */
    return this
endmethod
   
method add takes unit source, unit target returns thistype
    set this = this.check(source, target)
    return this
endmethod

method buff takes nothing returns nothing
    local TestBuff b = TestBuff.createEx()
    set b.test = 1
    set b.duration = ...
    call b.add(GetTriggerUnit(), GetSpellTargetUnit())
endmethod

I just found it that creates a problem. For some reason, when doing "BUFF_STACK_NONE", it tried to remove instances that do not exist in the end of the buff. I tested it with the old version, and there are no problems.

 

[Pyrogasm]

For some reason, when doing "BUFF_STACK_NONE", it tried to remove instances that do not exist in the end of the buff. I tested it with the old version, and there are no problems.

It's not clear to me what you describing. Can you be more specific/concrete about what occurs? Could you post an example map or example code?

 

[maxodors]

It's not clear to me what you describing. Can you be more specific/concrete about what occurs? Could you post an example map or example code?

Edit: I adjusted Flux Test Map. You can see the bug if you cast W multiple times in a row.

 

[maxodors]

Up

 

[maxodors]

up

 

[Pyrogasm]

That sort of a reply is really not warranted in a forum with as little traffic as this one has. I cannot get any debug error messages to show up in the test map, no matter how quickly I spam the W skill. Yes I have it in debug mode. Yes I recompiled after enabling debug mode.

Please post a test map or video or series of pictures or text output or anything with a clear set of reproducible steps to cause the erroneous removal.

 

[maxodors]

That sort of a reply is really not warranted in a forum with as little traffic as this one has. I cannot get any debug error messages to show up in the test map, no matter how quickly I spam the W skill. Yes I have it in debug mode. Yes I recompiled after enabling debug mode.

Please post a test map or video or series of pictures or text output or anything with a clear set of reproducible steps to cause the erroneous removal.

Here is a video. I just use W two times in the map I attached (it is the same map as before). I think you did not get the error because I meant literally to press W instead of using the W skill.

 

[Pyrogasm]

I think you did not get the error because I meant literally to press W instead of using the W skill.

No, it's because debug mode enabled in JASSHelper actually does nothing as of the 1.33b patch. I had to manually remove the debugs with debug mode enabled to get it to show that message.

The issue is an due to an intersection of a design choice I kind of don't agree with and laziness about timers. In the DoT trigger the buff instance is created (with .createEx()) and the duration is assigned before the stacking check is performed. Then .add() is called which eventually calls .check():

private static method onCast takes nothing returns nothing
    local DOTBuff b
    call BJDebugMsg(GetUnitName(GetTriggerUnit()) + " casted DamageOverTime on " + GetUnitName(GetSpellTargetUnit()))
    set b = DOTBuff.createEx()
    set b.duration = 3.0
    call b.add(GetTriggerUnit(), GetSpellTargetUnit())
endmethod

Now when .check() is run and the buff is BUFF_STACK_NONE and the target already has an instance of the same buff what does it do? It destroys the 'new' buff it was trying to apply and instead just reassigns this = <the old buff object>:

elseif this.stackType == BUFF_STACK_NONE then
    //Check if a similar buff type with the same target exist
    set temp = thistype.get(null, target, this.getType())
    if temp == 0 then   //None is found
        set apply = true
    else                //Buff is found, use the previous Buff as the newly applied Buff
        call this.destroy()
        set this = temp
    endif
endif

This is kind of silly since when the buff is cast on a target that already has it you allocate a new instance of the struct and then immediately destroy it... but it wouldn't be problematic, merely inefficient. However, when the duration was set before the stack check it immediately started a one-shot timer for that duration. That timer isn't paused or destroyed before/as the aborted buff instance is destroyed so it's still running and it'll still expire (and it's also a memory leak):

method operator duration= takes real time returns nothing
    if this.t == null then
        static if LIBRARY_TimerUtils then
            set this.t = NewTimerEx(this)
        else
            set this.t = CreateTimer()
            call SaveInteger(thistype.hash, GetHandleId(this.t), 0, this)
        endif
    endif
    call TimerStart(this.t, time, false, function thistype.expires)
endmethod

You can fix this by adding an onDestroy method that automatically cleans up the timers whenever .destroy() is called. Put this inside the main Buff struct declaration:

       private method onDestroy takes nothing returns nothing
            static if LIBRARY_TimerUtils then
                call ReleaseTimer(this.t)
            else
                call RemoveSavedInteger(thistype.hash, GetHandleId(this.t), 0)
                call DestroyTimer(this.t)
            endif
            set this.t = null
        endmethod

 

[maxodors]

No, it's because debug mode enabled in JASSHelper actually does nothing as of the 1.33b patch. I had to manually remove the debugs with debug mode enabled to get it to show that message.

The issue is an due to an intersection of a design choice I kind of don't agree with and laziness about timers. In the DoT trigger the buff instance is created (with .createEx()) and the duration is assigned before the stacking check is performed. Then .add() is called which eventually calls .check():

private static method onCast takes nothing returns nothing
    local DOTBuff b
    call BJDebugMsg(GetUnitName(GetTriggerUnit()) + " casted DamageOverTime on " + GetUnitName(GetSpellTargetUnit()))
    set b = DOTBuff.createEx()
    set b.duration = 3.0
    call b.add(GetTriggerUnit(), GetSpellTargetUnit())
endmethod

Now when .check() is run and the buff is BUFF_STACK_NONE and the target already has an instance of the same buff what does it do? It destroys the 'new' buff it was trying to apply and instead just reassigns this = <the old buff object>:

elseif this.stackType == BUFF_STACK_NONE then
    //Check if a similar buff type with the same target exist
    set temp = thistype.get(null, target, this.getType())
    if temp == 0 then   //None is found
        set apply = true
    else                //Buff is found, use the previous Buff as the newly applied Buff
        call this.destroy()
        set this = temp
    endif
endif

This is kind of silly since when the buff is cast on a target that already has it you allocate a new instance of the struct and then immediately destroy it... but it wouldn't be problematic, merely inefficient. However, when the duration was set before the stack check it immediately started a one-shot timer for that duration. That timer isn't paused or destroyed before/as the aborted buff instance is destroyed so it's still running and it'll still expire (and it's also a memory leak):

method operator duration= takes real time returns nothing
    if this.t == null then
        static if LIBRARY_TimerUtils then
            set this.t = NewTimerEx(this)
        else
            set this.t = CreateTimer()
            call SaveInteger(thistype.hash, GetHandleId(this.t), 0, this)
        endif
    endif
    call TimerStart(this.t, time, false, function thistype.expires)
endmethod

You can fix this by adding an onDestroy method that automatically cleans up the timers whenever .destroy() is called. Put this inside the main Buff struct declaration:

       private method onDestroy takes nothing returns nothing
            static if LIBRARY_TimerUtils then
                call ReleaseTimer(this.t)
            else
                call RemoveSavedInteger(thistype.hash, GetHandleId(this.t), 0)
                call DestroyTimer(this.t)
            endif
            set this.t = null
        endmethod

Thanks, but this creates a new error. Warning: attempt to release a null timer. Edit: old error still stays

In terms of efficiency, do you think there is a better buff system to use? Performance is really important for me. I was thinking of using this one: [System] Buff

 

[Pyrogasm]

The old error does not stay. It stopped being shown in my map with the onDestroy I wrote (and returns when it’s commented out). I don’t know what to tell you.

Yes, the other parts of this code that destroy the struct instance will still try to destroy the timer. Note that this is just a warning and you can only see it if debug is enabled. The only way this is bad is if a user doesn’t have TimerUtils and then double-destroys a timer that had been already destroyed and whose handle ID had been reallocated to something else. This could possibly destroy the wrong timer.
L

 

[maxodors]

The old error does not stay. It stopped being shown in my map with the onDestroy I wrote (and returns when it’s commented out). I don’t know what to tell you.

Yes, the other parts of this code that destroy the struct instance will still try to destroy the timer. Note that this is just a warning and you can only see it if debug is enabled. The only way this is bad is if a user doesn’t have TimerUtils and then double-destroys a timer that had been already destroyed and whose handle ID had been reallocated to something else. This could possibly destroy the wrong timer.
L

Wouldn't it just be better to move duration to the add function and then this problem is solved?

 

[Pyrogasm]

You could just try that instead of asking.

Yes.

 

[maxodors]

You could just try that instead of asking.

Yes.

I mentioned it because it might cause another bug that I am not aware of. Since you know the system, you could have realised it.

On another note, it works perfectly fine now. Thanks, without your detailed explanation, I wouldn't be able to come with the solution.

        method add takes unit source, unit target, real duration returns thistype
            set this = this.check(source, target)
            set this.duration = duration
            return this
        endmethod

Edit: It seems like it creates another. The buff works only the first time. It does not work afterwards. I will try and see what is the issue

Edit: everything works fine. It was my mistake.