search

viniciusgerevini / godot-clyde-dialogue

Clyde Dialogue Language Importer and interpreter for Godot.
MIT License
89 stars 11 forks source link

Orphaned nodes on exit #1

Closed Tols-Toy closed 3 years ago

Tols-Toy commented 3 years ago

Hey Vini,

I am by no means an expert on Godot's internal working, so bear with me: running the addons with the provided example files (or any other project for that matter) leaves orphaned nodes on game exit.

This could be either fixed by making sure the addons nodes get properly queue_free()'d or -- and this is what I did and suggest -- changing what the scripts in the addon extend from. Right now all of them extend Godot's Node class, which really doesn't seem to be necessary as they dont get added onto the scene tree. Changing

extends Node

to

extends Resource

or even

extends Reference

in all scripts fixes the error as both Resources and References are ref counted and dont need to be manually queue_freed()'d

Unfortunately this somehow messes up one the GUT tests (test_option), not sure why tho.

Best regards, Enes

viniciusgerevini commented 3 years ago

Hey Enes, Interesting. Thanks for the suggestions. Very helpful. I haven't paid attention to orphaned nodes. I'll try your tips and see if I can find why the tests brake. Thanks! :)

viniciusgerevini commented 3 years ago

Hello @Tols-Toy . I made the change to Reference and I can see a big improvement. In my case none of the tests broke. Here is one of the tests I run with my game. I triggered the same dialogue 20 times in each run: The first one is using the current implementation: using Node The second one is using Reference: using Reference

There are orphan nodes still being created because I have some other leaks in my game, but regarding the dialogue, as you can see by only changing it to Reference it recovered ~2.5mb of memory. I've done some other tests and the results are consistent.

That's a big deal, because these references are not cleaned until I close the game. Thanks a lot for spotting this issue, it will also help me to fix some other projects. I'll be releasing the changes soon. Let me know if it fixes the issue on your side.

Tols-Toy commented 3 years ago

Heyo @viniciusgerevini,

just did a diff to find out why your fix #2 doesnt mess up the tests while the same fix on my end does. Apparently at one point I randomly did something weird to one of the .clyde.import files in the dialogue folder. The rest of the files were identical. Anyways, the tests run as intended now. Very happy to have been of help.

Have a good one