search

KOBUGE-Incubator / ringed

2D Topdown Shooter Godot Game
GNU General Public License v3.0
88 stars 14 forks source link

Memory leaks #57

Closed hilfazer closed 7 years ago

hilfazer commented 7 years ago

Replaced queue_free() with free() to delete objects that were not part of a SceneTree.

akien-mga commented 7 years ago

Why would those be memory leaks? queue_free() would just trigger free() when the node is no longer used, so at most during the following idle frame AFAIK.

hilfazer commented 7 years ago

Every time i quit to main menu i see more objects in Debugger/Monitors/Objects. How much more depends on how long i've beed playing. After replacing queue_free() with free() this number goes up by only 1 each time (so there's still a leak somewhere).

akien-mga commented 7 years ago

I wonder if it's not an engine bug then, I'll investigate.

volzhs commented 7 years ago

https://github.com/godotengine/godot/blob/master/scene/main/node.cpp#L2573-L2577 based on Node::queue_delete() which is queue_free, A node will not be deleted if it is not in tree.

var c_sprite = get_node("Sprite")
if(c_sprite):
    self.remove_child(c_sprite)
    c_sprite.queue_free()

so, I suggest to just remove self.remove_child(c_sprite) this line.

I don't have any clue why it's blocked if it's not in tree.

volzhs commented 7 years ago

oh, I get it. if node is not inside tree, get_tree() is null so...

akien-mga commented 7 years ago

For the reference, it's now fixed in Godot upstream with https://github.com/godotengine/godot/pull/11447

But that's bogus code anyway in Ringed and should be refactored, it makes no sense to remove_child() and then free(), one could queue_free() directly. And the bullet/holder code is super hacky... But well, that patch makes it at least slightly better, so we can merge.