IsUnitMoving Simple [GUI] v. 1.0

[Moderator]

18th Apr 2016

Your resource has been reviewed by BPower.
In case of any questions or for reconfirming the moderator's rating,
please make use of the Quick Reply function of this thread.

Review:

An inefficient re-make of an already existing resource ( Is Unit Moving ).
The term in such a case is Reinventing the wheel.
You can check out comment #5 and #6 for more detailed information.

I tend to reject this submission. For now Need Fix.

Troubleshooting:

• There is a group handle leak in your code.
  ---
• There is a periodically occuring rect handle ID leak, because GetUnitsInRectAll is badly implemented into GUI.
  That was the fault of the Blizzard coding team in charge. :/
  ---
• A false positive result on the first iteration.

Review changelog:

1. -

 

[Johnromar09]

.........

 

[Almia]

http://www.hiveworkshop.com/forums/spells-569/unit-moving-2-0-0-0-a-199189/?prev=d=list&r=20&u=bribe is much better

 

[Meatmuffin]

Isn't this too simple?

 

[BPower]

Let's sum it up it:
Your code is shorter, eventally easier to read, but mainly it's much much slower than Bribe's IsUnitMoving.

Furthermore your system is inaccurate when a unit is enumerated the first time.

You have a group leak.

 

[Bribe]

0.03 seconds is slower than movement systems based on 0.03125 periodic movement or higher. In fact, the 0.05 I use in IsUnitMoving is already lower than necessary and could have been set to 0.10 for better performance.

The reason the events of the indexer are used is to capture valid units into an iterable list. This is the fastest way to iterate all units. Systems which rely on fast timers which never stop should eliminate all possible overhead. Opening a new thread for each unit is devestating and will cause far more overhead than adding a couple of variables to store the prev/next unit ID's.

The events to detect when a unit stops or starts moving can be disabled by the importing user if undesired. The performance difference is, however, negligible as a unit doesn't start and stop moving every interval and will therefore never tax the system.

You're leaking a unit group.

 

[Bribe]

This still uses dynamic groups and still opens up a new thread for each unit on the map. It also doesn't work with Locust units due to the nature of GroupEnumUnits...

The biggest technical problem right now is the function GetUnitsInRectAll which leaks handle ID's each time it is called.

I don't understand your motivation in making this resource. What about the existing IsUnitMoving resource is so deterring for you to embrace it?

 

[Meatmuffin]

Also, why is this in GUI? You can make the demo example of how
you use the spell and just make it in JASS, you have the knowledge
for it and it will be more efficient.

 

[SteakMediumRare]

Ugh.

Yes.

5/5

 

[Chaosy]

Is this really such a large downgrade? I mean it could use some tweaking like using a variable for GetEnumUnit() and fixing leaks, but otherwise is there really a performance difference?
Should be approved if changes are made (if I were in charge, but I aint) but I still don't know if it'll benefit others that much since there is another one.
Maybe if he used another indexing method which doesn't force you to use a unit indexer.

 

[Bribe]

Chaosy, there are 0 problems using the current IsUnitMoving resource. The submitter still has not replied to my critique.

And yes, enumerating the entire map several times per second is much heavier in performance. Each picked unit opens up a new thread.