RPG Threat System v3.2c

[Malhorne]

So any plans on this engine to actually be in GUI? I suggest on doing that as I mentioned. More people will then use this.

By God !
Please stop with this "Please make it GUI" seriously !
The most easy to use is vJASS and it is well-known ><

 

[Daffa]

I dare trying to make this GUI!
Seriously, it really kills time to do such things.

 

[Elprede]

Hmmm... I will see when and where I can use this. This system is niffy and useful in my eyes.

 

[PurgeandFire]

Long-awaited review:

• In filter2:
  not((a2 <= a1 - rad or a2 >= a1 + rad) and (a2 <= a1 - rad - 360. or a2 >= a1 + rad - 360.))
  These statements become a tad confusing as you have more "not"s and parenthesis. Let's cut it down a bit.
  
  not((A or B) and (C or D))
  (not (A or B)) or (not (C or D))
  (not A and not B) or (not C and not D)
  (see De Morgan's Laws)
  
  Then you just evaluate the nots in this case, and you'll end up with:
  (a2 > a1 - rad and a2 < a1 + rad) or (a2 > a1 - rad - 360 and a2 < a1 + rad - 360)
  (if my logic was correct)
  It might be convenient to write a function for that, but it is up to you.
• Using similar logic reduction, this:
  set b = not((p == PASSIVE or p == AGGRESSIVE) or not UnitAlive(u))
  Can become:
  set b = (p != PASSIVE and p != AGGRESSIVE) and UnitAlive(u)
• Likewise:
  set b = not(not IsUnitVisible(u, PASSIVE) and not IsUnitVisible(u, AGGRESSIVE))
  Can become:
  set b = (IsUnitVisible(u, PASSIVE) or IsUnitVisible(u, AGGRESSIVE))
  Useful, huh? But take caution, it is easy to make mistakes. Hell, I might've even made a mistake. So keep a copy of your original code to compare with.
• If I didn't make any mistakes, this should be an equivalent function to "functionHelper". I won't explain these ones since it takes time, but try it out for yourself and see if you get the same reduction!
  
  Code

      private function filterHelper takes nothing returns boolean
      
          local unit u = GetFilterUnit()
          local integer ut = GetUnitTypeId(u)
          local integer dex = LoadInteger(Hashtable, 1911, ut)
          local integer ht = LoadInteger(Hashtable, 7550, dex)
          local integer hand = GetHandleId(u)
          local boolean b = true
          
          set b = (ht != 1 or ut == DummyType) and ht >= 1
          if b then
              set b = (GetOwningPlayer(u) == PASSIVE and not LoadBoolean(Hashtable, 2343, dex))
          endif
          if b then
              set b = not LoadBoolean(Hashtable, hand, 7) or LoadEffectHandle(Hashtable, hand, 8) == null
          endif
          set u = null
          
          return b
      endfunction

• This doesn't make much sense to me:
  

          loop
              exitwhen i > TotalCharacter
              call GroupEnumUnitsInRange(PickGroup, GetUnitX(MainCharacter[i]), GetUnitY(MainCharacter[i]), DISTANCE, function pickFilter)
              set i = i + 1
          endloop

  
  GroupEnumUnitsInRange actually clears units in the group each time you enumerate. So that loop is equivalent to:
  call GroupEnumUnitsInRange(PickGroup, GetUnitX(MainCharacter[TotalCharacter], GetUnitY(MainCharacter[TotalCharacter]), DISTANCE, function pickFilter)
  
  If I'm not mistaken. Although, I don't know if that is the intended result...
• (Optional) It is difficult to read through the onLoop because of the Real[] array. Personally, I prefer locals, even if they bloat the code–they have readability since you can change the names. But that is up to you.
• Make a subroutine for firing the event. Since you repeat this a lot:
  

  set EventUnit = u
  set EventTrigger = EVENT_CREEP_SLEEP
  set EventTrigger = -1
  set EventUnit = null

  Make a function that does it for you!
  

  private function FireEvent takes unit u, integer ev returns nothing
      local unit temp = EventUnit
      //.. add other event params, e.g. distance, etc.
      set EventUnit = u
      set EventTrigger = ev
      set EventTrigger = -1
      set EventUnit = temp
      set temp = null
  endfunction

  Add all the parameters that you might need to edit, and all the variables. If you do this, it will cut down on A LOT of code. That is the beauty of subroutines.
  
  Also, I noticed you were setting the variables, and then setting them back to null or 0. That isn't the proper way to implement a recursive event. You have to use a temporary variable, because it isn't guaranteed that the former value is 0 or null.
• This:
  set Real[3] = GetRandomReal(0., 359.9) * bj_DEGTORAD
  ->
  set Real[3] = GetRandomReal(0., 2*bj_PI)
• This:
  

  if COLOR then
      call SetUnitOwner(u, AGGRESSIVE, false)
  else
      call SetUnitOwner(u, AGGRESSIVE, true)
  endif

  Can be:
  

  call SetUnitOwner(u, AGGRESSIVE, not COLOR)

  (same for the other one)
• About this:
  

  // Evade critical bug (everlasting combat duration where it's not supposed to)
  if Real[0] == 0. then
      set Real[0] = -1.
  endif

  This won't really be needed if you change the exit condition appropriately:
  

  if Real[0] <= 0. or not UnitAlive(LoadUnitHandle(Hashtable, hand, 6)) then

  (I changed Real[0] < 0. to Real[0] <= 0.)

• loop
      // Continue the loop ala total help hasn't reach the max
      if ht < ix then
      // ... a bunch of code
      endif
  endloop

  You have two of those. They look exactly the same to me, so you should put it in a separate function. It'll cut down on a lot of code as well. DRY principle.
• In "setAlliance":
  

  if p != PASSIVE then
      if p != AGGRESSIVE then
          // ... code
      endif
  endif

  That is equivalent to: if p != PASSIVE and p != AGGRESSIVE then
• You have a few leaks in "ini":
  GetWorldBounds()
  That actually generates a new rect each time. Set a global to WorldBounds(), and then use it over and over. Or you can start a timer to fire after 0 seconds, and use bj_mapInitialPlayableArea instead (if that works for your fog modifiers).
• Why public?
  public function initPick takes nothing returns nothing
• (Optional) The code is difficult to read with the random keys. I recommend using global constants for the keys so that the code will be comprehensible.

Overall, this system performs the job it needs to do. The main things that need improvement: repeated code, reduction, readability, and comprehensibility. But don't be worried! Your coding is constantly getting better. And I know it is a lot to read/fix, but hey, the code was a lot to review in itself!

 

[Kazeon]

Pheww, 'bout time.

As have been promised, I hope this is approvable now. Since it took weeks to update it to this version, and I forgot to write all of changed logs so I can't provide the changelog in this version.

 

[PurgeandFire]

Some people have been requesting well-defined reviews. I'll try my best to accommodate
for that. The format, calibration, and scaling is still WIP. But overall I'm pretty happy
with it. Note: the scores are not final, so don't freak out! A lot of the deductions
are correctable. PM me or VM me for any questions, comments, or responses.

General

Coding

Concept

Design

Misc

Docs

Required Changes

Purgeandfire :: 29 October 2014

Criteria (Systems):

• Coding (20): efficiency and potential improvement.
• Concept (10): originality and features.
• Design (20): modularity, importability, ease of use, and approach.
• Misc (10): test map, image, hive submission, misc.
• Docs (10): documentation

Score:

• Coding: 10 / 20
• Concept: 7 / 10
• Design: 14 / 20
• Misc: 8 / 10
• Docs: 5 / 10
• Total Score: 44 / 70

Needs Fix :: see Required Changes for approval.

3 / 5

Score Distribution

• 65 - 70 becomes 5 / 5
• 55 - 65 becomes 4 / 5
• 40 - 55 becomes 3 / 5
• 30 - 40 becomes 2 / 5
• 0 - 30 becmes 1 / 5

Past Reviews

1. http://www.hiveworkshop.com/forums/2589556-post54.html

GetWorldBounds() leaks. You need to store it in a variable and then remove it manually (2 occurrences). [-3] Just a note, adding [13] does not do anything for the following line: private trigger array Handler[13] It does not change anything unless you use a size > 8192. filter2 should have a better name. [-1] In pickHelper, if I'm not mistaken, it seems like you are trying to choose the closest units and order them to help. In this case, it may be better to take advantage of Spinnaker's system and use: GetClosestNUnitsInRange It is designed to do what you need to do, and it will result in a significantly lighter computation. Note: You will have to change your entire loop to adapt to this change. [-2] This: set Real[R_TEMP_FACE] = GetRandomReal(0, 359.9) * bj_DEGTORAD Is equivalent to: sett Real[R_TEMP_FACE] = GetRandomReal(0, 2*bj_PI) Or you can use 6.28319 instead of 2*bj_PI. In onHit, you can move the assignment of 'dex' outside the blocks. (saves 1 line) In onDeath, you need to set u to null. [-1] If you really wanted to be extreme, you could redo this entire system using structs instead of hashtables. > [-2] Why use a trigger action for initUnit? (use a condition) [-1] Score: 10 / 20

1. Cool concept! I love the diverse features! It really shows effort.
2. Creep engines are quite common though.
3. It could be cool to have creep-specific modularity, e.g. creep
   specific sleep durations, so forth.

Score: 8 / 10

The module design is not very good. Generally, the best design is where a user does not have to touch the system at all, and only needs to know its functions. Generally, it is better to separate example code from the library itself (usually in a different trigger). A second design flaw is that the data is all expected upon initialization. The setup functions have too generic names, e.g. ResetTimer or LowBound To fix issues 1-3, I recommend translating your setup over to a struct and adding a .register() method so the user can invoke registering a creep. struct SpecialCreep extends array private static group cg = CreateGroup() private static integer uid = 0 method operator tamed= takes boolean tame returns nothing call SaveBoolean(Hashtable, P_TAMED_BOOL, this, tame) endmethod method operator sleepDuration= takes real duration returns nothing call SaveReal(Hashtable, P_SLEEP_DUR, this, duration) endmethod // etc... private static method registerCreeps takes nothing returns boolean local unit f = GetFilterUnit() if GetUnitTypeId(f) == thistype.uid then // ... actions to register creeps ... // uid -> rawcode of creep being registered // f -> creep being registered endif set f = null return false endmethod method update takes nothing returns nothing local rect r = GetWorldBounds() set uid = this call GroupEnumUnitsInRect(cg, r, Filter(function thistype.registerCreeps)) call RemoveRect(r) set r = null endmethod static method get takes integer raw returns thistype return raw endmethod endstruct This may be a little advanced. In this situation, we are treating the raw ID as the struct itself. With this, we do not even need to map the rawcodes to integers 1, 2, 3... etc. We can just use the rawcode for everything! In case this was confusing, maybe this will help: local SpecialCreep murloc = SpecialCreep.get('nmrl') set murloc.tamed = false set murloc.sleepDuration = 7.25 set murloc.sleepTime = 21.0 // etc. call murloc.update() Note: my code above would change a lot of the system. I recommend making these changes only if you understand the code 100%. But the advantage is it would allow you to update unit stats at any time by using .update()! That is awesome modularity. When you order a unit to patrol (via the MOVE order), won't it ignore enemy targets? Score: 14 / 20

1. The test map should have instructions on what to do, or some
   form of debriefing. It can either be in a message, a quest, etc. [-2]

Score: 8 / 10

Needs documentation. It is preferrable to have it at the top of the code, and have a list of the functions that should be used. Ideally, a user should not have to look through your system at all in order to use your system. Add some descriptions to functions and such. See the JASS section for examples. It will make your code a lot more user-friendly. [-5] Score: 5 / 10

• Coding: #1, #4 (or explain if I read your code wrong), #7, #9
• Design: #5 (need a response)
• Misc: #1

 

[Kazeon]

Thnks, but don't say this thing again to me

the scores are not final, so don't freak out!

 

[Deleted member 219079]

"When you order a unit to patrol (via the MOVE order), won't it ignore
enemy targets?" Yeah it will, that might have been intentional too?

That effort and amount of bbcode for a review though

But please don't stress yourself, I would feel quilty for it :/

Edit:

If you really wanted to be extreme, you could redo this entire
system using structs instead of hashtables. > [-2]

I totally agree with this, if you try to avoid the 8190 instance limit of structs, how could there be so many creeps

Also this is a nice system overall, +rep.

 

[Kazeon]

I totally agree with this, if you try to avoid the 8190 instance limit of structs, how could there be so many creeps

Yup. I'm working on it. Cz I want to be extreme >

Anyway, well-defined doesn't mean your review should be heavily BB-coded. Just give a clean explanation about every point and I think it's good enough.

"When you order a unit to patrol (via the MOVE order), won't it ignore
enemy targets?" Yeah it will, that might have been intentional too?

I don't clearly understand that part. Do you mean ordering player units or creep units?

 

[PurgeandFire]

Lines like this:

call IssuePointOrderById(u, MOVE_ORDER_ID, tx, ty)

For wandering creeps. If creeps are ordered to move (for wandering), won't they ignore targets approaching?

Anyway, well-defined doesn't mean your review should be heavily BB-coded

Yep, it doesn't have to be. But I think it leads to a better organization than one massive review. This way, it is a lot less overwhelming. And it doesn't take very long to make once you have a base template. I just write python scripts to automate the process, paste my feedback in, and the script takes care of the formatting.

 

[Kazeon]

Updated. May I jump to v3.0? :3
Well, the sleep thingy was so nasty to handle. But finally it seems to work correctly now.

Okay, now it's time to answer your reviews

Lines like this:

call IssuePointOrderById(u, MOVE_ORDER_ID, tx, ty)

For wandering creeps. If creeps are ordered to move (for wandering), won't they ignore targets approaching?

No, it's still acquiring targets even while wandering.

In pickHelper, if I'm not mistaken, it seems like you are trying to
choose the closest units and order them to help. In this case, it may
be better to take advantage of Spinnaker's system and use

I don't know how to use it, so I will just stay with my way.

Creep engines are quite common though.

But I haven't seen any :/ If you meant AI system, yes, there are plenty of them

Needs documentation. It is preferrable to have it at the top of
the code, and have a list of the functions that should be used.
Ideally, a user should not have to look through your system at all
in order to use your system.

Add some descriptions to functions and such. See the JASS section
for examples. It will make your code a lot more user-friendly.

Don't worry, there is README thingy there

 

[IcemanBo]

onHit?:

// If the creep is currently sleeping
if Data[uDex].isSleep then
    call DestroyEffect(Data[uDex].sleepSfx)
    set Data[uDex].sleepSfx = null
    set Data[uDex].isSleep  = false
    call fireEvent(EVENT_CREEP_AWAKE, t, null)
endif

The sleep timer needs to be destroyed/released, if the creep is currently sleeping onHit.


No need to null h in function pickhelper.

// If true, creeps will use PASSIVE's player color
private constant    boolean     COLOR               = true

^That's a bad name for a boolean.

//"sleepDur" "real" Sleep duration (in hour)
Making it possible to depent it on real time would probably be useful, too.

//"nestArea" "real" Nest area AoE
Should have better explaination.

 

[Kazeon]

The sleep timer also needs to be destroyed/released, if the creep is currently sleeping onHit.

no

^Where does TempUnit come from? ... I know it's set before you call the function, but why don't you use a local and take it as argument?
Global boolean Move is not needed. It's used to make such things with locals.
Global integer DumyType is not needed. It's used to make such things with locals.

I use global because the function can't take arguments/parameters.

I don't care about the other ones.

And please, don't touch my stuffs. You have no idea how hard I worked on them. Don't ruin it.

 

[IcemanBo]

Ah, yes it was used as code. I have removed that point, and will re-check it some later.

 

[chobibo]

Why not recycle/destroy the timer? Wouldn't that leak the timer if you didn't?

 

[Kazeon]

Because after get hit, the creep supposed to get back to sleeping state, the timer is still running. It will be recycled correctly on wake up time.

 

[chobibo]

Oh, I see. Thanks for the clarification.

 

[Kazeon]

my pleasure

 

[IcemanBo]

But sleeping unit gets attacked -> dies -> no recylce will happen. At least I can't see it in code. Enlighten me.

If unit is alive, timer will be released properly, I can see that.

 

[Kazeon]

But sleeping unit gets attacked -> dies -> no recylce will happen. At least I can't see it in code. Enlighten me.

If unit is alive, timer will be released properly, I can see that.

Nice catch! Will be updated not soon.

 

[IcemanBo]

Thread opened.

 

[bimmy]

Do i need to change something after pasting the triggers in my map? aside from changing the units in the Demolib.

 

[Kazeon]

You just need to follow the instruction:

    *  Implementation:                                                                  *
    *     - Copy paste SCE folder into your map                                         *
    *     - Give two player slots for passive and aggresive player, better just         *
    *        leave'em as closed slot                                                    *
    *     - Configure the system                                                        *
    *     - Done.

 

[Arad MNK]

Whatever I do the creeps don't start wandering (Yes I've registered them and set the method calls correctly). Also after I attack the creeps, and I run/die/whatever, the creeps don't change from AGGRESSIVE to PASSIVE (they do change from PASSIVE to AGGRESSIVE).

 

[Kazeon]

Hey, I'm sorry for very late reply. I thought I have replied to your post.

Can you provide me the map? Because I didn't see any problem when the last time I used this system on my Guild Master project.

 

[Elprede]

So... What is new?

 

[Kazeon]

Nothing...
I only renamed the system and improved the documentation so it's more understandable. I think the system offers enough features already, or I'd say, a bit too much.

 

[Strikest]

Hey, pretty cool system! I want to use this in my project, but I'm using AIDS in it. Is there a way to use this system with a different unit indexer?

 

[Kazeon]

Hey, pretty cool system! I want to use this in my project, but I'm using AIDS in it. Is there a way to use this system with a different unit indexer?

Of course, all needed to change is this line:

call RegisterUnitIndexEvent(Condition(function onIndex), UnitIndexer.INDEX)

And some parts of onIndex function. If you can link me to your indexer code I can help you to adjust the system.

 

[Strikest]

That'd be cool, I'm using Jesus4Lyf AIDS: System - Advanced Indexing & Data Storage