Blessed Hammer 1.11

[Moderator]

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

23:54, 23rd Aug 2012
Magtheridon96:

- Spells needs proper documentation
- Use your own global group instead of bj_lastCreatedGroup.

 

[-Derp-]

Hey.

Heeeeeey.

This is your friend.

 

[maddeem]

My bad, I completely forgot to do that. Fixed

 

[zv27]

There is absolutely no need to use two timers (all this can be done with only one timer )
There is no need to use two local groups (simply use one global group and bj_lastCreatedGroup )

All this would make your code much shorter and more efficient

WEAPON_TYPE_WHOKNOWS

->
null

 

[Lambdadelta]

• You should comment your code, include white space and name your variables more meaningfully.
• Speed configuration should be in Warcraft III units/second.
• The unit filter should be at the top of the code where the rest of the configuration is.
• Use only one group instead of creating and destroying them periodically.
• Why not make use of vJass structs instead of a hashtable? It'd probably be a lot more readable.
• Damage parameters (attacktype, damagetype, weapontype) should be constant variables.
• Sound volume should be a constant variable.
• 'uu' doesn't need to be nulled, it's used in FirstOfGroup loop.
• Your grammar in the tooltip? Why capitalised beginning letter for every word?
• You should include the description in your submission post, not just the screenshot.
• As zv27 says, you don't need two timers. One is sufficient.

I'd suggest working on the presentation of your code more than anything else at this stage.

I personally find this spell overly simple and boring, and shouldn't be approved into the spells database. But meh, not my call. Anyway, I have rated it a 1/5.

 

[Magtheridon96]

Your sounds don't play by the way.
It's because you can't play a sound in the same thread that it is created.
Try SoundTools. It's a system that allows you to play a sound anytime you want and it uses a sound recycler to assure that you have as little handles as possible

 

[maddeem]

Your sounds don't play by the way.
It's because you can't play a sound in the same thread that it is created.
Try SoundTools. It's a system that allows you to play a sound anytime you want and it uses a sound recycler to assure that you have as little handles as possible

They do play... o.o

There is absolutely no need to use two timers (all this can be done with only one timer )
There is no need to use two local groups (simply use one global group and bj_lastCreatedGroup )

All this would make your code much shorter and more efficient

WEAPON_TYPE_WHOKNOWS

->
null

I could use 1 timer but then I would have to update the current duration and check to see if it is equal to total duration, which is less efficient, and also would not be exact.
I do need to use at least 1 local group because each hammer can only hit a target once, like in diablo 2.

• You should comment your code, include white space and name your variables more meaningfully.
• Speed configuration should be in Warcraft III units/second.
• The unit filter should be at the top of the code where the rest of the configuration is.
• Use only one group instead of creating and destroying them periodically.
• Why not make use of vJass structs instead of a hashtable? It'd probably be a lot more readable.
• Damage parameters (attacktype, damagetype, weapontype) should be constant variables.
• Sound volume should be a constant variable.
• 'uu' doesn't need to be nulled, it's used in FirstOfGroup loop.
• Your grammar in the tooltip? Why capitalised beginning letter for every word?
• You should include the description in your submission post, not just the screenshot.
• As zv27 says, you don't need two timers. One is sufficient.

I'd suggest working on the presentation of your code more than anything else at this stage.

I personally find this spell overly simple and boring, and shouldn't be approved into the spells database. But meh, not my call. Anyway, I have rated it a 1/5.

I use Hashtables because they are a personal preference. Variables that I want to have names have them, like globals. Anything else is a waste of space. Commenting is pretty useless. Speed and configuration should NOT be in warcraft 3 yards. That would require more math, slowing down the code. I must create and destroy 1 unit group per hammer. I will make sound volume a constant variable.

As for the text be capitals, I was trying to put in the feeling of diablo 2. All diablo 2 text is capitalized.

Edit: Updated Code

 

[zv27]

I could use 1 timer but then I would have to update the current duration and check to see if it is equal to total duration, which is less efficient, and also would not be exact.

FALSE

I do need to use at least 1 local group because each hammer can only hit a target once, like in diablo 2

The same thing you can do with the global group,but after all it is your choice what you gonna do.

 

[maddeem]

FALSE



The same thing you can do with the global group,but after all it is your choice what you gonna do.

I could remove the group but that would require me to store units hit into the hashtable as well. Which is totally fine by me.
For the sake of getting the spell approved I will edit the code to have only 1 timer and no groups.

 

[zv27]

I could remove the group but that would require me to store units hit into the hashtable as well. Which is totally fine by me.
For the sake of getting the spell approved I will edit the code to have only 1 timer and no groups.

It seems that we did not understand.No need to remove the group.I am just saying that instead of the local groups would be better to use a global group because then you would not need the following:

local group g = CreateGroup()
call SaveGroupHandle( HASH, it, 1, g)
call SaveGroupHandle( HASH, ie, 1, g)
local group g = LoadGroupHandle(HASH, i, 1)
call DestroyGroup(LoadGroupHandle(HASH, i, 1))
set g = null



...and one more little thing.

and IsUnitInGroup(uu, g) != true

->

and not IsUnitInGroup(uu, g)


local player p = GetOwningPlayer(u)

->

local player p = GetTriggerPlayer()


if IsPlayerEnemy(p, GetOwningPlayer(uu)) and IsUnitInGroup(uu, g) != true and IsUnitType(uu, UNIT_TYPE_GROUND) and GetUnitState(uu, UNIT_STATE_LIFE) > 0 then

->

Note: there are better things than this GetUnitState( u, UNIT_STATE_LIFE ) > 0 but no matter...

function ExampleYourFilter takes unit u,player p returns boolean
    return IsUnitEnemy( u, p ) and IsUnitType( u, UNIT_TYPE_GROUND ) and GetUnitState( u, UNIT_STATE_LIFE ) > 0    
endfunction

or

function ExampleYourFilter takes unit u,player p returns boolean
    return IsUnitEnemy( u, p ) and not IsUnitInGroup(uu, GLOBALGROUP) and IsUnitType( u, UNIT_TYPE_GROUND ) and GetUnitState( u, UNIT_STATE_LIFE ) > 0    
endfunction

or

function ExampleYourFilter takes unit u, player p, group g returns boolean
    return IsUnitEnemy( u, p ) and not IsUnitInGroup(uu, g) and IsUnitType( u, UNIT_TYPE_GROUND ) and GetUnitState( u, UNIT_STATE_LIFE ) > 0    
endfunction

...or blah,blah

...then for example

if ExampleYourFilter( uu, p ) and not IsUnitInGroup(uu, g) then

...then full version

call GroupEnumUnitsInRange(bj_lastCreatedGroup, GetUnitX(d), GetUnitY(d), HIT_AOE, null)
loop

    set uu = FirstOfGroup(bj_lastCreatedGroup)
    exitwhen null == uu
    call GroupRemoveUnit(bj_lastCreatedGroup, uu)
    
    if ExampleYourFilter( uu, p ) and not IsUnitInGroup(uu, g) then
        // your action here...
    endif

    // or
    if ExampleYourFilter( uu, p )then
        // your action here...
    endif

   //or
   if ExampleYourFilter( uu, p,  bj_lastCreatedGroup )then
        // your action here...
    endif
    
endloop

call UnitDamageTarget(u, uu, dam * UNDEAD_BONUS, true, false, ATTACK_TYPE_MAGIC, DAMAGE_TYPE_NORMAL, WEAPON_TYPE_WHOKNOWS)

->

call UnitDamageTarget(u, uu, dam * UNDEAD_BONUS, true, false, ATTACK_TYPE_MAGIC, DAMAGE_TYPE_NORMAL, null)


local unit d = CreateUnit( Player(0), DUMMY_ID, GetUnitX(u), GetUnitY(u), a)

...should be

local unit d = CreateUnit( GetTriggerPlayer(), DUMMY_ID, GetUnitX(u), GetUnitY(u), a)


This can also be much better and more efficient

private function DAMAGE takes unit a returns real
    return 10 + GetHeroInt(a, true) * .5 + GetUnitAbilityLevel(a, ABILITY) * 8
endfunction

...and this is unnecessary.

set bj_lastCreatedGroup=CreateGroup()

call DestroyGroup(bj_lastCreatedGroup)

Well, that's enough

 

[Magtheridon96]

They do play... o.o

What? ಠ.ಠ

 

[maddeem]

Upgrade complete!
Btw I made Blessed_Hammer a global for the people who use spell event systems

Sigh, 2 ratings of 1. It took me so long to search the diablo 2 MPQ sounds to find them, and to change the icon, and find a model.
As for a spell goes this is pretty solid and bug free. I know the concept is simple but this was intended to be a COPY from diablo 2. And it actually behaves exactly the same.
YOU ARE THE REASONS I STOPPED MAKING SPELLS
NEVER AGAIN SHALL I MAKE A SPELL FOR YOU HIVE

 

[-Derp-]

Just make spells in GUI and you'll find a less egotistical bunch.

As a side note I've rated the spell to 5/5 in hopes of counterbalancing the lower ratings.

 

[zv27]

YOU ARE THE REASONS I STOPPED MAKING SPELLS
NEVER AGAIN SHALL I MAKE A SPELL FOR YOU HIVE

Now, you disappointed me. Life is a bitch but never give up.

 

[Lambdadelta]

Variables that I want to have names have them, like globals. Anything else is a waste of space.

Part of good coding includes good documentation. Whitespace isn't a waste of space as it makes code easier to read. Perhaps in this case, it doesn't make too much difference as your code is quite simple, but it's just a general rule of thumb one should want to follow for when things become ... bigger?

Speed and configuration should NOT be in warcraft 3 yards. That would require more math, slowing down the code.

Efficiency and speed should not the primary concern with spells. A spell that is easy to customize is something I'd prefer anyday over one that is hard to modify.

I will make sound volume a constant variable.

Good to see.

As for the text be capitals, I was trying to put in the feeling of diablo 2. All diablo 2 text is capitalized.

I honestly think that doing so makes you look stupid. It just doesn't work on Warcraft III. Just my thoughts on that, I don't think a change is mandatory here.

Here are my comments on 1.10's code:

• You should set the damagetype and weapontype to constants at the top.
• if IsUnitType(uu, UNIT_TYPE_UNDEAD) then. The units affected by bonus damage should be in a filter such as ValidBonusDamageTarget(whichUnit).
• You call GetUnitX/Y(u) twice in the CAST function.
• As zv27 says, bj_lastCreatedGroup=CreateGroup is not needed.

Btw I made Blessed_Hammer a global for the people who use spell event systems

If you don't mind, could you please tell me some example applications for this?

Sigh, 2 ratings of 1. It took me so long to search the diablo 2 MPQ sounds to find them, and to change the icon, and find a model.

I rated it a 1/5 because it is boring and uninteresting to me. It'll stay a one unless you change the concept. Don't be so dissapointed, what did you expect?

YOU ARE THE REASONS I STOPPED MAKING SPELLS
NEVER AGAIN SHALL I MAKE A SPELL FOR YOU HIVE

If the only things you have to offer are easy-to-create spells that do not distract me from my boredom, for even a second, then I do support you in your decision to stop making spells for the Hive Workshop.

 

[zv27]

Just make spells in GUI and you'll find a less egotistical bunch.

As a side note I've rated the spell to 5/5 in hopes of counterbalancing the lower ratings.

By the way, what is egotistical here?!?!
I see no reason at all for your comments.

For example:

local player p = GetOwningPlayer(u)

->

local player p = GetTriggerPlayer()


local unit d = CreateUnit( Player(0), DUMMY_ID, GetUnitX(u), GetUnitY(u), a)

->

local unit d = CreateUnit( GetTriggerPlayer(), DUMMY_ID, GetUnitX(u), GetUnitY(u), a)

and so on...

As you can see, there is no word here about some kind of egoism,just a bunch of failures.

I just tried to be helpful, not to be an egoist.
As for the author, he was too personally understand some things, so that in future I will never suggest to him anything related to its resources.(NEVER AGAIN!!!)

Anyway, thank you for the categorization.

 

[maddeem]

By the way, what is egotistical here?!?!
I see no reason at all for your comments.

For example:

local player p = GetOwningPlayer(u)

->

local player p = GetTriggerPlayer()


local unit d = CreateUnit( Player(0), DUMMY_ID, GetUnitX(u), GetUnitY(u), a)

->

local unit d = CreateUnit( GetTriggerPlayer(), DUMMY_ID, GetUnitX(u), GetUnitY(u), a)

and so on...

As you can see, there is no word here about some kind of egoism,just a bunch of failures.

I just tried to be helpful, not to be an egoist.
As for the author, he was too personally understand some things, so that in future I will never suggest to him anything related to its resources.(NEVER AGAIN!!!)

Anyway, thank you for the categorization.

These aren't failures. You cannot use GetTriggerPlayer() in the loop. In the cast you can, and I did. I made it Player(0) because there is no need to make it owned by other players. It does not give point value or anything.

You could make them owned by Hostile and it wouldn't matter then.
And I would agree with -Derp-, many who moderate vJass spells have an over bloated ego. You point out the tiniest shit and think its worth making this not a good resource. This is a diablo 2 resource. Things were simple back then. And I don't give two shits if you think this is overly simple. It's designed to be simple.

 

[zv27]

These aren't failures. You cannot use GetTriggerPlayer() in the loop. In the cast you can, and I did. I made it Player(0) because there is no need to make it owned by other players. It does not give point value or anything.

You could make them owned by Hostile and it wouldn't matter then.
And I would agree with -Derp-, many who moderate vJass spells have an over bloated ego. You point out the tiniest shit and think its worth making this not a good resource. This is a diablo 2 resource. Things were simple back then. And I don't give two shits if you think this is overly simple. It's designed to be simple.

At no point did I say that's crap or not good. My only intention was to try to point you to some things , but definitely you're all misunderstood.

It is clear to me that you believe that your coding is perfect, just because it's yours, but believe me not. ( Otherwise you would not be so bitter and angry )

If you feel that this is perfection

"For me this is the biggest shit I've ever seen"
set bj_lastCreatedGroup=CreateGroup()

call DestroyGroup(bj_lastCreatedGroup)

then that's your problem.


Hmm...but it does not matter so much now.What is important is that for me this topic is finished for ALL TIME.

 

[maddeem]

Made minor improvements to the code.

Efficiency and speed should not the primary concern with spells.

You sir, have made my day.

 

[Lambdadelta]

My comments on v1.11a code:

• You don't need two sound locals in the CAST function. Create the first sound then reuse that local.
• Your code is probably sane now. It could do with a few more constants that hold damagetype/weapontype, a filter for the bonus damage units at the top, and comments to explain each constant for people who don't comprehend vJass.

And I would agree with -Derp-, many who moderate vJass spells have an over bloated ego. You point out the tiniest shit and think its worth making this not a good resource. This is a diablo 2 resource. Things were simple back then. And I don't give two shits if you think this is overly simple. It's designed to be simple.

*giggle* That's not very nice. I actually revoke my support for your discontinuing of spell submissions. Although your submissions are likely to be boring and uninteresting, the responses you provide are highly interesting. You now have your own Lambdadelta stalking you, you are interesting.

Hmm...but it does not matter so much now.What is important is that for me this topic is finished for ALL TIME.

Why? For me, this topic is interesting. I don't get why you declare in red that it is finished.

You sir, have made my day.

Thankyou, you also make my day. It isn't everyday you find a thread that doesn't bore you. I ask that you continue to distract me from my boredom, forever, until I get bored. Your replies are interesting, more so then your submission.

---

Hauu auuu~ Bern, are you watching? (It seems I also got some Hanyuu in me)

 

[Magtheridon96]

Don't use bj_lastCreatedGroup, use your own global group.

Trust me, using bj_lastCreatedGroup was a terrible idea. It's just as bad as using (Integer A) in GUI and causes the same problem.

If a group enumeration using the same group were to be executed inside the group enum code, then the first enum would fail.

 

[maddeem]

Holy crap, you guys above me are EVIL.
Alright I'll change it back to a local group T.T
Wait but magtheridon, it only enums once.. o.o

 

[baassee]

Wait but magtheridon, it only enums once.. o.o

Can be other things that enums with the group.