View Single Post
Old 10-23-2009, 10:32 AM   #16
grim001
requires vJass
 
grim001's Avatar


Code Moderator
 
Join Date: Nov 2006
Posts: 1,540

Submissions (10)

grim001 is just really nice (277)grim001 is just really nice (277)

Send a message via AIM to grim001
Default

Ok, I have spent some time looking at this, and I think it's pretty cool. I will probably use it myself for some things eventually, and not need to finish DamageMod, since I really hate working on that thing.

Questions/Suggestions:
  • I see that you've made improvements in the area of damage prevention. I'm not going to bother to test that it works perfectly, because that is very time consuming, and I'm sure you've tested it quite a lot. Do you know if there's any possible situation where it can still fail, other than receiving damage that exceeds the life bonus value?
  • You don't keep the unit's life proportional when the bonus HP is added, which make life bar flickering an issue in some situations. Is there any way to fix that without introducing other problems?
  • Is it safe to use UnitDamageTarget within the damage detection callbacks? If not, you could provide a safe version.
  • I'd personally prefer if the interface events were named onDamage, onDamaged, it reflects the naming scheme that's been commonly used for events and looks prettier.

Criticisms:
  • You should have the object merger call enabled by default, that is pretty much how all other scripts have been doing it. Also, Earth-Fury recently told me something about how the ObjectMerger works that I have confirmed through testing. If you save the map with the object merger call, disable it, then save the map again, the ability will not exist. You have to run object merger call, then close and re-open the map for it to become permanent. You might want to update your documentation to reflect that.
  • It is nice to provide compatibility, but there is such a thing as going overboard. The number of scripts here is going to confuse and scare away some people. If you think about it, versions of DamageEvent do not need to exist for IDDS and LLDD. People who have those scripts in their library and don't switch to DamageEvent probably prefer that syntax, and it would be lame to use two different methods of damage detection in one map.
  • What is needed instead are IDDS and LLDD versions of DamageModifiers. In this case, you can even use static ifs to make it all into one script, which optionally supports either DamageEvent, IDDS, or LLDD. It would not throw the proper compile error if you lacked any damage detection system, but that is less confusing than the multitudes of scripts that exist right now.
  • Although Table is a little bit of a syntax upgrade for end users, it is not much of a boon in system making, now that it doesn't serve the purpose of consolidating gamecaches. If you turn your table version into a hashtable version, you can then make DamageModifiers use hashtables by default, and optionally utilize AutoIndex if it's in the map.
  • A delay is created the first time the extra life ability is used. You can optionally require AbilityPreload to preload the ability.
  • The final result would be one version of DamageEvent, and one version of DamageModifiers that optionally requires DamageEvent, IDDS, LLDD, AutoIndex, and AbilityPreload.

About using extends:
It is the truth that many people will avoid this system just because they either don't like or don't understand using extends. But aside from that, there is the legitimate problem that you can't extend more than one interface. That means you can't mix your damage/damaged events with structs that extend other interfaces, which would be a very useful thing to do. Imagine mixing those events with a struct extending from a buff system, for example.

It used to be impossible to do anything about that, but now that modules have become more powerful, something can be done. First you would have to port the damage modifiers over to using function interfaces rather than extends, which will be preferable and more understandable for many people to use. It would then be possible to create a module which restores the current functionality, with the added benefit of not needing to extend anything.

Well, how do you port the current functionality to function interfaces? The way you are currently using extends is to provide three things to the user: a way to associate damage/damaged functions with units, a way to associate priorities with those functions, and a way to facilitate attaching data to that specific DamageModifier. So what if you do it this way?:

Expand JASS:

Say you ported it to this setup, how do you restore the current functionality using a module? Well, it would be best to let me handle that, since I am making a system which creates modules for this sort of thing, and supports every approved damage detection system. If you want to discuss how it works you can find me on IRC. But suffice to say it would look something like this for the user:
Expand JASS:
grim001 is offline   Reply With Quote
Sponsored Links - Login to hide this ad!