Godot reports "Resources still in use at exit"

[deadhostdave]

When building a map at runtime, Godot reports:

ERROR: clear: Resources still in use at exit (run with --verbose for details). At: core/resource.cpp:450

I can reproduce this using a fresh project with Qodot addon, and running the 'runtime-map-building' example.

I've had a look into the issue, and have found sources of the leaks: 1) build_entity_nodes: node is allocated at top of for loop, which can be overwriting the reference to already allocated node in the previous iteration; in the "continue" case. 2) build_entity_nodes: node is overwritten with ClassDB.instance() 3) build_entity_nodes: node is overwritten with entity_definition.scene_file.instance() 4) build_entity_nodes: if use_trenchbroom_group_hierarchy is enabled, the node is added to the entity_nodes but may not be given to queue_add_child - some of these nodes do not end up in the scene tree, and leak. Should these nodes end up in the scene tree during resolve_group_hierarchy?

[JamesC01]

I have this problem too, thought it was something I did.

[Shfty]

@deadhostdave Have you modified any of the mentioned call sites to confirm that doing it differently does in fact fix the leaks?

I'm a little hesitant to take this error at face value, as I've seen it in a lot of non-Qodot projects, and Godot's reference-counted memory model (which Resource is a part of via its Reference ancestry) is allegedly supposed to 'just work' with regard to freeing objects when they're no longer in use. Qodot doesn't go out of its way to manually alter reference counts, so in theory this should be down to the engine.

I'm open to the idea that my understanding is flawed here, but equally cognizant that Godot has a habit of outputting 'harmless' errors such as the "Time should be greater than zero." spam that's been happening on startup for a few versions now.

Also, can you post the output with --verbose? That will print more specifics about each leaked object, and I'd like to get a better idea of what the engine is seeing here.

[deadhostdave]

@Shfty I've modified build_entity_nodes to fix and test (1)-(3) above, for (4) I've only pinpointed the use of use_trenchbroom_group_hierarchy.

For (1)-(3) above, I believe the leak is from QodotEntity which derives from Object via Spatial and not Reference - my understanding is that in this case, there is no garbage collection, and nodes derived from Object will only be deallocated if part of the scene tree - or manually with free or queue_free. In these cases, node is overwritten without freeing - and will not be freed by Godot as it is not part of the scene tree.

I agree that Resource should not be an issue here, as it derives from Reference - which as you say implements reference-counted objects.

Partial fix in build_entity_nodes with 3 "<< partial fix" comments:

if 'classname' in properties:
    var classname = properties['classname']
    node_name += "_" + classname
    if classname in entity_definitions:
        var entity_definition := entity_definitions[classname] as QodotFGDClass
        if entity_definition is QodotFGDSolidClass:
            if entity_definition.spawn_type == QodotFGDSolidClass.SpawnType.MERGE_WORLDSPAWN:
                entity_nodes.append(null)
                node.queue_free() # << partial fix
                continue
            elif use_trenchbroom_group_hierarchy and entity_definition.spawn_type == QodotFGDSolidClass.SpawnType.GROUP:
                should_add_child = false
            if entity_definition.node_class != "":
                node.queue_free() # << partial fix
                node = ClassDB.instance(entity_definition.node_class)
        elif entity_definition is QodotFGDPointClass:
            if entity_definition.scene_file:
                node.queue_free() # << partial fix
                node = entity_definition.scene_file.instance(PackedScene.GEN_EDIT_STATE_INSTANCE)

This is the original verbose output. I think that the Leaked instance: Spatial:2380 - Node name: with blank node names indicate the issue (1)-(3) because the generated node names aren't assigned until outside the above code block - after the node has been leaked.

original_verbose_output.txt

And with the above partial fix applied:

partial_fix_verbose_output.txt

Now... for issue (4) - I need to look at more, as I'm no longer confident I didn't generate a TrenchBroom map that uses func_group perhaps in a way that broke Qodot (I'm only just learning TB) - Using the 0-trenchbroom-group-hierarchy.map example scene didn't have the same leaks as reported when loading my map.

[deadhostdave]

Quick update for issue (4).

I don't know if I've done something stupid here and misunderstood something or not. Is use_trenchbroom_group_hierarchy expected to effect using Spawn Type of Group?

The issue I've seen appears to occur when a QodotFGDSolidClass uses a Spawn Type of Group. If use_trenchbroom_group_hierarchy is enabled, the built map doesn't display the geometry, and instances are leaked. If use_trenchbroom_group_hierarchy is disabled, the build map displays the geometry, and the Solid does appear in its own Group in the scene tree - so all good.

Verbose output with a simple map - 1 entity with spawn type group applied: spawn_group_verbose.txt

This can be quickly tested with:

// Game: Qodot
// Format: Standard
// entity 0
{
"classname" "worldspawn"
}
// entity 1
{
"classname" "func_group"
// brush 0
{
( -64 -64 -16 ) ( -64 -63 -16 ) ( -64 -64 -15 ) __TB_empty 0 0 0 1 1
( -64 -64 -16 ) ( -64 -64 -15 ) ( -63 -64 -16 ) __TB_empty 0 0 0 1 1
( -64 -64 -16 ) ( -63 -64 -16 ) ( -64 -63 -16 ) __TB_empty 0 0 0 1 1
( 64 64 16 ) ( 64 65 16 ) ( 65 64 16 ) __TB_empty 0 0 0 1 1
( 64 64 16 ) ( 65 64 16 ) ( 64 64 17 ) __TB_empty 0 0 0 1 1
( 64 64 16 ) ( 64 64 17 ) ( 64 65 16 ) __TB_empty 0 0 0 1 1
}
}

I appreciate this is taking advantage of "func_group" (being marked as Qodot Internal so not exported), but it works for testing import. I have also tested by adding a new custom QodotFGDSolidClass using a Spawn Type of Group to recreate the issue.

[Shfty]

The TrenchBroom Group Hierarchy setting is a bit complicated - TB uses hidden func_group entities under the hood to drive its named brush grouping functionality, which doesn't map directly to the tree hierarchy that ends up in Godot due to the distinction between structural geometry (basic no-class brushes) and solid-class brush entities.

A conversion has to take place that assumes a few things about your map structure - namely that each func_group will have a single entity inside it to act as the attach root for any sibling structural geometry. Violating that assumption (by either not providing a root, as per your basic example, or by having >1 entity inside it) effectively results in undefined behavior, which may be the cause of the leak here.

There's some info about it on the wiki, and an example map demonstrating the intended usage inside the plugin directory.

If memory serves, the Group spawn type is intended to enable all of this nonsense by marking a given classname (i.e. func_group) as exhibiting that special-case behavior, since that makes it mutually exclusive with the other spawn types.

Being honest, it's not the best approach - it'd probably be better off as a completely distinct step from the map building itself, but that's beyond the scope of qodot at this point; qodot-next and beyond are more suited to that kind of big structural upheaval.

[JamesC01]

The TrenchBroom Group Hierarchy setting is a bit complicated - TB uses hidden func_group entities under the hood to drive its named brush grouping functionality, which doesn't map directly to the tree hierarchy that ends up in Godot due to the distinction between structural geometry (basic no-class brushes) and solid-class brush entities.

A conversion has to take place that assumes a few things about your map structure - namely that each func_group will have a single entity inside it to act as the attach root for any sibling structural geometry. Violating that assumption (by either not providing a root, as per your basic example, or by having >1 entity inside it) effectively results in undefined behavior, which may be the cause of the leak here.

There's some info about it on the wiki, and an example map demonstrating the intended usage (inside the plugin directory)[https://github.com/Shfty/qodot-plugin/tree/master/addons/qodot/example_scenes/2-miscallaneous/0-trenchbroom-group-hierarchy].

If memory serves, the Group spawn type is intended to enable all of this nonsense by marking a given classname (i.e. func_group) as exhibiting that special-case behavior, since that makes it mutually exclusive with the other spawn types.

Being honest, it's not the best approach - it'd probably be better off as a completely distinct step from the map building itself, but that's beyond the scope of qodot at this point; qodot-next and beyond are more suited to that kind of big structural upheaval.

Ah, I didn't realise qodot-next was a thing! Is this why you wouldn't fix the icon.PNG bug?

[deadhostdave]

Thanks for taking the time to explain that - very helpful! I need to read up on how to handle grouping in TB and make sure all groups meet Qodot constraints. I was thinking that I could use grouping to group "rooms" to make use of the upcoming Portal Culling feature in the next Godot.

Are you considering the fix above for leaks (1)-(3)? Also, how would you feel about adding a final post attach build step that looped over all remaining entities in the entity_nodes list (as they are now effectively orphaned) and deallocating them and reporting a warning (e.g. entities not added to the scene, etc)?

Is the plan that qodot-next will ultimately supersede qodot?

[TheRektafire]

@deadhostdave for what it's worth I've been working on a blender script to convert objs exported from trenchbroom into a format that is importable into several different n64 games all of which use room systems (and a few of which use portals too), I put an initial version on the overkart64 discord but that version naturally only supports mario kart 64 for now, but I plan to support other games too including godot. So maybe that can be a possible option for you if using qodot directly doesnt work out