Overgrowth

[akolyt0r]

Originally Posted by Pyrogasm

A double free is exactly the opposite of a leak. It's destroying the same object twice.

sure, but pretty often a double free means you most probably missed to destroy one object, but destroyed another one twice instead ...

So the first object will leak.
Havent looked at the code though ..so i dunno if this applies to your code aswell.

[FriendlyPsycho]

I don't get the double frees when I test it though..

[Anitarf]

Originally Posted by Flame_Phoenix

Please fix it.

Update your xe, you nab.


As for the spell, TimedXefx is still an issue. It hasn't yet gone through our review process yet so you can't use it in a spell you submit here. Since it's such a simple library, I'll review it right now: The struct data is useless, you could have just as well attached the xefx to the timer directly. The end result is such a short code that you could just as well inline it into the spell entirely and thus avoid me telling you again that you can't use it.

Also, you should remove the earlier versions of the spell from the post since only the last version will actually be the one to be approved.

[Flame_Phoenix]

Update your xe, you nab.

Actually if you bother enough to see my replay and my screen, you will notice that I got the "double free message" using the test map provided by the creator. I tested the most recent version (at that time) and so I assume that the demo map had the most recent xe (at that time). If anyone has to update something, that someone is not me, but the creator in the demo map, because the error was found in the test map.
On the other hand, if you don't care about that problem, than it is also not my problem as well. The fact is that (at that time) I got an error message I didn't like and I reported in a friendly way. By ignoring my report you risk your credibility as a mod, because if someone tests this spell in the future and if the tester finds the problem again, the tester will think that Anitarf is a nab mod because he approved a spell with a problem.
I see the version was updated, I am happy by that, I didn't test the latest version yet, so I can not say if the problem was fixed or not. I will just remove myself from this thread, I can obviously see my reports are not welcome here.

Btw, Thx to ako for the explanation of the error. I would give you rep++ but it looks like I have to spread some more rep =S

[Anitarf]

Originally Posted by Flame_Phoenix

Actually if you bother enough to see my replay and my screen, you will notice that I got the "double free message" using the test map provided by the creator. I tested the most recent version (at that time) and so I assume that the demo map had the most recent xe (at that time). If anyone has to update something, that someone is not me, but the creator in the demo map, because the error was found in the test map.

I know what a double free error is so I didn't really need to look at any screenshots or replays to know what was going on.

On the other hand, if you don't care about that problem, than it is also not my problem as well. The fact is that (at that time) I got an error message I didn't like and I reported in a friendly way. By ignoring my report you risk your credibility as a mod, because if someone tests this spell in the future and if the tester finds the problem again, the tester will think that Anitarf is a nab mod because he approved a spell with a problem.

You didn't consider the possibility that I tested the map before assuming the error was on your side. Apparently, the xe in the spell map (or maybe the spell itself, I don't know and don't care enough to bother finding out) was updated between our tests and this led to the misunderstanding.

[FriendlyPsycho]

Originally Posted by Anitarf

As for the spell, TimedXefx is still an issue. It hasn't yet gone through our review process yet so you can't use it in a spell you submit here. Since it's such a simple library, I'll review it right now: The struct data is useless, you could have just as well attached the xefx to the timer directly. The end result is such a short code that you could just as well inline it into the spell entirely and thus avoid me telling you again that you can't use it.

Right'o, I'll optimize the library. Should I submit it as another resource instead, might come in handy for those who want to use xefx but are stuck with TimedEffects, or maybe should I just tack in the code with moyack's TimedEffects thread?

Again, I'm sorry if this bothers you, but I kinda disagree with inlining it. What if someone wants to use two variations of it? Even though inlining only takes up very few lines, it is still redundant code, and I might as well just use a library for added modularity.

[Anitarf]

As I said, you should inline it. It's not much of a library once you write it correctly, and it wouldn't get approved as such. Modularity is all nice and good for coding but when it comes to the end user having a ton of libraries in the map can be a pain.

[FriendlyPsycho]

Ah, now I see your point.

Updated! Sorry for making your job any harder by trying to prove some of my points. My bad :P

[Anitarf]

Just a few minor issues remaining:

• There already is a max collision size constant in xebasic, no need to have another one in your spell.
• You should use a local trigger variable instead of the GUI global, so users don't have to get the trigger name exactly right.
• tx and ty could have been local variables instead of struct members, which are a bit less efficient.
• The same is true for the .a member, like tx and ty you calculate it in loopfunc every time so there's no need to store it.
• With that in mind, the EventTargetLib is kind of unneeded since the only thing it shortened in the spell code was the angle calculation.
• You forgot to include a IsUnitInRangeXY check in the enum function.
• You never use temploc. I assume you meant to use it for the IsUnitInRange check but you could just reuse the GG_Distance's members for that instead.

[FriendlyPsycho]

1.) Ok done.
2.) What GUI global? The gg_trg thing? Well, it is created anyways so I just used that.
3.) Done.
4.) Done.
5.) Removed usage.
6.) Um I don't need to? Pretty sure about that..
7.) Oops, forgot to clean that up heh. Done.

First post/spell updated to conform to the aforementioned stuff.

[Vexorian]

2.) What GUI global? The gg_trg thing? Well, it is created anyways so I just used that.

smells like bad scoping.

These gg stuff and for example not using a initializer but calling InitTrig are all wrong, they are GUI-dependent, we can get better than that... relying on such a variable or InitTrig just makes your code less portable, it also makes it harder to use the template twice (a la JESP ...)

[FriendlyPsycho]

I do use an initializer, scope Overgrowth initializer init

[Anitarf]

Originally Posted by FriendlyPsycho

I do use an initializer, scope Overgrowth initializer init

He didn't say you weren't using it, it was just an example:

Originally Posted by Vexorian

These gg stuff and for example not using a initializer but calling InitTrig are all wrong,

And he's right. The variable is only created if you have a GUI trigger named exactly the same as the spell's scope name, which is an entirely unnecessary implementation requirement.

Originally Posted by FriendlyPsycho

6.) Um I don't need to? Pretty sure about that..

I assume you're trying to get spell like targetting where units are affected as long as their collision size is within the given radius, at least that's the only reason I see for enuming units within (radius+max collision size) range. If that's what you want, then you need a IsUnitInRangeXY check, since the enum may pick units that have their entire collision size outside the given radius, as long as their center is within (radius+max collision size).

[FriendlyPsycho]

Ok then. Updated the first post.

[Anitarf]

The IsUnitInRangeXY check should be in the enum function, not the TargetsAllowed function. There's no need for this condition to be calibrateable, it's an entirely technical part of the code that shouldn't be in the calibration section. Also, like I said, there's no need for special variables to pass the x and y coordinates to the enum function when you could easily use the GG_Distance's x and y members to do that.

The rest looks good.