Faction System

streetpunk · Mar 18, 2017

1.0 Version Released

IcemanBo · Mar 21, 2017

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 · Mar 22, 2017

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 · Apr 5, 2017

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 · Apr 20, 2017

KILLCIDE said:
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:

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 · Apr 20, 2017

streetpunk said:
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.

streetpunk said:
° 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 · Apr 21, 2017

Reventhous said:
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 · Apr 22, 2017

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 · Apr 24, 2017

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 · Apr 24, 2017

streetpunk said:
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

streetpunk said:
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

streetpunk said:
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 · Apr 26, 2017

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

Fixed, now it uses

Set FACSYS_Grouped[FACSYS_TempInteger] = True

KILLCIDE said:
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

KILLCIDE said:
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

Code:

GroupEnumUnitsInRange()

KILLCIDE · Apr 26, 2017

streetpunk said:
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 · Apr 26, 2017

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

KILLCIDE · Apr 26, 2017

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

streetpunk · Apr 26, 2017

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 · Apr 26, 2017

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 · Apr 26, 2017

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 · Apr 26, 2017

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 · Apr 26, 2017

Didn't knew there were so many leaks

IcemanBo · Apr 26, 2017

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 · Apr 28, 2017

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 · Apr 28, 2017

Hotwer said:
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 · Apr 30, 2017

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 · May 3, 2017

KILLCIDE said:
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

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

FACSYS_Loc2 now is set to Position of FACSYS_Unit2

Reventhous · May 3, 2017

streetpunk said:
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 · May 7, 2017

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 · May 8, 2017

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 · May 8, 2017

ZiBitheWand3r3r said:
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).

ZiBitheWand3r3r said:
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!

ZiBitheWand3r3r said:
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!

ZiBitheWand3r3r said:
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 · May 8, 2017

Bribe said:
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.

Bribe said:
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 · May 9, 2017

KILLCIDE said:
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.

KILLCIDE said:
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"

KILLCIDE said:
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".

KILLCIDE said:
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 · May 11, 2017

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 · May 12, 2017

ZiBitheWand3r3r said:
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 · May 23, 2017

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 · May 23, 2017

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 · May 27, 2017

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 · May 28, 2017

Paillan said:
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 · May 28, 2017

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

streetpunk · May 28, 2017

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 · Jun 2, 2017

KILLCIDE said:
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!

Sulfar said:
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 · Jun 15, 2017

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 · Jun 16, 2017

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 · Jun 17, 2017

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.

Faction System

Faction System (Map)

Needs Fixed

Suggestions

Status

Needs Fixed

Suggestions

Status

Needs Fixed

Suggestions

Status

Needs Fixed

Suggestions

Status

Needs Fixed

Suggestions

Status

Needs Fixed

Suggestions

Status

Similar threads