• 🏆 Texturing Contest #33 is OPEN! Contestants must re-texture a SD unit model found in-game (Warcraft 3 Classic), recreating the unit into a peaceful NPC version. 🔗Click here to enter!
  • It's time for the first HD Modeling Contest of 2024. Join the theme discussion for Hive's HD Modeling Contest #6! Click here to post your idea!

Custom Death v1.07

  • Like
Reactions: Renly Baratheon
This is a small utility designed to make it seem like a unit is dead.

Features:
Events for when a unit dies or is revived.
Makes it possible to target dead units for restoration spells.
Lightweight.

How to import:
Make sure you have "Automatically create unknown variables" set to true
Copy the appropriate configuration to your map, in order to create variables.
Copy the needed trigger "Death" to your map. Names can be changed. You might also need the unit indexer from requirements.
Make sure you have a unit indexer or the system will not work.

vJASS code
JASS:
//
//                  CustomDeath
//                  by Xonok
//
//          This system makes it possible to "kill" units
//          while still keeping them targetable. The system
//          requires any kind of unit indexer or it won't
//          work properly. 
//          Currently this system only contains API functions. 
//
//          KillUnitCustom(unit)
//          |
//          Adds a unit into the system by playing its
//          death animation and making it paused and
//          invulnerable. 
//
//          ReviveUnitCustom(unit) -> return boolean
//          |
//          Removes a unit from this system. Plays the birth
//          animation if unit has one. 
//
//          IsUnitDeadCustom(unit) -> return boolean
//          |
//          A relatively fast check to see whether the unit
//          is in the system. 
//

library CustomDeath
    globals
        private boolean array UnitDead
    endglobals
    
    //+ API
    function IsUnitDeadCustom takes unit un returns boolean
        debug if UnitDead[GetUnitUserData(un)] then
            debug call DisplayTextToPlayer(GetLocalPlayer(),0,0,"CustomDeath: Checked unit" + GetUnitName(un) + "is dead")
        debug else
            debug call DisplayTextToPlayer(GetLocalPlayer(),0,0,"CustomDeath: Checked unit"  + GetUnitName(un) + "is not dead")
        debug endif
        return UnitDead[GetUnitUserData(un)]
    endfunction
    
    function KillUnitCustom takes unit un returns nothing
        local unit backup = udg_DeathUnit
        local integer ID = GetUnitUserData(un)
        debug if UnitDead[GetUnitUserData(un)] then
            debug call DisplayTextToPlayer(GetLocalPlayer(),0,0,"CustomDeath: Tried to kill a dead unit: "+GetUnitName(un))
        debug else
            call SetUnitInvulnerable(un,true)
            call UnitRemoveBuffs(un,true,true)
            call PauseUnit(un,true)
            call SetUnitAnimation(un,"death")
            call SetUnitUseFood(un,false)
            set UnitDead[ID] = true
            set udg_DeathUnit = un
            set udg_DeathEvent = 1
            set udg_DeathEvent = 0
        debug endif
        set udg_DeathUnit = backup
        set backup = null
    endfunction
    
    function ReviveUnitCustom takes unit un returns boolean
        local integer i = 1
        local unit backup = udg_DeathUnit
        local integer ID = GetUnitUserData(un)
        debug if not(UnitDead[GetUnitUserData(un)]) then
            debug call DisplayTextToPlayer(GetLocalPlayer(),0,0,"CustomDeath: Tried to revive a living unit: "+GetUnitName(un))
        debug else
            call SetUnitInvulnerable(un,false)
            call PauseUnit(un,false)
            set UnitDead[ID] = false
            call SetUnitAnimation(un,"birth")
            call QueueUnitAnimation(un,"stand")
            call SetUnitUseFood(un,true)
            set udg_DeathUnit = un
            set udg_DeathEvent = 2
            set udg_DeathEvent = 0
            set udg_DeathUnit = backup
            set backup = null
            return true
        debug endif
        set udg_DeathUnit = backup
        set backup = null
        return false
    endfunction
endlibrary

Changelog:
v1.00 - Uploaded
v1.01 - Added a version of the system with no vJASS
v1.02 - Fixed some variable names issues in vanilla version and made the system disable food cost for dead units.
v.1.03
*Deprecated the vanilla version because importing it is more complicated than simply getting JNGPE.
*Also nulled some globals and added an optional boolean "UseLocust" to make dead units unselectable.
*All buffs are now removed upon death, to fix any issues that might exist with pausing.
*Using SetUnitInvulnerable instead of adding the ability.
v.1.04 - Made it less likely that function names are mistaken for what they are not.
v.1.05 - Deprecated UseLocust because fixing it would require an ability for every possible unit-type.
Also made safety toggleable (whether the system should automatically avoid multiple instances of the same unit)
v.1.06 - Deprecated the unit stack. Added debug messages if debug mode is on in JASSHelper. Fixed the SAFETY static ifs.
v.1.07 - Changed function names to contain only full words.
Simplified debug mode and removed my own constant for it, as it's unnecessary if the system only posts debug messages upon errors.
Contents

CustomDeath v1.07 (Map)

Reviews
00:35 9th May 2014 BPower: Leakless and working. Approved. Rated with ~3, because it's a useful resource for some mappers. If you consider to add extra functionality I'll reconsider my rating. 21:35 8th May 2014 Here is your current review...

Moderator

M

Moderator

00:35 9th May 2014
BPower:
Leakless and working. Approved. Rated with ~3, because it's a useful resource for some mappers. If you consider to add extra functionality I'll reconsider my rating.


21:35 8th May 2014
Here is your current review.
We're not getting anywhere aslong as the function names remain as they are now.

16:42 21nd Mar 2014
BPower:
http://www.hiveworkshop.com/forums/spells-569/custom-death-v1-04-a-248629/index2.html#post2502389

20:03, 2nd Mar 2014
Magtheridon96: IsUnitDead is a terribly misleading function name and UnitDie is a terrible name that makes no sense. I think you should label these deaths as PseudoDead. IsUnitPseudoDead, PseudoKillUnit and PseudoUnkillUnit would be better function names because they insinuate the side effects better.

You would then specify "pseudo-dead" in your required documentation.

Also, space out the functions nicely so the code is easier to read. :v
 

Kazeon

Hosted Project: EC
Level 33
Joined
Oct 12, 2011
Messages
3,449
you reserved it for what? give a valid argument or I will report..

dont use call PauseUnit(un,true) or just remove all buffs first

you dont need to store 'Avul' into globals call UnitAddAbility(un,INVULN) => call UnitAddAbility(un,'Avul'), it's a standart ability. although, 'Aloc' is better than 'Avul', so the dead unit can't be enumerated. it can be a good point or bad because normally dead unit can be enumerated, but using 'Avul' the healthbar is not hidden. so I just recommend everyone to use normal dead instead.

I'm pretty sure you need to null it local unit backup = udg_DeathUnit => set backup = null

then, what's wrong with the normal death? this system is quite buggy where dead units keep all their buffs until they are revived since you pause them. I think this is acceptable if you can prove that this system works faster than the native one :)
 
Level 21
Joined
Mar 27, 2012
Messages
3,232
you reserved it for what? give a valid argument or I will report..

dont use call PauseUnit(un,true) or just remove all buffs first

you dont need to store 'Avul' into globals call UnitAddAbility(un,INVULN) => call UnitAddAbility(un,'Avul'), it's a standart ability. although, 'Aloc' is better than 'Avul', so the dead unit can't be enumerated. it can be a good point or bad because normally dead unit can be enumerated, but using 'Avul' the healthbar is not hidden. so I just recommend everyone to use normal dead instead.

I'm pretty sure you need to null it local unit backup = udg_DeathUnit => set backup = null

then, what's wrong with the normal death? this system is quite buggy where dead units keep all their buffs until they are revived since you pause them. I think this is acceptable if you can prove that this system works faster than the native one :)

Reserved for possible future posts, like many people tend to do.

I use PauseUnit because setting the propulsion angle doesn't completely disable a unit. It must be absolutely sure that a dead unit doesn't receive orders. If you know a better way to disable all abilities and orders, then please tell.

INVULN is a global because I want the system to be configurable. For instance, if for some reason the user wants to use a different ability for invulnerability. Perhaps corpses can be hurt by magic? I have no idea and don't have to know exactly what the user will do.
I could make it a constant though.
Also, I didn't use locust because I need corpses to be targetable and also I don't remember how exactly to remove locust without issues. If someone tells me how to remove locust, then I'll add it as an option that dead units are unselectable.

Forgot about the nulling. Thanks.

The reason I made this system is that I'll need it in my own map. I want non-hero units to be able to be revived, like in some RPGs. For various reasons the usual resurrect ability is not good enough, so eventually I figured that it's just better to simulate death.
It is intentional that "dead" units can be targeted with abilities, as elseway a revival would not work.

This system can also be made to work together with a unit recycler in maps that need them, so that a unit could be killed and stay dead for a while, after which it can easily be revived (a defense map where corpses can be revived maybe)

Unmentioned: The reason I keep the unit list non-private is that then you can loop through the dead units no matter what. This can perhaps be made to work together with locust in the future to loop through all dead units in an area. That also reduces the looping done because you don't have to filter out alive units.
 

Kazeon

Hosted Project: EC
Level 33
Joined
Oct 12, 2011
Messages
3,449
Reserved for possible future posts, like many people tend to do.
if you dont mind, what kind of post is that?

I use PauseUnit because setting the propulsion angle doesn't completely disable a unit
remove all the buffs first or the buff will stay on unit until they get revived

The reason I made this system is that I'll need it in my own map. I want non-hero units to be able to be revived
if that's the reason, you should make a normal unit respawn system instead of this :p it's a lot easier.
 
Level 21
Joined
Mar 27, 2012
Messages
3,232
if you dont mind, what kind of post is that?


remove all the buffs first or the buff will stay on unit until they get revived


if that's the reason, you should make a normal unit respawn system instead of this :p it's a lot easier.

I don't know. Doesn't really matter much, does it? It does not qualify as a bump because I made the post just minutes after uploading, so the resource is visible just as much.

I can remove the buffs. Just wondering if there's a better way than pausing.

What do you have in mind? Respawning by creating units messes up quite a lot of things.
 

Kazeon

Hosted Project: EC
Level 33
Joined
Oct 12, 2011
Messages
3,449
Respawning by creating units messes up quite a lot of things
I'm not pretty sure if that's what's happening but I think you are right, never thought of it before.

more than pausing, huh? maybe you can add that unit to a unit group which if any unit in that group casts a spell, they will be ordered to stop. not sure if it's good :p
 
Level 21
Joined
Mar 27, 2012
Messages
3,232
I'm not pretty sure if that's what's happening but I think you are right, never thought of it before.

more than pausing, huh? maybe you can add that unit to a unit group which if any unit in that group casts a spell, they will be ordered to stop. not sure if it's good :p

I tried changing turning speed and propulsion angle to 0. Turning still worked and this meant that eventually units would simply stand up and move, although still turn very slowly.

Cancelling orders through stop has a slight delay due to the limit of 30 orders per second per player.
I guess for now it's best to just remove buffs.
 
Level 19
Joined
Mar 18, 2012
Messages
1,716
All global variables should be private, furthermore INVULN should be constant aswell.

I think IsUnitDead should read from either a Table/Hashtable return table.has(GetHandleId(who))
or from an boolean/integer/unit array using UnitUserData. dead[GetUnitUserData(who)]

Keep in mind that pausing an unit also pauses applied buffs and can screw up animations.

Edit: Already mentioned.
You have to null local unit variables. There are diverse opinions about nulling global variables, still I strongly recommend to null them aswell.

For logical reasons the loop index should start with 0.

Please explain:
JASS:
    function in takes nothing returns nothing
    endfunction

In my mind the visual effect is poor, because the health bar is still shown.
Using locust, coupled with bearform/metamorph would make the system much nicer.
There is no way to remove the healthbar and make the unit target-able.

Consider function RegisterDeath/ReviveEvent takes Event which, boolexp expr returns nothing
 
Level 21
Joined
Mar 27, 2012
Messages
3,232
All global variables should be private, furthermore INVULN should be constant aswell.

I think IsUnitDead should read from either a Table/Hashtable return table.has(GetHandleId(who))
or from an boolean/integer/unit array using UnitUserData. dead[GetUnitUserData(who)]

Keep in mind that pausing an unit also pauses applied buffs and can screw up animations.

Edit: Already mentioned.
You have to null local unit variables. There are diverse opinions about nulling global variables, still I strongly recommend to null them aswell.

For logical reasons the loop index should start with 0.

Please explain:
JASS:
    function in takes nothing returns nothing
    endfunction

In my mind the visual effect is poor, because the health bar is still shown.
Using locust, coupled with bearform/metamorph would make the system much nicer.
There is no way to remove the healthbar and make the unit target-able.

Consider function RegisterDeath/ReviveEvent takes Event which, boolexp expr returns nothing

I definitely see the merit in using a hashtable. It would perhaps even make the system faster when there are many dead units (when it truly matters).
I don't see how using an additional boolean array would help though. Currently the system only stores "dead" units and thus, all of them would have the boolean true.
Or hmm, now that I think of it, I think I know what you mean. If the user has a unit indexer, then I can avoid the looping in that function completely and just make each unit have a static position in the array.

I use loop index 1 because in most of my coded things I set 0 for errors. Indeed, there can be no casual errors in a code like this, as it would break something very fast.
The second reason is that if I used position 0, then the variable DeadUnits would not tell the last index directly anymore, but instead with an offset of 1.

At first I thought that I'll need an initalizer, but I was wrong. I'll remove it in the next version.
I'll use locust in a future version once I find out the exact way to remove it.

RegisterDeath/Revive. That's quite a good idea, although there is a significant problem. If I register a death event, then isn't the unit already dead? It would be better to do this through a DDS, but that is complicated due to the high amount of damage detection systems and the differences they have in implementation.
 
Level 18
Joined
Sep 14, 2012
Messages
3,413
JASS:
    function UnitDie takes unit un returns nothing
        local unit backup = udg_DeathUnit
        set DeadUnits = DeadUnits + 1
        set DeadUnit[DeadUnits] = un
        call UnitAddAbility(un,INVULN)
        call PauseUnit(un,true)
        call SetUnitAnimation(un,"death")
        set udg_DeathUnit = un
        set udg_DeathEvent = 1
        set udg_DeathEvent = 0
        set udg_DeathUnit = backup
    endfunction
You forgot to null the local unit handle.

JASS:
function UnitRevive takes unit un returns boolean
        local integer i = 1
        local unit backup = udg_DeathUnit
        loop
            exitwhen i > DeadUnits
            if DeadUnit[i] == un then
                set DeadUnit[i] = DeadUnit[DeadUnits]
                set DeadUnits = DeadUnits - 1
                call UnitRemoveAbility(un,INVULN)
                call PauseUnit(un,false)
                call SetUnitAnimation(un,"birth")
                call QueueUnitAnimation(un,"stand")
                set udg_DeathUnit = un
                set udg_DeathEvent = 2
                set udg_DeathEvent = 0
                set udg_DeathUnit = backup
                return true
            endif
            set i = i + 1
        endloop
        set udg_DeathUnit = backup
        return false
    endfunction
Same there
 
Level 21
Joined
Mar 27, 2012
Messages
3,232
I've went through the feedback once more and added some things.
However, I noticed that using morphs to remove locust is unacceptable, because it requires an ability for each unit type.
Thus, currently using locust is an option, but it's buggy.
Also, nulled some things.
 
Level 13
Joined
Mar 24, 2013
Messages
1,105
You have to use a morph ability if you want the target-ability back.


Also, Instead of pause unit, maybe use channel with a disable abilities checked a follow thru time of 1 second and just reorder the unit to cast it until they should be respawned.
 
Level 18
Joined
Sep 14, 2012
Messages
3,413
Directly ripped from my fear system :
JASS:
//Credits to Maker for this awesum func <3
    private function DisableControl takes unit u returns nothing
        local boolean b
        call UnitAddAbility(u, 'Aloc')
        call UnitRemoveAbility(u, 'Aloc')
        if IsUnitType(u, UNIT_TYPE_HERO) then
            call UnitAddAbility(u,MORPH_ID)
            call IssueImmediateOrder(u, "metamorphosis")
            call UnitRemoveAbility(u,MORPH_ID)
        else
            call UnitAddAbility(u, BEAR_ID)
            call IssueImmediateOrder(u, "bearform")
            call UnitRemoveAbility(u, BEAR_ID)    
        endif
        //Thanks to chobibo for this idea
        if GetLocalPlayer() != GetOwningPlayer(u) then
            set b = not IsUnitHidden(u)
            call ShowUnit(u,false)
            call ShowUnit(u,b)
        endif
        //I added this line to disable their attack too.
        call UnitAddAbility(u,DISABLE_ATTACK)
    endfunction
    
    private function EnableControl takes unit u returns nothing
        local boolean backup = not IsUnitHidden(u)
        call ShowUnit(u,false)
        //I added this line to enable their attack.
        call UnitRemoveAbility(u,DISABLE_ATTACK)
        call ShowUnit(u,backup)
    endfunction

The disable attack thingy is not important.
I use Morphe for heroes and bear form for units and it works well.
 
Level 21
Joined
Mar 27, 2012
Messages
3,232
Directly ripped from my fear system :
JASS:
//Credits to Maker for this awesum func <3
    private function DisableControl takes unit u returns nothing
        local boolean b
        call UnitAddAbility(u, 'Aloc')
        call UnitRemoveAbility(u, 'Aloc')
        if IsUnitType(u, UNIT_TYPE_HERO) then
            call UnitAddAbility(u,MORPH_ID)
            call IssueImmediateOrder(u, "metamorphosis")
            call UnitRemoveAbility(u,MORPH_ID)
        else
            call UnitAddAbility(u, BEAR_ID)
            call IssueImmediateOrder(u, "bearform")
            call UnitRemoveAbility(u, BEAR_ID)    
        endif
        //Thanks to chobibo for this idea
        if GetLocalPlayer() != GetOwningPlayer(u) then
            set b = not IsUnitHidden(u)
            call ShowUnit(u,false)
            call ShowUnit(u,b)
        endif
        //I added this line to disable their attack too.
        call UnitAddAbility(u,DISABLE_ATTACK)
    endfunction
    
    private function EnableControl takes unit u returns nothing
        local boolean backup = not IsUnitHidden(u)
        call ShowUnit(u,false)
        //I added this line to enable their attack.
        call UnitRemoveAbility(u,DISABLE_ATTACK)
        call ShowUnit(u,backup)
    endfunction

The disable attack thingy is not important.
I use Morphe for heroes and bear form for units and it works well.

The problem is that bearform requires a separate ability for each unit-type. This is simply not acceptable for me.
 
Level 21
Joined
Mar 27, 2012
Messages
3,232
Updated. Function names are changed to make it less likely that they are messed up with what they are not.

Also, I did not do exactly as Mag suggested, because putting the word Pseudo into every function is an easy way towards too-long-function-name-syndrome.
 
Level 19
Joined
Mar 18, 2012
Messages
1,716
constant boolean UseLocust = false should be private.
if UseLocust then --> static if UseLocust then

function names as such are currently horrible.

Mind explaining the use of local unit backup?

You can fake the death of the same unit multiple times, although they are already fake dead.

If you want to take advantage of an Unit Indexer. You could base the index in your stack on UnitUserData aswell.

I certainly don't approve any resouce with
in it.
 
Level 21
Joined
Mar 27, 2012
Messages
3,232
constant boolean UseLocust = false should be private.
if UseLocust then --> static if UseLocust then

function names as such are currently horrible.

Mind explaining the use of local unit backup?

You can fake the death of the same unit multiple times, although they are already fake dead.

If you want to take advantage of an Unit Indexer. You could base the index in your stack on UnitUserData aswell.

I certainly don't approve any resouce with in it.

function names - Mind suggesting better ones? I am out of ideas and wouldn't want to use the smurf naming convention.

The backup is used because of the event. When someone would call any functions of this system from an event, then without a backup the global could change, which would break things unless handled very carefully. Thus, I consider it better to just back up the unit and not give people the need to worry about it.

Killing Multiple times - I expect people to either know what they're doing or check if the unit is in the system (since there is an appropriate native). Thinking about it now, I guess I could also remove it from the other function or make a static if that toggles this kind of safety.

buggy - People requested for locust, which I added. However, I'll remove it since I can't think of any real use.

Anything that I didn't mention will be fixed in next version, which I'll make in an hour or few. (awaiting feedback meanwhile)
 
Level 21
Joined
Mar 27, 2012
Messages
3,232
This isn't true, it can remove half of the locust effect, but you still won't be able to cast targeted unit spells on that unit.
Also, no offense but what's the point of this system? People can easily make units seem dead...

This is for having a simple and standardized way. It also provides events and so far doesn't seem to bug unless you make it use locust.
 

Bannar

Code Reviewer
Level 26
Joined
Mar 19, 2008
Messages
3,140
Wrda is right, to completely remove locust from given unit you have to not only hive/unhide it, but also mix transform/bear ability.

@Xonok hive's jass convention.
Change non constant variable names.

You use GetUnitUserData without any UnitIndexer? Meaby you meant to use it, however, nowhere in script is written that you actually need one. Second, if you want to couple with an indexer, please, dont use GetUnitUserData. Its bad habbit since its not common that systems use many optional directives and/or configurables thus some of those allow you to actually resignate from GetUnitUserData (enabling you to do whatever you wish to do with it) and use hashtable instead. Propper solution: GetUnitId. All of popular indexers contain such function.

And as mentioned already, add requires/uses/needs <any indexer>. You could however, add contant boolean instead, like: private constant boolean UNIT_INDEXER.

Where are linebreaks to separate code and make it actually readable? Snippets before approval need to be tested and moderated yeah? Dont force mods to do this for you or, whats worse, force them to manually manipulate the code. Example:
JASS:
        static if SAFETY then
        if IsUnitDeadC(un) then
        endif

->>>
        static if SAFETY then
            if IsUnitDeadC(un) then
        endif

Next, use inline feature:
JASS:
local integer ID = GetUnitUserData(un)
return UnitDead[ID]

->>>
return UnitDead[GetUnitId(un)]

Remove //- End of API, its pointless, everyone sees thats its an end of api since its an end of whole library. Damn, lol.
 
Level 21
Joined
Mar 27, 2012
Messages
3,232
Wrda is right, to completely remove locust from given unit you have to not only hive/unhide it, but also mix transform/bear ability.

@Xonok hive's jass convention.
Change non constant variable names.

You use GetUnitUserData without any UnitIndexer? Meaby you meant to use it, however, nowhere in script is written that you actually need one. Second, if you want to couple with an indexer, please, dont use GetUnitUserData. Its bad habbit since its not common that systems use many optional directives and/or configurables thus some of those allow you to actually resignate from GetUnitUserData (enabling you to do whatever you wish to do with it) and use hashtable instead. Propper solution: GetUnitId. All of popular indexers contain such function.

And as mentioned already, add requires/uses/needs <any indexer>. You could however, add contant boolean instead, like: private constant boolean UNIT_INDEXER.

Where are linebreaks to separate code and make it actually readable? Snippets before approval need to be tested and moderated yeah? Dont force mods to do this for you or, whats worse, force them to manually manipulate the code. Example:
JASS:
        static if SAFETY then
        if IsUnitDeadC(un) then
        endif

->>>
        static if SAFETY then
            if IsUnitDeadC(un) then
        endif

Next, use inline feature:
JASS:
local integer ID = GetUnitUserData(un)
return UnitDead[ID]

->>>
return UnitDead[GetUnitId(un)]

Remove //- End of API, its pointless, everyone sees thats its an end of api since its an end of whole library. Damn, lol.

I know what it takes to remove locust, but it's not worth it. (which is why I removed it)

The convention is not a part of the rules and thus, I have the right to disagree if I consider my way of doing things better (functionality-wise there is no difference).

I'll write the requirement somewhere.

I do not see how the static if's would work with any proper kind of indentation in this case, so I simply made it as if they are not there. (the other choice would be putting nearly the same code in an ITE, which hurts readability a lot more)

Good point about the inlining. I'll write it down somewhere so that I can fix it once I have more things to change.

The end of API doesn't really hurt anyone. Also, it's a part of the documentation standard that I use. I'll think about changing it, but it's not a real problem.
 
Level 19
Joined
Mar 18, 2012
Messages
1,716
I still don't get the point of the backup unit. In my eyes it is always null.
Aslong as this system is not used udg_DeathUnit is null.
ReviveUnit --> set backup = null --> set udg_DeathUnit = backup.
KillUnit --> set backup = null --> set udg_DeathUnit = backup.

The convention is not a part of the rules and thus, I have the right to disagree if I consider my way of doing things better (functionality-wise there is no difference).
That is not true. The JPAG is part of the spell submission rule set for JASS resources.

SAFETY should cover a debug message, because that's how you handle systems and code. Even most experienced coders make mistakes here and there.

There is no room for shortcuts within the API. Character length is no valid argument, as we already past "ultra-optimizing-for-no-reason-era".
Unless you can convince Mag to relabel RegisterPlayerUnitEvent into RegisterPlayerUnitE I'm not approving this API.

I do not see how the static if's would work with any proper kind of indentation in this case, so I simply made it as if they are not there.
Why should static if's be handled different to normal if then else conditions?

Propper solution: GetUnitId. All of popular indexers contain such function.
Bannar makes a good point, yet I would stay with UnitUserData. That's because not all UnitIndexers share the same API, nor do all list GetUnitId(unit) in their API.
For instance Bribe's unit indexer or AIDS.

Why don't you revive a unit directly via UnitUserData. I don't see a real point for the loop. Add a debug only condition --> toRevive == deadUnit + message to filter out wrong usage or add onDeindex Event.
 

Kazeon

Hosted Project: EC
Level 33
Joined
Oct 12, 2011
Messages
3,449
I still don't get the point of the backup unit. In my eyes it is always null.
Aslong as this system is not used udg_DeathUnit is null.
ReviveUnit --> set backup = null --> set udg_DeathUnit = backup.
KillUnit --> set backup = null --> set udg_DeathUnit = backup.

I also used backup in my chat system, it's not logical.. but it gives the system extra capabilities for certain cases.. you can try to remove the backup in my ucs then you will notice what will happen afterward, the Triggs wont work anymore..

EDIT:
I can explain more about the case if you really need it :)

@Xonok
What is this thing? looks like a bug in my eyes
JASS:
        static if SAFETY then
        if IsUnitDeadC(un)
        endif // <= this endif will close "if IsUnitDeadC(un)" instead of "static if SAFETY then"
            .....
        static if SAFETY then
        endif
        endif
 
Level 21
Joined
Mar 27, 2012
Messages
3,232
I still don't get the point of the backup unit. In my eyes it is always null.
Aslong as this system is not used udg_DeathUnit is null.
ReviveUnit --> set backup = null --> set udg_DeathUnit = backup.
KillUnit --> set backup = null --> set udg_DeathUnit = backup.


That is not true. The JPAG is part of the spell submission rule set for JASS resources.

SAFETY should cover a debug message, because that's how you handle systems and code. Even most experienced coders make mistakes here and there.

There is no room for shortcuts within the API. Character length is no valid argument, as we already past "ultra-optimizing-for-no-reason-era".
Unless you can convince Mag to relabel RegisterPlayerUnitEvent into RegisterPlayerUnitE I'm not approving this API.


Why should static if's be handled different to normal if then else conditions?


Bannar makes a good point, yet I would stay with UnitUserData. That's because not all UnitIndexers share the same API, nor do all list GetUnitId(unit) in their API.
For instance Bribe's unit indexer or AIDS.

Why don't you revive a unit directly via UnitUserData. I don't see a real point for the loop. Add a debug only condition --> toRevive == deadUnit + message to filter out wrong usage or add onDeindex Event.

Basically if you use the event from this system, then you can nest function calls.

You can do something like (pseudocode)

func a
do something, backup is unit1
call function b//changes DeathUnit to something else temporarily and may launch additional events.
do something with unit1, because the global is same as it used to be.
end a

Well, I can change the API back to what it was - simple to understand and verbose function names. But then mag will yell at me because I don't use smurf naming convention.

Good point about the static if's. I checked the actual compiled code and it appears I've misunderstood them greatly.

I'll not put any unit indexer in the requirements unless it becomes an actual problem to someone. GetUnitUserData should work.

Good point about the loop. As it is now, ever since I implemented the boolean array it seems unnecessary to have a stack at all.

About JPAG, I do not see how a convention should ever be compulsory in an environment like JASS. Nothing breaks if I don't use camelcase, etc. This is just personal prefence as long as what I do makes any sense.
I also used backup in my chat system, it's not logical.. but it gives the system extra capabilities for certain cases.. you can try to remove the backup in my ucs then you will notice what will happen afterward, the Triggs wont work anymore..

EDIT:
I can explain more about the case if you really need it :)

@Xonok
What is this thing? looks like a bug in my eyes
JASS:
        static if SAFETY then
        if IsUnitDeadC(un)
        endif // <= this endif will close "if IsUnitDeadC(un)" instead of "static if SAFETY then"
            .....
        static if SAFETY then
        endif
        endif

You are right. In the compiled code, things go much different than they should.
 
Level 21
Joined
Mar 27, 2012
Messages
3,232
I curious, can you give an example where firing this Event is actually useful. After all it's not a big suprise that it does return the unit you just passed into the function.
For me it looks like unnecessary overhead.

Well, there can be various effects that launch when a unit dies, such as reincarnation or damage upon death (both known from melee warcraft III).
Overall, there's nearly as many possibilities for this event as there are for the usual one.
This makes it possible to "kill"/"revive" units from various different triggers without having to keep in mind making effects apply.
 
JASS:
function IsUnitDeadC takes unit un returns boolean
    local integer ID = GetUnitUserData(un)
    return UnitDead[ID] endfunction
-->
JASS:
function IsUnitDeadC takes unit un returns boolean
    return UnitDead[GetUnitUserData(un)]
endfunction

You should indent properly, parts with static ifs are confusing.

I don't even know the static if is even needed. Could you explain, please?

Maybe units should be filtered that you can't add buildings and heros to your system.

In function ReviveUnitC you loop through your whole system until you find matching unit.
Working with hashtable would bring some benefits here.

Is the variable "udg_DeathUnit" needed?

JASS:
set udg_DeathUnit = un
set udg_DeathEvent = 2
set udg_DeathEvent = 0
set udg_DeathUnit = backup
Why you set udg_DeathUnit twice? What are the DeathEvents for?

Before you custom revive your unit there should be a check, if the food limit is reached or not, so no bug occurs.
 
Level 21
Joined
Mar 27, 2012
Messages
3,232
JASS:
function IsUnitDeadC takes unit un returns boolean
    local integer ID = GetUnitUserData(un)
    return UnitDead[ID] endfunction
-->
JASS:
function IsUnitDeadC takes unit un returns boolean
    return UnitDead[GetUnitUserData(un)]
endfunction

You should indent properly, parts with static ifs are confusing.

I don't even know the static if is even needed. Could you explain, please?

Maybe units should be filtered that you can't add buildings and heros to your system.

In function ReviveUnitC you loop through your whole system until you find matching unit.
Working with hashtable would bring some benefits here.

Is the variable "udg_DeathUnit" needed?

JASS:
set udg_DeathUnit = un
set udg_DeathEvent = 2
set udg_DeathEvent = 0
set udg_DeathUnit = backup
Why you set udg_DeathUnit twice? What are the DeathEvents for?

Before you custom revive your unit there should be a check, if the food limit is reached or not, so no bug occurs.

Inlining and the static if problems are in my list of things to fix.

Why would I not want to allow heroes? It makes no sense to me. Likewise, I will not filter for food. That's what the user should think about, as I know plenty of maps where going past the food limit is not a problem.

Looping - Noted. I'll make it use the boolean array only for this in next version, thus deprecating the unit stack completely.

udg_DeathUnit is essentially for events. You can register when it changes to 1 or 2 with an event that launches instantly once that happens (tested). This makes it easier to make things happen upon death, just like a usual death event would.
 
Level 19
Joined
Mar 18, 2012
Messages
1,716
KillUnitC and ReviveUnitC are terrible function names. At least write them out in full. I want to highlight that I'm not willing to approve this snippet, with the current function names.
Like Magtheridon96 I think that pseudo or fake are very fitting prefixes for the system.
:thumbs_down:

debug call DisplayTextToPlayer(GetLocalPlayer(),0,0,"CustomDeath: Checked unit is dead")
This debug line would be much more useful if it either prints index, object id or object name of the unit in question. The same applies in case of the else condition.

static if SAFETY could simplified to static if DEBUG_MODE. After all that's what debug mode is there for.
constant boolean DEBUG_MODE is generated by the JassHelper and set to true if checked in the world editor.
You also save a lot of lines, if you do it like this:
JASS:
static if DEBUG_MODE then
    if IsUnitDeadC(un) then
        call DisplayTextToPlayer(GetLocalPlayer(),0,0,"CustomDeath: Tried to kill a dead unit")
        set backup = null
        return
    endif
endif

I still don't see why someone would want to fire an event upon pseudo-deaths, but that's just my opinion.

Sadly, because of the remaining health bars the system loses a bit of its quality. Messing around with locust surely would spice up the desired effect.

//- End of API Seriously you should remove this line.

You should specify that this system requires an UnitIndexer within the documentation. Another option would be to add the needs/requires/uses keyword with one of the existing UnitIndexer out there.

All over it's a useful system.
 
Level 21
Joined
Mar 27, 2012
Messages
3,232
KillUnitC and ReviveUnitC are terrible function names. At least write them out in full. I want to highlight that I'm not willing to approve this snippet, with the current function names.
Like Magtheridon96 I think that pseudo or fake are very fitting prefixes for the system.
:thumbs_down:

debug call DisplayTextToPlayer(GetLocalPlayer(),0,0,"CustomDeath: Checked unit is dead")
This debug line would be much more useful if it either prints index, object id or object name of the unit in question. The same applies in case of the else condition.

static if SAFETY could simplified to static if DEBUG_MODE. After all that's what debug mode is there for.
constant boolean DEBUG_MODE is generated by the JassHelper and set to true if checked in the world editor.
You also save a lot of lines, if you do it like this:
JASS:
static if DEBUG_MODE then
    if IsUnitDeadC(un) then
        call DisplayTextToPlayer(GetLocalPlayer(),0,0,"CustomDeath: Tried to kill a dead unit")
        set backup = null
        return
    endif
endif

I still don't see why someone would want to fire an event upon pseudo-deaths, but that's just my opinion.

Sadly, because of the remaining health bars the system loses a bit of its quality. Messing around with locust surely would spice up the desired effect.

//- End of API Seriously you should remove this line.

You should specify that this system requires an UnitIndexer within the documentation. Another option would be to add the needs/requires/uses keyword with one of the existing UnitIndexer out there.

All over it's a useful system.

Fixed the things noted. Anything more left?

Eventually I just figured that there is no reason to post debug messages when everything is going right. This simplified the debug behavior a lot and removed the need for this system to be possible to debug separately.

EDIT: Locust is currently unusable, because it can not be reliably removed without a map-specific implementation (morph abilities).
 
Level 19
Joined
Mar 18, 2012
Messages
1,716
Please remove constant boolean safety. It's not used anymore.
I'll approve it now, as I think you will take care of it.

The fact that locust is currently no option leaves a bitter taste.
I hope we can see this feature on future updates.
If so design it as seperate function, because locust units can't be selected, etc....

The code itself is ok and working.
All over this is an useful resource. I used a very similar script once to design a graveyard area with skeletons and ghouls.

The new function names are not what I was hoping, yet acceptable.
Once approved it's very unlikely to change the API. (Extra functionality yes, but renaming no).
 
Top