• 🏆 Texturing Contest #33 is OPEN! Contestants must re-texture a SD unit model found in-game (Warcraft 3 Classic), recreating the unit into a peaceful NPC version. 🔗Click here to enter!
  • 🏆 Hive's 6th HD Modeling Contest: Mechanical is now open! Design and model a mechanical creature, mechanized animal, a futuristic robotic being, or anything else your imagination can tinker with! 📅 Submissions close on June 30, 2024. Don't miss this opportunity to let your creativity shine! Enter now and show us your mechanical masterpiece! 🔗 Click here to enter!

A major issue with FirstOfGroup iterating method

Status
Not open for further replies.

~El

~El

Level 17
Joined
Jun 13, 2016
Messages
558
I haven't seen this being mentioned anywhere else, so here it is.

When working on my map, I was having some issues with groups. In particular, I was using the FoG method to iterate over groups. If you don't know what it is, consider the following:

JASS:
// zinc code, but you should get the idea

function FoG(group g) {
        unit fog = FirstOfGroup(g);
     
        while (fog != null) {
            // some action with the unit
            GroupRemoveUnit(g, fog);
            fog = FirstOfGroup(g);
        }
}
It has it's own issues, but they are not relevant here. I was using groups across time, i.e. I was saving some units in a group, and using them later on for something else.

However, I discovered a very troublesome bug when using FoG in this manner.

If you have units in a group, and then one of these units is removed some time later, then the FoG method will not iterate over the entire group anymore. In particular, any units that were added to the group after the removed unit will not be iterated over.

Using ForGroup works fine, but FoG does not.

This code replicates the issue at it's finest:
JASS:
//! zinc
library Test {
    function printGroup_aux() {
        unit u = GetEnumUnit();
     
        BJDebugMsg(GetUnitName(u));
    }

    function printGroup(group g) {
        BJDebugMsg("------ForGroup Test------");
        ForGroup(g, function printGroup_aux);
        BJDebugMsg("------ForGroup TestEnd------");
    }
 
    function printGroup2(group g) {
        unit fog = FirstOfGroup(g);
     
        BJDebugMsg("------FoG Test------");
     
        while (fog != null) {
            BJDebugMsg(GetUnitName(fog));
            GroupRemoveUnit(g, fog);
            fog = FirstOfGroup(g);
        }
     
        BJDebugMsg("------FoG TestEnd------");
    }

    function onInit() {
        group g;
        unit u1;
        unit u2;
        unit u3;
        unit u4;
        unit u5;
        player p = Player(0);
     
        g = CreateGroup();
     
        u1 = CreateUnit(p, 'hfoo', 0, 0, 0);
        u2 = CreateUnit(p, 'hfoo', 0, 0, 0);
        u3 = CreateUnit(p, 'hfoo', 0, 0, 0);
        u4 = CreateUnit(p, 'hfoo', 0, 0, 0);
        u5 = CreateUnit(p, 'hfoo', 0, 0, 0);
     
        GroupAddUnit(g, u1);
        GroupAddUnit(g, u2);
        GroupAddUnit(g, u3);
        GroupAddUnit(g, u4);
        GroupAddUnit(g, u5);
     
        RemoveUnit(u4);
        TriggerSleepAction(4);
     
        printGroup(g);
        printGroup2(g);
    }
}

//! endzinc
After removing u4, u5 will not be iterated by the FoG method, while still showing up in ForGroup.
If you remove u2, then u3,u4,u5 will not be iterated over.
If you remove u1, then no units will be iterated over, while ForGroup will still work correctly.

From this, we can conclude that FirstOfGroup uses some sort of linked list, and works differently from how ForGroup operates. After removing a unit, the linked list isn't properly restored, and thus, FirstOfGroup stops operating correctly.

This invalidates the use of the FoG iterating method for groups that are stored across time. It is still fine for cases where you create a group and you can reliably expect no units in the group to die/be removed.

P.S. Now, I am not saying this isn't impossible to circumvent, but this is something that should be considered either way.
 
Last edited:

~El

~El

Level 17
Joined
Jun 13, 2016
Messages
558
This has been known for years. That information is pretty much in all FoG tutorials and the reason why GroupUtils' refresh function exists.

That's why we only use FoG in immediate enumerations and use ForGroup for permanent groups.

I see. Somehow, I missed that, and it bit me on the ass. My bad.

Still, would be nice to see it fixed in a patch. A man can dream.

P.S. Thanks for letting me know about GroupRefresh, though, it wasn't clear to me what it was being used for.
 
Level 13
Joined
Nov 7, 2014
Messages
571
It begs the question of why FirstOfGroup doesn't do whatever ForGroup does when it finds a "removed" unit.

I would assume the answer is "speed"... but perhaps it's better not to remove units without removing them from their owning groups in the first place, rather than doing "group refreshing".

It seems that in order for a unit to automatically clean after itself upon removal it would have to store all the groups it has been added to... 1 more reason for a Unit/Group structs? =)
 

~El

~El

Level 17
Joined
Jun 13, 2016
Messages
558
It begs the question of why FirstOfGroup doesn't do whatever ForGroup does when it finds a "removed" unit.

I would assume the answer is "speed"... but perhaps it's better not to remove units without removing them from their owning groups in the first place, rather than doing "group refreshing".

It seems that in order for a unit to automatically clean after itself upon removal it would have to store all the groups it has been added to... 1 more reason for a Unit/Group structs? =)

I have, in fact, implemented a Unit/Group struct for use in my map which basically replicates normal groups and allows FoG-style iteration using lists. It keeps track of removed units and correctly adjusts the groups when a unit is removed so that integrity is conserved. However, I am in the process of transitioning to using ForGroup, for various reasons, but I can post what I had.
 

~El

~El

Level 17
Joined
Jun 13, 2016
Messages
558
Why? I find the callback iteration very unappealing, because you lose all the local context of the function your calling it from and it's slower (even if it isn't, I still don't like it =)).

I wouldn't switch if there was no ops limit in JASS, but there is. As it stands right now, I sometimes might need to iterate over very large sets of units (>500), which causes the callback-less versions to crash. Sure, this can be circumvented with TriggerEvaluate/TriggerExecute, but then the benefit of retaining local variables is lost.
 

~El

~El

Level 17
Joined
Jun 13, 2016
Messages
558

This is pretty much what I had implemented, albeit slightly more lightweight.

The jass vm can handle several hundred of simple loop iterations before "crash".

Could you give an example where you need to iterate over a such large number please ? (i'm just curious)

I am making an RP sandbox map and I want to add support for global commands (i.e. running a command on all your units, etc.), and when you have a ton of units (and that can happen with very complicated builds with a lot of details easily), it would crash. I would prefer my commands to be robust and be able to handle any number of units (especially since some commands consume more ops per unit and will, therefore, crash on smaller numbers) rather than be very fast. I don't want to mess with TriggerExecute/TriggerEvaluate to restart threads either, because it very quickly turns the code into a bloody mess.
 

~El

~El

Level 17
Joined
Jun 13, 2016
Messages
558
I suppose it's fine, unless all these "new" threads opened cause lags.
Then, you would considering splitting your code and using a dummy ForForce (funny here) to reset the limit op.

I just hope that one of the upcoming patches will introduce a way to reset or increase the ops limit without having to resort to all these hacky methods that involve starting new threads and whatnot.
 
Level 14
Joined
Jul 1, 2008
Messages
1,314
Sry, I do not want to hijack your thread, I just wanted to add details to what has been said by Zwiebelchen: You can rescue your demo-code with GroupRefresh(g) by Rising Dusk from Wc3net, which is basicly this:

JASS:
  globals
      private boolean Flag = FALSE
      private group Refr = null
  endglobals

  private function AddEx takes nothing returns nothing
      if Flag then
        call GroupClear(Refr)
        set Flag = false
     endif
     call GroupAddUnit(Refr, GetEnumUnit())
  endfunction

  public function GROUP takes group g returns nothing
       set Flag = true
       set Refr = g
       call ForGroup(Refr, function AddEx)
       if Flag then
          call GroupClear(g)
       endif
  endfunction
// http://www.wc3c.net/showthread.php?t=104464
I inserted GroupRefresh right after the 4 seconds wait and before the print out, and it rescued all 4 footmen from the group.
 
  • Like
Reactions: ~El
Level 26
Joined
Aug 18, 2009
Messages
4,097
I am not much a fan of FoG iteration either. The local variables are not that much of a problem, could be solved by language. When function bodies become larger, they should be outsourced anyway and that's where the op limit proves a threat. The problem is that you do not necessarily know the load of the body as that may change later on/be dynamic when calling custom functions. A flexible iteration size is of course unsettling too but most likely you will have/want an iterator behavior where that's set beforehand and thus apply optimizations. The op limit introduces a not fully statically-analyzable problem, tracking and automatically scheduling threads with the required actions seems too expensive, jumbles up the whole code output and may hit other limits there. So that's always been a rough estimation there.
 
can you name them? i've never met such tutorial, neither had idea about mentioned issue.
strangely i had much luck not to use FoG under such conditions.
GroupUtils: http://www.hiveworkshop.com/threads/system-grouputils.205251/ ... which is an improved version of http://www.wc3c.net/showthread.php?t=104464

Originally Posted by Captain Griffen
Units that are removed from the game or decay while part of a group remain in the hash table. They also appear to remain in the list (which can cause issues with FirstOfGroup loops).

Therefore, if units are not removed manually from the group before this happens, there is a minor leak, and the CPU cost of many group operations increases dramatically. Verification is in this thread here.

NB: This only applies to where a group has units in it for longer than an instant, and where those units may decay / be removed. Most uses of groups are not vulnerable to that (most uses you should be using a static group and a boolexpr these days - those are completely unaffected).

Fortunately, GroupClear solves the problem, enabling the creation of a function that can be called on a group and flushes out all of the shadow units in it. The cost is pretty much O(n), where n is the number of real units in the group (rather than shadows).

GroupRefresh does not affect the group in a noticeable way, aside from removing the shadow references in the hash table, which speeds up most operations (see the link above for further details). The only effect it will have on outputs is on FirstOfGroup, which can return null when it comes to a shadow reference in the list, even if there are real units after it. GroupRefresh clears those out, so makes FirstOfGroup act properly again.

More frequent calls of GroupRefresh will reduce CPU costs of most operations involving groups whose usage makes them vulnerable to this, but will incur a higher CPU cost of its own. Look at the benchmarks on the link above and decide for yourself what the best balance is.

Granted, most of these discoveries are from 2008, so I don't blame you for not knowing about them, but for everyone who has been around since TFT, this is a known issue.
 
Level 19
Joined
Dec 12, 2010
Messages
2,069
It's not like that is a serious issue, though. There are pretty easy workarounds to reset the OP limit.
any kind of "workaround" should become part of the engine which needs this workaround. especially if that doesnt break backward compat. literally any of systems which are done here should be just a pack of natives provided within game's natives.
 

~El

~El

Level 17
Joined
Jun 13, 2016
Messages
558
any kind of "workaround" should become part of the engine which needs this workaround. especially if that doesnt break backward compat. literally any of systems which are done here should be just a pack of natives provided within game's natives.

Agree. This might not be a major issue, but there are many small "quirks" like these in WC3, which together make mapmaking a minefield. The fact that there are workarounds doesn't justify the presence of these issues in the first place.
 
We had this discussion before. We won't get all the stuff we would like to have, simply because Blizzard is not willing to invest the resources into fixing a decade old game.
What we can hope for is maintenance and some desperately needed fixes. This is not one of them.
It's nice to have, but we simply have to set priorities on what we want, simply because the classic game team can't do everything and there are way more pressing matters that need to be adressed first.

If you want freedom and a "minefield"-free mapping experience, there is Starcraft II for you.
 
Status
Not open for further replies.
Top