[SimonArs]

Fixes FlxPoint leak in findFrame()

[MTUnixic]

will this be merged?

[SimonArs]

will this be merged?

idk

[Geokureli]

This fix is not as straight forward, and needs some changes. Also, this isn't really a high priority, tbh. I don't think this is going to impact anyones performance significantly, and if it does you could always just pass in a zero point and put() it yourself after

[SimonArs]

This fix is not as straight forward, and needs some changes. Also, this isn't really a high priority, tbh. I don't think this is going to impact anyones performance significantly, and if it does you could always just pass in a zero point and put() it yourself after

Actually it'll because functions like

flixel/flixel/graphics/frames/FlxAtlasFrames.hx

Line 243 in 5e147e8

var frames:FlxAtlasFrames = FlxAtlasFrames.findFrame(graphic);

and similar ones check for existence of frames but if point.equals itself calls putWeak then points that fail check simply remain in pool increasing it and you can’t manually call putWeak here

[Geokureli]

I think the existing setup of this method is bad practice, unconventional and I wanna take a closer look, when I have time. Please have a little patience.

We need to stop treating every PR like it's an urgent super-critical issue when we all know this will not greatly impact everyone's flixel game

If your project sorely need this change, let me know, and I'll bump it up in priority, but I really doubt that, as there's a pretty solid workaround and this method isn't called very often

[SimonArs]

I think the existing setup of this method is bad practice, unconventional and I wanna take a closer look, when I have time. Please have a little patience.

We need to stop treating every PR like it's an urgent super-critical issue when we all know this will not greatly impact everyone's flixel game

If your project sorely need this change, let me know, and I'll bump it up in priority, but I really doubt that, as there's a pretty solid workaround and this method isn't called very often

Okay, I guess I really need to slow down a bit
I'll close PR with comment

[Geokureli]

This is still a valid issue, and keeping it open serves as a reminder to me, to get to this when I do have time.

I mainly just don't want people coming in here and bumping these as if there's some kind of urgency, i.e.:

will this be merged?

[Geokureli]

I have time now, sorry for the long wait.

My issue here is that using a weak point for an optional arg is not common, and for good reasons. weak points are destroyed and put back in the pool when used in methods. In the case, here, that may happen the first time atlas.border.equals(border) is called, and if there's multiple atlases to check later checks will be done with a destroyed point.

This may be fine, as destroying the point doesn't change the value, but it's fragile, and odd. If that changes (and it has, in the past) then this would start misbehaving.

To be clear, the entire issue I have is with existing code, the new code only illustrates that issue. Using a FlxPoint to represent a border is dumb, since pixels are an integer unit and points are floats, and having to create point instances in all of these methods is also dumb and should be changed to take ints. It also seems odd that findFrame(graphic) is used to see if there is already an atlas, when it's only looking for a borderless graphic, but there may already be an atlas with a border that isn't 0, 0.

Anywho, I made a new issue referencing this one, gonna merge this and address my concerns later after I see if there's a fix I can add

[Geokureli]

@SimonArs any thoughts or concerns on this change?

[SimonArs]

@SimonArs any thoughts or concerns on this change?

No i haven't. Do as you see fit