Illusion System - v1.00e

[Moderator]

12th Dec 2015
IcemanBo: Too long as NeedsFix. Rejected.

24th July 2012
Magtheridon96:

New review

• Your CreateIllusion function can simply return Illusion.create(target, d, h, dur).
• You can totally forget about using a linked list here
  You only need a global variable that gets set to the current instance
  because the function that executes upon summoning the illusion runs directly after the IssueTargetOrder call in the create function.
  Just store 'this' into a global variable and use that global variable inside the 'main' function as if it were this and it would work.
  But, this could bug in rare cases. The safer solution is to use a Table.
  Store 'this' while using the handle id of this.Dummy as the index.
  You would load it in the main function.
• Struct members should be camel-cased. Refer to this tutorial by Bribe to learn more about JASS convention ^.^

Old review

9th Nov 2011
Bribe: Who wrote this ListModule? It fails pretty hard compared to some of our other list modules.

I can't approve this while it requires such a weird resource. You could maybe inline the ListModule yourself and remove the requirement, but its name can easily clash with other superior LinkedList modules.

 

[Bribe]

Debug message should be debug-mode only.

GetSummonedUnit should be GetTriggerUnit

You never remove the dummy you preloaded.

Your GetDummyId is search-based, when it should be UnitUserData based or hashtable based.

You need to link to required stuff.

 

[Robbepop]

okay thank - I will do what has to be done. ;-)

(very nooby from me not to remove the dummy unit :-O)

I will implement the hashbased search later - no time atm.

Robbepop


edit: i really don't know if it will be better for the performance if I chance the search loop to a hashtable key because mostly there are just very few instances of illusion types at the same time ...

 

[watermelon_1234]

The system looks like it works well.

• //Remove this block if you already have got a global dummy unit!

  doesn't really make sense. You should just say that the global should be configured to the dummy id the user uses.
• You could implement some kind of dummy unit recycling.
  When a struct is first used, a dummy unit would be created like normal. However, when the struct is destroyed, hide Dummy somewhere else instead of killing it. Then when that struct gets used again, the Dummy unit would already be there, only needing to be shown and to have an owner change.
• The local variable owner is unnecessary in New.
• onDestroy is kind of bad. Change the name to destroy and put .deallocate() at the end of that method.
• You could do everything in method main in method condition instead.
• I would use a local variable instead of gg_trg_Illusion_Sys so the user won't have to worry about naming the trigger to be "Illusion Sys."
• There's really no point in removing the ability from the dummy unit when you preload the ability. Also, you should just use RemoveUnit instead of KillUnit

I was also thinking that it would be nice if you also allowed the user to use his own custom ability instead of the default ability you provide so that he could use values that are out of the range you specify.

 

[Robbepop]

hiho,

thank you for your time. =)

I go through your points and say sth, k? =)

1. ok, I will do it like that.
2. the dummy is the link to the summoned illusion so I need one dummy for every illusion creation which isn't that bad as I instantly clear up the dummy when I summon the illusion shortly after the dummy creation. =)
3. I got teached that player variables do leak. that's why I always use them to clear them after their usage.
4. also learned onDestroy the way you see it - but I will change it if it is better. =)
5. I like to have a condition and a main. If everythink was in the main I would initialize all variables in the main just to check if the summoned unit is an illusion which sounds bad to me.
6. ok, using a local variable is perhaps better in this situation. ;-)
7. I didn't know that it is enough to just add the ability - thanks.^^

Your last points sounds nice to me but it would be too lavish for normal user to use their own illusion spell object.
However, I could extend my ability to create more factors for you. =)

Robbepop

 

[Bribe]

>> I like to have a condition and a main. If everythink was in the main I would initialize all variables in the main just to check if the summoned unit is an illusion which sounds bad to me.

Then I recommend for you splitting it into two functions, but there is no reason you
should have both conditions and actions assigned to this trigger.

 

[Magtheridon96]

Your GetDummyId is search-based, when it should be UnitUserData based or hashtable based.

Hashtable-based is better
UnitUserData would ONLY work if you have the right filters in UnitIndexer or AIDS

Few things:

• Move the onInit method outside the struct and put it in a module that gets implemented
• You must follow vJass convention ( Struct members are written like this: structMember). Seriously, do it
• Don't use onDestroy, use the destroy method and call .deallocate() at the end
• The spacing and the comments really decrease readability
• Use the private/read only keywords to encapsulate struct members that no one is supposed to change/access.
• The reals x and y in your "New" method are useless..

Oh and Bribe, GetSummonedUnit() isnt the same as GetTriggerUnit
If you check the event in GUI, it says: "A unit spawns a summoned unit"
So technically, the triggering unit is the spawner

 

[Robbepop]

hiho,

what is the sense of moving the onInit method out of my struct and into a module to implement it !? 0.o
hmm, ... did you ever code in C#?
i really like that syntax and the "New" instead of the "create" - that's why my code doesn't look like other vJass codes. :-(
do you really have problems reading this code?
hmm, how could I improve the readability in your opinion?

Thanks so far! ;-)

Robbepop

 

[Magtheridon96]

Well, when it comes to the convention, I feel your pain
C++ convention is like the worst >.<
I've never coded C# though

To improve readability, you could start out by making all the members follow the same convention.

Oh and about the 'moving the method into a module and implementing it' thing, it's actually because what you put in an onInit method inside a module is initialized before any onInit method inside a struct ^^

edit

Much better now, but still they could be improved:

Your entire system could be composed of one function and 6 globals

Instead of registering an event to a trigger that checks when a unit is summoned, why dont you do the actions of that trigger in the function "New"

 

[Robbepop]

Oh and about the 'moving the method into a module and implementing it' thing, it's actually because what you put in an onInit method inside a module is initialized before any onInit method inside a struct ^^

that is really a great thing to know! o.o

Robbepop

 

[Magtheridon96]

library IllusionSystem
/***************************************************
*    function CreateIllusion:                      *
*        takes:                                    *
*            unit target                           *
*            real unitX                            *
*            real unitY                            *
*            real damageFactor                     *
*            real hitFactor                        *
*            real duration                         *
*        returns:                                  *
*            unit                                  *
***************************************************/
    globals
        // The id of the dummy unit in your map
        private constant integer DUMMY = 'n000'
        // The id of the illusion ability
        private constant integer ILLUSION = 'A000'
        // Last created illusion
        private unit l
    endglobals
    
    function CreateIllusion takes unit u, real x, real y, real d, real h, real t returns unit
        local unit v
        set l = null
        if d >= 1. and d <= 3. and h >= 0. and h <= 1. then
            set v = CreateUnit(GetOwningPlayer(u),DUMMY,x,y,0)
            call UnitAddAbility(v,ILLUSION)
            call SetUnitAbilityLevel(v,ILLUSION,R2I(d*11+h*10-10))
            call IssueTargetOrderById(v,852274,u)
            if t>0 then
                call UnitApplyTimedLife(v,'Blil',t)
            endif
            set l = v
        endif
        set v = null
        return l
    endfunction
    
    function GetLastCreatedIllusion takes nothing returns unit
        return l
    endfunction
    
    private module Init
        private static method onInit takes nothing returns nothing
            local unit u = CreateUnit(Player(15),DUMMY,0,0,0)
            call UnitAddAbility(u,ILLUSION)
            call RemoveUnit(u)
            set u = null
        endmethod
    endmodule
    private struct Inits extends array
        implement Init
    endstruct
endlibrary

This is what I would do

edit
Fixed.

 

[watermelon_1234]

2. the dummy is the link to the summoned illusion so I need one dummy for every illusion creation which isn't that bad as I instantly clear up the dummy when I summon the illusion shortly after the dummy creation. =)

Hm, I actually meant something different. I was talking about recycling the dummy units.

Something like this:

            if .Dummy == null then // This means .Dummy doesn't refer to a dummy unit, so create one.
                set .Dummy = CreateUnit(owner, DUMMY_ID, x, y, 0)
                call UnitAddAbility(.Dummy, ILLU_ABIL)                
            else // Just reuse the dummy unit from before                
                call SetUnitOwner(.Dummy, owner, false)
                // I suppose you could also set the dummy unit's coordinates to the target, but you don't really need to.
            endif

You wouldn't kill the dummy unit in the destroy method.

If you check the event in GUI, it says: "A unit spawns a summoned unit"

GUI is dumb and misleading.
GetTriggerUnit is the same, so you should replace GetSummonedUnit with it in the onSummoning method.

You should set trigger t to CreateTrigger on the same line you declare it in onInit.

EDIT:
@Magtheridon96: You don't null u in the onInit method. Or v in CreateIllusion
Your method doesn't even make sense. You're making the dummy unit get treated as the illusion.

 

[Magtheridon96]

Your method doesn't even make sense. You're making the dummy unit get treated as the illusion.

Well, when I tested it, it worked perfectly

 

[watermelon_1234]

^Yes, but it makes the dummy unit that created the illusion to be considered the illusion.

 

[Magtheridon96]

^Yes, but it makes the dummy unit that created the illusion to be considered the illusion.

Well, what fix would you suggest

 

[watermelon_1234]

Um, the system that we're talking about in?
I can't think of any other way to get the illusion besides that. (The only other way might be checking when a unit enters the map but that should be less efficient.)

 

[Magtheridon96]

I'm fixing it. Will post the code here in a minute.

edit
Meh.. Nothing's working correctly..

Suggestions:
1-

    private constant integer ILLU_ORDER = 852274
    private constant integer ILLU_BUFF = 'Blil'

These aren't really configurable, so just inline them
2- Use your own linked list like this:

// Requires:
private static integer array next
private static integer array prev
// How to add to list
set next[this] = 0
set prev[this] = prev[0]
set next[prev[0]] = this
set prev[0] = this
// How to remove from list
set next[prev[this]] = next[this]
set prev[next[this]] = prev[this]
// How to get first element
set ___ = next[0]
// How to iterate
local integer i = next[0]
loop
    exitwhen i == 0
    // ....
    set i = next[i]
endloop

3-
if IsUnitIllusion(GetSummonedUnit()) then -> if IsUnitIllusion(GetTriggerUnit()) then
4- Try to combine functions main and onSummoning
5- Read number 3 and again and just deprecate ListModule
6- Change the function name "New" to "create"
7- Add an API (CreateIllusion and GetLastCreatedIllusion)
8- [highlight]Remove ALL the locals except 'this' from function New and just inline them since they're all used only once[/code]

 

[maddeem]

Why not use the default illusion spell?
What makes this better?

 

[Magtheridon96]

Object editor work sux
With this, you can create an illusion for any unit at any time and anywhere for any duration ^^

 

[Robbepop]

I did some changes and improvements and added the API. =)

 

[baassee]

Create dummy via objectmerger line as I would be too lazy to open a map file for a system (for a spell ok but not for a system even though it's only one ability )

 

[Magtheridon96]

R2I(((dmgfactor - 1) * 11) + ((hitfactor * 10) + 1))

->

R2I(dmgfactor * 11 + hitfactor * 10 - 10)

:3

 

[Kanadaj]

Hmm if it could be done in GUI then why vJASS instead of normal JASS? I don't like messing with JNGP

 

[Magtheridon96]

vJASS is much easier and cleaner
JNGP is just like the normal editor, except for the fact that it doesn't crash when you have syntax errors

 

[xorkatoss]

Object editor work sux
With this, you can create an illusion for any unit at any time and anywhere for any duration ^^

same thing as the mirror image spell does

 

[Magtheridon96]

I know, but this takes advantage of the level of the ability to vary the illusion's stats.

I think this is a very useful system
I'm voting for approval.

 

[maddeem]

I know, but this takes advantage of the level of the ability to vary the illusion's stats.

Normal mirror image does that

 

[Robbepop]

hiho,

yeah but for a normal mirror image you need an ability object for every single mirrir image and with this system you can create different mirror images with one object and one line of code!

besides that it supports GetLastCreatedIllusion(). =)

Robbepop

 

[Magtheridon96]

Seriously maddeem, just accept it
I know you're the kind of guy who judges things by their negatives, but just this once, say "I vote for approval"

This is great because it gives you a way to manage illusions in Jass

 

[Robbepop]

thanks for approving in advance!

but I will still go on fixing and improving the code if you find something else which needs to be done.

Robbepop

 

[Magtheridon96]

There's always room for improvement

Suggestions:

- Use your own linked list!! ( My post on the second page explains how )
- Make this require Table and use a table to store the struct instance with the index of the handle Id of the dummy unit so that you can directly have the instance with a hashtable lookup (With the Table instance)
- Shorten the variable names
- Remove the spacing between each line
- Change "New" to "create"
- Change "Dummy" to "u"
- Change "IllDuration" to "d"
- Follow Jass convention damn it!

 

[Robbepop]

changed made - v1.00e !

 

[xorkatoss]

offtopic: does this spell has anything to do with your goh map?

 

[Robbepop]

sure it does.

I use it for some new and cool assassin spells.^^

Robbepop

 

[Magtheridon96]

- Use your own linked list!! ( My post on the second page explains how )
- Make this require Table and use a table to store the struct instance with the index of the handle Id of the dummy unit so that you can directly have the instance with a hashtable lookup (With the Table instance)
- Shorten the variable names
- Remove the spacing between each line
- Change "New" to "create"
- Change "Dummy" to "u"
- Change "IllDuration" to "d"
- Follow Jass convention damn it!

This... for the third time -_-

 

[Robbepop]

hiho,

why do you dislike the ListModule?
it doesn't make sense to make an own list for the dummy get method if I shall use table to save the units struct instance. 0.o doublesave?

shortened var names just make everything confusing - I like nice-looking codes where I see after a few seconds what the snippet of code does exactly.
with var names like "u", "a" and "i" you first have to find out what they stand for ...

could you please show me where to find the Jass conventions?

Robbepop

 

[Magtheridon96]

Jass convention:


JassFunction
structMember // methods and variables
GlobalVariable
GLOBAL_CONSTANT_VARIABLE
localVariable
functionParameter

Pretty easy

 

[Bribe]

GlobalVariable

That is wrong. It should be globalVariable. In fact, you could follow convention to a T and
say all global variables have to be prefixed by something relevant like vg_global (vJass
global) but that's just asking too much.

 

[Robbepop]

ok, I understand this.
but I still don't get why I should shorten and rename all my variable names to something like "d" or "a". 0.o

Robbepop

 

[Bribe]

The simple answer is don't shorten your variable names.

 

[xorkatoss]

ok, I understand this.
but I still don't get why I should shorten and rename all my variable names to something like "d" or "a". 0.o

Robbepop

maybe easier to locate it?

 

[Bribe]

it's in fact harder to locate it then

 

[xorkatoss]

it's in fact harder to locate it then

really? idk any vJass i just tried to guess lol

 

[Magtheridon96]

You shouldn't shorten your variable names unless you're 100% sure that the reader will know what the acronym stands for
Example: local unit u
I don't think anyone would have a problem figuring out this one xD
Don't shorten ALL your variable names, only the ones that no one's going to care about

 

[maddeem]

Seriously maddeem, just accept it
I know you're the kind of guy who judges things by their negatives, but just this once, say "I vote for approval"

This is great because it gives you a way to manage illusions in Jass

Until vJass ceases to exist I WILL APPOSE IT!
Anyway I really like the "GetLastIllusion" thingy, could possibly be useful. Then again you could do that with normal mirror image. Just have a few lines of code n bam!

 

[Bribe]

I agree with maddeem here, just using the wand of illusion
ability to create illusions seems a lot more logical to me.

 

[Robbepop]

isnt it what the system already does? ~.~