Faction System

[streetpunk]

1.0 Version Released

 

[IcemanBo]

I'm pretty much lost when I open the map as user - no readme or info that tells me what to do.

Why is one trigger called "debug", it doesn't seem like a debug trigger, but like kind of a demo trigger, but also kind of not.

There are memory leaks.

The name conventions don't follow current standards.

I think the onIndex event should be used to register units, so a unit wont be registered multiple times.

Here are common GUI conventions: [GUI] - GPAG - GUI Proper Application Guide
And here some other nice infos, and link about memory leaks: Spell Submission Rules

 

[streetpunk]

Edited map so fit most of the rules I could, still I could use some help on how to make a good and informative statement on "Readme/How to use"

 

[KILLCIDE]

Needs Fixed

• You are lacking a proper map description. Your map description should contain the code, credits, and how to import instructions. You should also have the import instructions inside the map
• Seperate the needed triggers and demo triggers. Since you are lacking a "how to import," it would be really hard to tell what is needed if a user was not experienced
• Loc2 is only created inside an If block, yet you remove the location outside of it. You should be removing the location inside the same block to prevent the chance of removing a null location
• With that in mind, you also use Loc2 inside the unit group enumeration. This creates a location leak since you dont remove the first Loc2 reference
• Loc2 leaks a lot inside that unit group enumeration

Suggestions

• You call the same function multiple times a lot in your code. I recommend you store these into variables and reference the variable instead (ex: Custom value of a unit)
• Periodic trigger should be configurable
• Position of (Picked unit) -> Position of (FACSYS_Unit1)
• (Position of (Picked unit) -> Position of (FACSYS_Unit2)
• Matching unit is a really ugly way of filtering units. I recommend you read Convenient Unit Group Filtering in GUI
• GetUnitsInRangeOfLocAll() has a handle refernce leak that cannot be removed if you use the GUI function
  

Status

Awaiting Update​

 

[streetpunk]

Needs Fixed

• You are lacking a proper map description. Your map description should contain the code, credits, and how to import instructions. You should also have the import instructions inside the map
• Seperate the needed triggers and demo triggers. Since you are lacking a "how to import," it would be really hard to tell what is needed if a user was not experienced
• Loc2 is only created inside an If block, yet you remove the location outside of it. You should be removing the location inside the same block to prevent the chance of removing a null location
• With that in mind, you also use Loc2 inside the unit group enumeration. This creates a location leak since you dont remove the first Loc2 reference
• Loc2 leaks a lot inside that unit group enumeration

Suggestions

• You call the same function multiple times a lot in your code. I recommend you store these into variables and reference the variable instead (ex: Custom value of a unit)
• Periodic trigger should be configurable
• Position of (Picked unit) -> Position of (FACSYS_Unit1)
• (Position of (Picked unit) -> Position of (FACSYS_Unit2)
• Matching unit is a really ugly way of filtering units. I recommend you read Convenient Unit Group Filtering in GUI
• GetUnitsInRangeOfLocAll() has a handle refernce leak that cannot be removed if you use the GUI function

Status

Awaiting Update​

Needs Fixed
° Added the code into map description
° Separated the needed triggers from the useless
° Loc2 is now removed inside the same block it was added

Suggestions
° Made all functions work with variables and reduced the number of "()" to 0
° I need to know what do you mean configurable on the periodic trigger, please.
° Corrected Locations putting them into variables to reffer later
° Corrected Matching Unit issue
° Corrected "Units within range issue", now it uses:

Hidden

• Unit Group Correction
  • Events
  • Conditions
  • Actions
    • Set FACSYS_TempReal = (Current acquisition range of FACSYS_Unit1)
    • Set FACSYS_TempGroup = (Units within FACSYS_TempReal of FACSYS_Loc1)
    • Custom script: set bj_wantDestroyGroup = true
    • Unit Group - Pick every unit in FACSYS_TempGroup and do (Actions)
      • Loop - Actions
        • -------- Actions --------

 

[Reventhous]

I need to know what do you mean configurable on the periodic trigger, please.

Maybe the 0.33s interval and the trigger constants should be configurable.

° Corrected "Units within range issue", now it uses:

I don't think the tempGroup is necessary when

• Custom script: set bj_wantDestroyGroup = true

exists.
It should be like this,

• Custom script: set bj_wantDestroyGroup = true
• Unit Group - Pick every unit in (Units within FACSYS_TempReal of FACSYS_Loc1) and do (Actions)
  • Loop - Actions
  • -------- Actions --------

 

[streetpunk]

Maybe the 0.33s interval and the trigger constants should be configurable.


I don't think the tempGroup is necessary when

• Custom script: set bj_wantDestroyGroup = true

exists.
It should be like this,

• Custom script: set bj_wantDestroyGroup = true
• Unit Group - Pick every unit in (Units within FACSYS_TempReal of FACSYS_Loc1) and do (Actions)
  • Loop - Actions
  • -------- Actions --------

Thanks for pointing that out, but it's always good to minimize the number of "()" present in actions. By the way what do you think about the system ?

 

[KILLCIDE]

Needs Fixed

• TempGroup leaks. set bj_wantDestroyGroup = true needs to be before the unit group is created

Suggestions

• In FACSYS Faction Setup, you store the custom value of the triggering unit into a variable, but you don't use it
• Periodic timeout should be configurable
• Why do you compare the custom value of a unit to the distance to it's target?
• You can store the current order of the unit into a variable instead of constantly calling the function
• Instead of converting the string orders into regular orders all the time, you can just store them into global variables and reference them when you need to
• Units within range has a handle reference leak

Status

Awaiting Update​

 

[streetpunk]

Suggestions:
° In FACSYS Faction Setup I use it (FACSYS_TempInteger) to set the "GroupInteger" variable of the unit itself throught the TempInteger, which is set on the begining of the trigger

• Set FACSYS_GroupInteger[FACSYS_TempInteger] = 1

° The issue with custom value of unit has been corrected and set to "point value" of unit, you need to change the point value of all units you're going to use, in the object editor change the value of the point value of unit to be equal to it's attack range (as described on the instructions).
° "Current order" have been stored
° I don't know if it solves the "Units whithing range" leak problem but I'm now using

• Set FACSYS_TempReal = (Current acquisition range of FACSYS_Unit1)
• Set FACSYS_TempGroup = (Units within FACSYS_TempReal of FACSYS_Loc1)
• Unit Group - Pick every unit in FACSYS_TempGroup and do (Actions)
  • Loop - Actions
    • -------- Actions --------
• Custom script: call DestroyGroup(udg_FACSYS_TempGroup)

Questions:
1) How can I make periodic timeout be configurable?
2) How can I fix the handle reference leak from "Units within range" ?

 

[KILLCIDE]

In FACSYS Faction Setup I use it (FACSYS_TempInteger) to set the "GroupInteger" variable of the unit itself throught the TempInteger, which is set on the begining of the trigger

This line -> Set FACSYS_Grouped[(Custom value of FACSYS_Unit1)] = True

1) How can I make periodic timeout be configurable?

Just make a real variable called PERIODIC_TIMEOUT or something in the setup trigger or a config trigger. Afterwards, use the "Add New Event" function

2) How can I fix the handle reference leak from "Units within range" ?

You don't have to fix it. It's just something I like to point out. If you really want to try to fix it, you have to use GroupEnumUnitsInRange() or GroupEnumUnitsInRect()

 

[streetpunk]

This line -> Set FACSYS_Grouped[(Custom value of FACSYS_Unit1)] = True

Fixed, now it uses

• Set FACSYS_Grouped[FACSYS_TempInteger] = True

Just make a real variable called PERIODIC_TIMEOUT or something in the setup trigger or a config trigger. Afterwards, use the "Add New Event" function

Fixed, now the value is pre-set on another trigger

You don't have to fix it. It's just something I like to point out. If you really want to try to fix it, you have to use
GroupEnumUnitsInRange()
or
GroupEnumUnitsInRect()

Fixed, now I'm using the function

GroupEnumUnitsInRange()

 

[KILLCIDE]

Fixed, now I'm using the function

Ermm I said that units within range leaks, but youre still using it. Actually, now that I look at it, you fill your group up using Units within range, and then you fill it again using GroupEnumUnitsInRange lol. Regardless, this is a tiny issue. I'll review this sometime this weekend.

 

[streetpunk]

How can I fix it so it won't cause any leak?
PS: Thanks for reviewing!

 

[KILLCIDE]

You use any of the GroupEnumUnitsInRange functions instead of "Units within range"

 

[streetpunk]

Ok, so now I'm using the following code for that specific action.

• New
  • Events
  • Conditions
  • Actions
    • -------- -------------------------------------------------------------- --------
    • -------- Checking units whithin range --------
    • Set FACSYS_TempGroup = (Units in (Playable map area))
    • Set FACSYS_TempReal2 = (Current acquisition range of FACSYS_Unit1)
    • Set FACSYS_TempReal3 = (X of FACSYS_Loc1)
    • Set FACSYS_TempReal4 = (Y of FACSYS_Loc1)
    • Custom script: call GroupEnumUnitsInRange(udg_FACSYS_TempGroup, (udg_FACSYS_TempReal3), (udg_FACSYS_TempReal4), (udg_FACSYS_TempReal2), null)
    • Unit Group - Pick every unit in FACSYS_TempGroup and do (Actions)
      • Loop - Actions
        • -------- ACTIONS --------
    • Custom script: call DestroyGroup(udg_FACSYS_TempGroup)

Appearently if I don't set the "FACSYS_TempGroup" var before this action, nothing will happen and no unit will be picked in TempGroup.

 

[KILLCIDE]

I can see that working the first time, but not anytime after that given you destroy the group. Recycle the group instead of constantly creating and destroying it.

 

[IcemanBo]

Note that GroupEnums do clear the group directly before they re-fill it. So you don't need to care yourself to clear it before.
Also btw, more BJs are leaking references, it's pretty uncool to fix them all in GUI.

 

[KILLCIDE]

I told him he didnt have to do it and that I point it out just for the hell of it, but he insists that he does it thats why I always put it under Suggestions and not Needs Fixed.

 

[streetpunk]

Didn't knew there were so many leaks

 

[IcemanBo]

Every function that internaly declares local agents leaks a reference, because none of them are nulled by blizzard. And there are some of them.
It's 'only' a reference leak, mostly, though, which is less important than a object leak: Memory Leaks - but if you personaly care much, then JASS, JASS, JASS. ^^

 

[Hotwer]

Very nice system, I really liked the idea, it sure would come in handy in a lot of situations.

Just two things I want to point out:
1. Why you're using the Value-Point of the units to check it's Attack Range? Isn't there a better method? This 'pre-configuration' sure would waste alot of time if you have alot of unit types to work with.
2. Fix your description, it's treat not threat.

 

[streetpunk]

Very nice system, I really liked the idea, it sure would come in handy in a lot of situations.

Just two things I want to point out:
1. Why you're using the Value-Point of the units to check it's Attack Range? Isn't there a better method? This 'pre-configuration' sure would waste alot of time if you have alot of unit types to work with.
2. Fix your description, it's treat not threat.

I need to use the point value because I could't find a way to get a unit's attack range on a trigger

 

[KILLCIDE]

Needs Fixed

• Why are you filling TempGroup with every unit in the map, and then filling it with units in the area, AND then filtering them out?
  
  • Set FACSYS_TempGroup = (Units in (Playable map area))
  • Custom script: call GroupEnumUnitsInRange(udg_FACSYS_TempGroup, (udg_FACSYS_TempReal3), (udg_FACSYS_TempReal4), (udg_FACSYS_TempReal2), null)

Suggestions

• Use the Index event provided by Bribe to catch when a unit enters a map
• Elapsed game time is 0.00 seconds -> Map Initialization
• Set FACSYS_Loc2 = (Position of (Picked unit)) -> (Position of FACSYS_Unit2)
• It's really pointless to be constantly storing variables like Order(holdposition), Order(stop), and Order(smart) whenever an event occurs. Store them into global variables on map init and reference them instead
• You don't have to empty a group before destroying it

Status

Awaiting Update​

 

[streetpunk]

Why are you filling TempGroup with every unit in the map, and then filling it with units in the area, AND then filtering them out?

• 
  Set FACSYS_TempGroup = (Units in (Playable map area))
• 
  Custom script: call GroupEnumUnitsInRange(udg_FACSYS_TempGroup, (udg_FACSYS_TempReal3), (udg_FACSYS_TempReal4), (udg_FACSYS_TempReal2), null)

Fixed TempGroup issue, now it uses a permanent group on which all units are removed after usage

Set FACSYS_Loc2 = (Position of (Picked unit)) -> (Position of FACSYS_Unit2)

FACSYS_Loc2 now is set to Position of FACSYS_Unit2

 

[Reventhous]

By the way what do you think about the system ?

I haven't test it yet but from description, I see this system have the ability to enhance RPG map gameplay. More faction = better (for me).

Overall, the code looks clean (for now )

 

[KILLCIDE]

Needs Fixed

• Use the index event of the Unit Indexer instead of "A unit enters (Playable map area)." There is a possibility that when users import this into their map, the Faction Setup will run before the unit is actually indexed

Suggestions

• Elapsed game time is 0.00 seconds -> Map Initilization
• I don't know how many times I have to tell this to you, but STORE constant orders (holdposition, stop, smart, attack, etc) into global variables instead of storing it every timeout or ever event run...
• You don't have to empty a group before destroying it

Status

Awaiting Update​

 

[ZiBitheWand3r3r]

Very nice idea! Is this system limited to 2 factions or there may be more?

I just tested demo map, and found a bug: if you make a ghoul enemy of other ghouls and abomination - they start to fight, then when make this ghoul back allied, he stops, but other ghouls continue attacking.
I think @KILLCIDE suggests something like this

• Created
  • Events
    • Game - UnitIndexEvent becomes Equal to 1.00
  • Conditions
    • FACSYS_Grouped[UDex] Equal to False
  • Actions
    • Set FACSYS_Unit1 = UDexUnits[UDex]

Also please consider upgrade from Unit Indexer to "Unit Event" by Bribe
GUI Unit Event v2.2.1.0

 

[streetpunk]

Very nice idea! Is this system limited to 2 factions or there may be more?

The number of factions that can be used on the same player for this system is limited only by the warcraft engine (8190 or something).

I just tested demo map, and found a bug: if you make a ghoul enemy of other ghouls and abomination - they start to fight, then when make this ghoul back allied, he stops, but other ghouls continue attacking.

This is caused because I didn't added a "stop" action on the demo triggers. I will add the "Issue unit to stop" action on the triggers of the Demo category when I upload next, thanks for pointing that out!

I just tested demo map, and found a bug: if you make a ghoul enemy of other ghouls and abomination - they start to fight, then when make this ghoul back allied, he stops, but other ghouls continue attacking.
I think @KILLCIDE suggests something like this

• 
  Created
  • 
    
    Events
    • 
      
      
      Game - UnitIndexEvent becomes Equal to 1.00
  • 
    
    Conditions
    • 
      
      
      FACSYS_Grouped[UDex] Equal to False
  • 
    
    Actions
    • 
      
      
      Set FACSYS_Unit1 = UDexUnits[UDex]

I got it, it's working now! (I tried doing it before with no success, thansk for writing it for me!

Also please consider upgrade from Unit Indexer to "Unit Event" by Bribe
GUI Unit Event v2.2.1.0

Can you please tell me what's the difference between them?

 

[ZiBitheWand3r3r]

Why did I do this? Simplification and efficiency. The current code for Unit Event is about half of what the combined length of the old Unit Event and Unit Indexer were.

I made some big improvements, as well:

UnitIndexEvent Becomes Equal to 2.00 now fires the instant the unit is removed, not after an arbitrary amount of time. It also does not bug when a paused unit is removed from a transport, so I no longer need a timer to detect when they are unloaded.

I also no longer need a timer to detect when a corpse is unloaded from a transport, as I am now using Jesus4Lyf's "enter region" trick to detect when those are unloaded.

I just thought about order attack to unit who is channeling a spell. You may want to not interrupt spells like Starfall, tranquility etc, but you have to track units for 'begin casting / stop casting ability' and set a bolean flag for each unit, but thats up to you if you want to implement such a thing

 

[streetpunk]

Needs Fixed

• Use the index event of the Unit Indexer instead of "A unit enters (Playable map area)." There is a possibility that when users import this into their map, the Faction Setup will run before the unit is actually indexed

It now used the events provided from the Unit Indexer.

Suggestions

• Elapsed game time is 0.00 seconds -> Map Initilization

It now uses the Unit Indexer event instead of "Elapsed game time is 0.00 seconds" and "Map Initialization"

I don't know how many times I have to tell this to you, but STORE constant orders (holdposition, stop, smart, attack, etc) into global variables instead of storing it every timeout or ever event run...

I've pré-set all the order variables in the "FACSYS Pre Setup trigger".

You don't have to empty a group before destroying it

The group isnt being destroyed at all, now I'm using a Permanent Group for the "FACSYS Periodic Checking" trigger.

 

[ZiBitheWand3r3r]

8100 FACSYS_Target_Cleaning groups is not acceptable in my opinion, its better to create / destroy groups dynamically
also make optimalization like (cut some ITE):

• FACSYS Order Check Copy
  • Events
    • Unit - A unit Is issued an order targeting an object
  • Conditions
    • (Issued order) Equal to FACSYS_OrderSmart
    • (Target unit of issued order) Not equal to No unit
  • Actions

better to check some conditions instead of running actions each time any unit is issued order
good luck

 

[streetpunk]

8100 FACSYS_Target_Cleaning groups is not acceptable in my opinion, its better to create / destroy groups dynamically
also make optimalization like (cut some ITE):

• FACSYS Order Check Copy
  • Events
    • Unit - A unit Is issued an order targeting an object
  • Conditions
    • (Issued order) Equal to FACSYS_OrderSmart
    • (Target unit of issued order) Not equal to No unit
  • Actions

better to check some conditions instead of running actions each time any unit is issued order
good luck

Fixed the 8100 groups problem, now it is created and destroyed dynamically:

• Custom script: set udg_FACSYS_Target_Cleaning[udg_FACSYS_TempInteger] = CreateGroup()
• Unit Group - Add FACSYS_Unit1 to FACSYS_Target_Cleaning[FACSYS_TempInteger]

• Custom script: call DestroyGroup(udg_FACSYS_Target_Cleaning[udg_FACSYS_TempInteger])

Updated the Order Check trigger and reduced actions taken:

• FACSYS Order Check
  • Events
    • Unit - A unit Is issued an order targeting an object
  • Conditions
  • Actions
    • Set FACSYS_TempOrder = (Issued order)
    • Set FACSYS_Unit2 = (Target unit of issued order)
    • If (All Conditions are True) then do (Then Actions) else do (Else Actions)
      • If - Conditions
        • FACSYS_TempOrder Equal to FACSYS_OrderSmart
        • FACSYS_Unit2 Not equal to No unit
      • Then - Actions
        • -------- ACTIONS --------
      • Else - Actions

Thanks for pointing that out, you've been very helpful!

I also fixed some "holdposition" order issues and removed a few useless lines, it will work as it should now.

 

[Sulfar]

It sounds great, I would need to implement something like this in my map but since it uses point value its incompatible, I use it for map specific purposes.

 

[streetpunk]

I'll be implementing a hashtable to use instead of point value, but I'll only cover playable races (Human, Orc, Night Elf and Undead), then the user can inject their own unit values if they wish to.

 

[Paillan]

it's worth noting that neutral hostile and extra neutral are natural enemies between each other (they will attack each other and can cast spells on each other)

 

[streetpunk]

it's worth noting that neutral hostile and extra neutral are natural enemies between each other (they will attack each other and can cast spells on each other)

You mind clarifying the meaning? I could't understand it very well.

 

[Paillan]

it means that units controlled by neutral hostile will automatically attack units of extra neutral. The opposite is also true.

 

[streetpunk]

yeah but there the user have only 2 factions to work with, what if he needs more? let's say a RPG map with 30 towns, then the user can make every town have it's own faction: 30 factions across de map. If you wish you can work with neutral hostile, but then you would need to setup player alliances between players and hostiles or the hostile units would auto-attack player units.

 

[streetpunk]

Needs Fixed

• Use the index event of the Unit Indexer instead of "A unit enters (Playable map area)." There is a possibility that when users import this into their map, the Faction Setup will run before the unit is actually indexed

Suggestions

• Elapsed game time is 0.00 seconds -> Map Initilization
• I don't know how many times I have to tell this to you, but STORE constant orders (holdposition, stop, smart, attack, etc) into global variables instead of storing it every timeout or ever event run... -DONE!
• You don't have to empty a group before destroying it -DONE!

Status
Awaiting Update

° Use the index event of the Unit Indexer instead of "A unit enters (Playable map area)." There is a possibility that when users import this into their map, the Faction Setup will run before the unit is actually indexed - DONE!

° Elapsed game time is 0.00 seconds -> Map Initilization - DONE!
° I don't know how many times I have to tell this to you, but STORE constant orders (holdposition, stop, smart, attack, etc) into global variables instead of storing it every timeout or ever event run... -DONE!
° You don't have to empty a group before destroying it -DONE!

It sounds great, I would need to implement something like this in my map but since it uses point value its incompatible, I use it for map specific purposes.

It now uses UNITDATA_Hashtable instead of point value as the previous version would conflict with many systems and maps!

 

[KILLCIDE]

A fairly simple system that can be useful to users. Unfortunately, this system only supports orders like patrol, smart, hold position, etc. It would be great if the system supported spells and such, but I know that would either be impossible or really complicated. Thanks for the well documented code.

Needs Fixed

• Nothing

Suggestions

• Include "How to Import" instructions
• Any triggers you run in the code will be disabled when you import them between maps. I recommend you store the triggers into variables so that it is easier to fix all the disabled actions instead of having to go through the code and re-enable each one.

Status

Approved​

 

[streetpunk]

It is with great joy that I recieve my first approval on the hive, I want to tell that all Suggestions have also been met on the last file I've sent (after the last review). Thanks a lot @Bribe for your Unit Indexer System, thanks @BloodSoul for the "Patrol Object" JASS function, thanks www.hiveworkshop.com, the staff and all users from the forums who helped in a way or another to achieve it. Thanks, @KILLCIDE for the review. This is for everyone, enjoy!

 

[BloodSoul]

A very nice and useful system for mapmakers who use all player slots and still want more in order to distinguish allies from enemies; I've seen a significant amount of users asking for something like this.