• 🏆 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!

[Snippet] MUI DummyCasters

Level 31
Joined
Jul 10, 2007
Messages
6,306
Coz I want it to be different...

Wanting the back mechanics of it to be different for the sake of being different is not a valid reason. That would be like me submitting a sorting algorithm using the bubble sort method because I want to be different, and that would be insta gy'd because bubble sort is crap.

If you'd prefer, someone else can just rewrite this lib using better algorithms with the same outcomes and submit it and then we can have this one graveyarded. I think a simple cast registration function using a Dummy recycling lib is the best solution. I just might do that ; ).
 
Level 22
Joined
Nov 14, 2008
Messages
3,256
I tried to write something like this once. I failed. But doesn't mean that you will! Although I have no idea what standards like here now, been awhile since I visited this place I'll just point out some personal opinions.

-Why not using Table instead of creating your own Hashtable?
-Why you have your own debug variable when you have the debug keyword?
-The oninit method should be in a module (as I guess Cohadar's JH's initialize order isn't standard yet).
-You mention that one dummy can bug. I wondered why you didn't in your test, move the dummy to the target rather than let it sit which would require the dummy spells having insane missile speed (if it is a missile ofc). And then I do wonder again why you use SetUnitPosition instead of SetUnitX/Y in your code. Yes it's one more call but SetUnitPosition interrupts a unit's order thus I wonder again, your fail-safe thingie especially this bit
cannot be casted
. Can SetUnitPosition be the reason to your problems?

And yes I agree for channeling spells I find such resource as this very useful indeed. Fire Strike, Aerial Shackles and the list goes on.
 
Level 29
Joined
Mar 10, 2009
Messages
5,016
1) Why not using Table instead of creating your own Hashtable?
2) Why you have your own debug variable when you have the debug keyword?
3) The oninit method should be in a module (as I guess Cohadar's JH's initialize order isn't standard yet).
4) You mention that one dummy can bug. I wondered why you didn't in your test, move the dummy to the target rather than let it sit which would require the dummy spells having insane missile speed (if it is a missile ofc).
5) And then I do wonder again why you use SetUnitPosition instead of SetUnitX/Y in your code. Yes it's one more call but SetUnitPosition interrupts a unit's order thus I wonder again, your fail-safe thingie especially this bit
6) Can SetUnitPosition be the reason to your problems?
1) Too lazy for that ATM
2) My taste, you can remove it anyway coz it just test how many dummies created
3) Nothing wrong with that
4) Coz the targets are reachable, so it doesnt bug, that's why I put in the note "it's best that the
hitPoint and hitTarget is reachable"
5) It doesnt matter coz it has no cooldowns everytime it casts (point/backswing is 0)
6) No, coz for example your target is invulnerable, the spell cant be casted to invulnerable units,
therefore you cant END the spell, that's why I created a fail-safe...

EDIT:
Btw this snippet is sitting a long time, is that really hard for this to moderate?...
 
Level 29
Joined
Mar 10, 2009
Messages
5,016
existing resources
which one?

EDIT:
we have seen awesome resources that got deprecated because of much better resources that does the same thing... so I go with what Nestharus is trying to point out, unless it has some advantages over the other library, it might or will never be approved... especially as it's on the JASS section...
INdexing for this is not going to happen...if this is graveyarded Ima post it in the spell's section instead with GUI format...
 

Bannar

Code Reviewer
Level 26
Joined
Mar 19, 2008
Messages
3,140
You focused too much on code size on paper, forgetting that your compiled code is far greater than what you're looking at.

First, you make no use of destroy and create members, thus your struct should extend array. Even if you would, its more efficient to write those yourself, or use parent ones if you extend another struct.

There are multiple snippets in jass resources made to cut overall code size in map and improve both, efficiency and code readability. You don't generate many handles with your hash, so Table support should be implemented at first.
Then, deal with indexing your dummies. Jass resources is visited mostly by experienced users, granted they use UnitIndexers. Protection from indexing your dummies should be added. There are many UnitIndexes, but since Nes' one is the most powerfull, you should at least provide:
JASS:
    static if LIBRARY_UnitIndexer then
        set UnitIndexer.enabled=false
        set dummy=CreateUnit("some shit here")
        set UnitIndexer.enabled=true
    else
        set dummy=CreateUnit("some shit here")
    endif
Next, I belive there is support for RegisterAnyUnitEvent too =)

After comparing this to DummySpellcast and Dummy Caster, I don't see point of using this over any of mentioned ones. Want to add GUI friendly support? Add functions, dont force them to write: YourStruct.yourMethod. Struct and method names should also be improved, look how unpleasant is to type: MuiDummyCasters.hitXXX // that sucks

FoG isn't the best method nowadays for dummy recycling ;/ Better make use of linked list.
Last but not least, lets talk about globals block. Quote from your lib://The DUMMY_CASTER raw ID is the only one you may adjust, NEVER touch the rest! says everything. Whats the reason of providing configurables when you do not want them to be configurable? Instead, you should split your data block into:
JASS:
globals
    // configurables
    private constant player             OWNER = Player(15)
    private constant integer     DUMMY_CASTER = 'xcvb'
endglobals

// some free room to clean the view
globals
    private hashtable                       H = InitHashtable()
    private group                   DummyTake = CreateGroup()
    private unit                        dummy = null
endglobals
Now it clearly shows, what user can touch and what he should not.

DEBUG_MODE - o'rly? Look at right top corner of WE menu -> JassHelper. It contains an option to enable debug mode, so additional stuff is pointless.
 
JASS:
//The DUMMY_CASTER raw ID is the only one you may adjust, NEVER touch the rest!
    private constant player             OWNER = Player(15)
    private constant integer     DUMMY_CASTER = 'xcvb'
    private hashtable                       H = InitHashtable()
    private group                   DummyTake = CreateGroup()
    private unit                        dummy = null
    //===FOR TESTING ONLY ON HOW MANY DUMMIES ARE CREATED:
    private constant boolean DEBUG_MODE = true 
    private integer index = 0

The first green line says that I shouldn't touch the rest, so if I don't want the debug mode enabled, I can't turn it off because I'm not supposed to touch any of the rest...
 
Level 29
Joined
Mar 10, 2009
Messages
5,016
First, you make no use of destroy and create members, thus your struct should extend array. Even if you would, its more efficient to write those yourself, or use parent ones if you extend another struct.
Im not using any destroy/create so yeah you have a point there...

There are multiple snippets in jass resources made to cut overall code size in map and improve both, efficiency and code readability. You don't generate many handles with your hash, so Table support should be implemented at first.
Next, I belive there is support for RegisterAnyUnitEvent too =)
I am cutting all requirements, that's why I didnt use those snippets, but anyway v4.4 is already finished long time ago and I applied the above snippets, just went on vacation before posting it...

Then, deal with indexing your dummies. Jass resources is visited mostly by experienced users, granted they use UnitIndexers. Protection from indexing your dummies should be added.
FoG isn't the best method nowadays for dummy recycling ;/ Better make use of linked list.
Me and Nes already debated that so pls, dont repeat it, indexing is not gonna happen :)...

After comparing this to DummySpellcast and Dummy Caster, I don't see point of using this over any of mentioned ones.
All those dummy casters out there including the above mentioned DOES NOT SUPPORT CHANNELING...

Want to add GUI friendly support? Add functions, dont force them to write: YourStruct.yourMethod. Struct and method names should also be improved, look how unpleasant is to type: MuiDummyCasters.hitXXX // that sucks
It's not gonna bite you (doesnt hurt), so nothing wrong with it my friend...:ogre_hurrhurr::ogre_hurrhurr::ogre_hurrhurr:

Last but not least, lets talk about globals block. Quote from your lib://The DUMMY_CASTER raw ID is the only one you may adjust, NEVER touch the rest! says everything.
The first green line says that I shouldn't touch the rest, so if I don't want the debug mode enabled, I can't turn it off because I'm not supposed to touch any of the rest...
:huh::huh::huh: I thought people here are smart enough that "NEVER touch the rest!" means the 5 declared variables below?...

DEBUG_MODE - o'rly? Look at right top corner of WE menu -> JassHelper. It contains an option to enable debug mode, so additional stuff is pointless.
Most of the time I dont use that, instead doing it my way inside the script. Planning to remove it anyway...
 
Review

  • I think you should call it MUIDummyCasters or MUIDummyCaster.
  • The struct must extend an array because you're not using thistype.allocate() nor are you using this.deallocate(), meaning JassHelper is generating some useless code.
  • The code should be moved outside of a hidden tag and everything should be cleaned up. I want to enforce some formatting rules in the JASS section so everyone can see good-looking code. This may sound pedantic, but I believe there is a difference between a resource that has all documentation contiguous and unaligned and a resource that has nice-looking documentation that just screams:

    C'n'P me now and win a free PS3! Limited Time Offer!

    Point is, you want to sell your code to users. You want to convince them that is a standard resource. For that, the code needs to be perfect.
  • The code needs to be consistent in its conventions. Either all your parameter lists are written func(arg,arg,arg) or they're written func(arg, arg, arg) (I lean towards the latter because it's more readable). Another problem having to do with convention is the inconsistent variable/function naming. Some would wonder why you camelCased everything except for owningplayer.
  • I'd recommend leaning towards using DisplayTimedTextToPlayer for debug messages instead of BJDebugMsg. It's not about performance, it's about presentation. Also, make the debug messages properly cite where they're coming from and make sure the mechanics are flawless (Capital letters used where necessary, punctutation, etc...). The same applies to some of the code comments.
  • I noticed that DummyTake violates the camelCase convention adopted in the rest of the code. This may encourage users to do so too. We need people to stick to certain conventions. If you're going to use one convention per global, all hell will break loose (Same applies to the H hashtable variable).
  • See if you can make this resource extend off of some other Dummy Recycling system (Like Dummy by Nestharus). The more code you can remove from the resource, the better. Really, I find a resource that can do something with very little code quite impressive. Especially if it uses external resources. This makes it more relevant to actual maps. No one would use a resource that has its own dummy recycler when they already have a dummy recycler incorporated in their map. Having little requirements is not ultimately a good thing. Having a lot of requirements should not be viewed as a bad thing because people only import resources once. When they do it for one resource, they don't need to do it for another (Once per map that is).
  • The API listing in the documentation should define the function headers with the types and their names. It's not very obvious what target is (No one should be forced to read your code). target could be a widget for all I know. This allows me to pass in a destructable and wonder why I'm getting a compiler error when I am passing in a valid "target".
  • The hitNone function is not very meaningful. What does it mean to hit nothing?

To wrap it up, you need to improve the API, follow a common convention and be consistent with it, fix the documentation, improve the formatting of the resource overall and fix the aesthetic details and reduce code size by relying on external libraries (Minimalism: Less code is More)​
 
Level 29
Joined
Mar 10, 2009
Messages
5,016
let me rephrase by posting this...

yours doesnt work on this...
JASS:
local real a = 0
loop
   exitwhen a>6.3
   call MuiDummyCasters.hitPoint(Player(0),0,0,300*Cos(a),300*Sin(a),'AHfs',852488,2)
   set spell.abilityId = 'AHfs' //flame strike
   set spell.abilityLevel = 2
   set spell.abilityOrder = 852488
   call spell.castPoint(300*Cos(a),300*Sin(a))
   set a = a+1.04
endloop

mine does...meaning it will create an instance by getting another dummy caster automatically...
JASS:
local real a = 0
loop
   exitwhen a>6.3
   call MuiDummyCasters.hitPoint(Player(0),0,0,300*Cos(a),300*Sin(a),'AHfs',852488,2)
endloop

EDIT:
Ima make a GUI version on this and post on the spell's section...
 
Level 31
Joined
Jul 10, 2007
Messages
6,306
JASS:
/*
MUIDummyCasters v4.4
Created by: Mckill2009

REQUIRES:
- Table by Bribe

INTRODUCTION:
- Creates only 1 dummy for instant spells like firebolt, carrion swarm, banish, etc
- Supports channel spells by creating another dummy to channel the spell like flamestrike, starfall, blizzard, etc
- You may control the point where the dummy is created and cast to a target or location where you can see the projectile is traveling

INSTALLATION:
- Copy ALL that is in the "MUIDummyCasters" folder to your map.
- Copy the Table library to your map.
- Replace the rawID of the DUMMY_CASTER if needed, you may view it via pressing CTRL+D in the object editor.
- DONE!

BUGS:
- Since the dummy cannot move, it's best that the MDCCastToPoint and MDCCastToTarget is reachable, else this will bug.
- The ability casted MUST have 0 cooldown.

API:
    function MDCCast takes player owningPlayer, real xDummy, real yDummy, integer abilityID, integer orderID, integer level returns nothing
    function MDCCastToPoint takes player owningPlayer, real xDummy, real yDummy, real xTarget, real yTarget, integer abilityID, integer orderID, integer level returns nothing
    function MDCCastToTarget takes  player owningPlayer, unit target, real xDummy, real yDummy, integer abilityID, integer orderID, integer level returns nothing

PARAMETEER EXPLANATION:
- owningPlayer (player) - the owner of the casting unit
- target (unit) - the spelltarget
- xDummy and yDummy (real) - the point/coordinate where the dummy will be placed
- xTarget and yTarget (real) - the point/coordinate where the dummy cast the spell
- abilityID (integer) - the raw code of of the ability being cast
- orderID (integer) - the orderID of the abilityID
- level (integer) - the level of the abilityID

CREDITS:
- Bribe (for info about the 'Amov' ability remove)
*/

It's difficult to find anything, yes

And your description of the functions is a list of parameters separated from the functions... : |

Your bugs are included in the docs rather than the thread

Installation should also be in the thread

No easy links to reqs

Then you have a hodgepodge of settings and globals?

JASS:
globals
    //The DUMMY_CASTER raw ID is the only one you may adjust, NEVER touch the rest!
    private Table c
    private constant integer     DUMMY_CASTER = 'h000'
    private constant player             OWNER = Player(15)
    private group                   DummyTake = CreateGroup()
    private unit                        Dummy = null
    //This is just
    private constant boolean TEST = true
    private integer DummyCount = 0
endglobals

I'm going to say this is difficult to read, yes.

edit
Just follow my documentation template man >.>, make it ez on urself. It'll be super ez to follow as u won't have to come up with anything dandy and u'll get a great result that will be easy to read.

http://www.hiveworkshop.com/forums/1876867-post2.html
 
Level 31
Joined
Jul 10, 2007
Messages
6,306
It's not a rule nor is it enforced. Most everyone does their own style, following that template thing ; ). Really, nobody's come up with a better documentation style on this site yet ;o, which is why a lot of ppl use it now =).

It came from my stuff, purge's stuff, and a few other people, taking the best from everybody.


Anyways, I usually follow it pretty closely, but sometimes I get lazy and drop the -'s :\, bad me. Proper parameter descriptions are especially annoying to do, I rarely if ever do those >.<. The spacing is a serious pain, especially when you need to update the description.
 
Top