View Single Post
Old 10-23-2009, 03:14 PM   #17
Anitarf
Procrastination Incarnate


Development Director
 
Join Date: Feb 2004
Posts: 8,190

Submissions (19)

Anitarf has a brilliant future (903)Anitarf has a brilliant future (903)Anitarf has a brilliant future (903)Anitarf has a brilliant future (903)Anitarf has a brilliant future (903)Anitarf has a brilliant future (903)Anitarf has a brilliant future (903)Anitarf has a brilliant future (903)

2008 Spell olympics - Fire - SilverApproved Map: Old School Alliance TacticsHero Contest #2 - 3rd PlaceSpell making session 2 winner

Default

Quote:
Originally Posted by grim001
  • 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?
As far as I know, the code accounts for all eventualities (aside for, as you mentioned, the bonus hp ability being inadequate). I haven't tested all possible cases systematically, but I have tested several complicated situations and the code performed as expected (although I had to spend some time figuring out myself what exactly to expect).

Quote:
  • 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?
That's one compromise I had to make in order to ensure that players get bounty correctly whenever the damage is supposed to kill a unit. The system always keeps the unit's life at the correct absolute value, rather than a relative value, to make sure that when the damage resolves, the unit gets killed by the original damage packet no matter how much the damage was increased/decreased; this also simplifies the rest of the code, making it easier to ensure robustness in other cases.

You will only notice the flickering regularly if most of the damage packets in the map are higher than the unit's max hp, which is the only case when the bonus hp ability is used.

Quote:
  • Is it safe to use UnitDamageTarget within the damage detection callbacks? If not, you could provide a safe version.
It is safe. The system can accept additional damage packets both before and after the original damage packet resolves.

Quote:
  • 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.
You're right, I will change this.

Quote:
  • 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.
My, mistake, it was supposed to be enabled but I forgot when copying it to the post. I didn't know about the object merger issues, never experienced the problem. I'll fix that too.

Quote:
  • 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.
Well, my concern are public resources. A user could download two spells, one that uses IDDS and one that uses DamageEvent. From the JESP manifest onward we have been striving to write spells and systems that can be used even by people who lack the skill to modify them, so there's no guarantee that our user could edit either of these two spells so they would use only one DD system.

The idea was that in such cases, DamageEvent could interface with the other damage detection system instead of wasting resources with it's own damage event trigger. I agree that the method of accomplishing this (having three different versions of the library) is awkward and I intend to change it to a single library with optional requirements instead.

Quote:
  • 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.
The problem is that it's DamageEvent that requires DamageModifiers, not the other way around. That's because DamageEvent calls DamageModifiers first, before any other response, and DamageModifiers then returns the modified damage back to the damage detection system. It's a two-way communication initiated by the damage detection system, so DamageModifiers can only work with DamageEvent or a modified third party DD system.

Quote:
  • 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.
Well, I like using Table in lieu of native hashtables because it's a lot more readable, making it easier for me to develop and maintain code. However, having a single version of DamageModifiers instead of two might be worth the change, especially since maintaining a single copy of the library is easier in the long run; I'll think about that.

Quote:
  • A delay is created the first time the extra life ability is used. You can optionally require AbilityPreload to preload the ability.
Agreed, will add that.

Quote:
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.
I think the first statement is a poor argument; it is after all also the truth that many people will avoid this system because it's in vJass and not GUI.

I can see the problem with extends, however, could you not use delegate to work around that? I've seen some people have been enthusiastic recently about the module-based coding style, but I still prefer the multiple struct approach myself. A damage modifier, for example, makes more sense to me as a struct itself (which can then be a member or even a delegate of other structs) than a property/module of any struct. I don't think either approach is functionally superior so mine should be a perfectly valid stylistic choice and I'd like to keep it as it is, unless you disagree.
__________________
Anitarf is offline   Reply With Quote