Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Mesh from Heightmap errors - possibly due to threading? #70

Closed
BryceBubbles opened this issue Jun 2, 2020 · 7 comments
Closed

Mesh from Heightmap errors - possibly due to threading? #70

BryceBubbles opened this issue Jun 2, 2020 · 7 comments
Labels
bug Something isn't working

Comments

@BryceBubbles
Copy link

I'm getting these errors when generating Mesh from Heightmap:
Can't add child 'MeshInstance' to 'Output', already has a parent 'Output'.
and
res://addons/concept_graph/src/core/concept_graph.gd:230 - Invalid type in function 'add_child' in base 'Spatial'. The Object-derived class of argument 1 (previously freed instance) is not a subclass of the expected argument class.

Sometimes no MeshInstance is output, and sometime multiple are output.
And then eventually clicking replay will crash Godot.

Is this a threading issue?
That error on line 230 seems to imply the new output nodes have been freed, not the old ones.
Is it reusing references?
Or do we just have to wait for queue_free to actually free the output before doing add_child again.
I'm sure you know exactly what's wrong :)
https://github.com/HungryProton/concept_graph/blob/9600a39c4aeecc081f2af7adf4cc07f711f7531c/src/core/concept_graph.gd#L216-L232

@HungryProton
Copy link
Member

HungryProton commented Jun 2, 2020

Well it sounds like a threading issue, but that's strange because if a thread is already running, I make sure to wait until it's completed before running a new generation.

Maybe Linux treats threads differently and there's extra steps I need to take to make sure it runs smoothly on every platforms. That's not going to be fun to debug.

In the meantime, I added a new setting in the projects property panel in 074f078. Try disabling multi-threading for now if the issue disappear at least we will know for sure.

image

@HungryProton HungryProton added the bug Something isn't working label Jun 2, 2020
@BryceBubbles
Copy link
Author

Thanks for adding that!
Ok so...

  • It's not a threading issue, as it happens with that turned off
  • It happens for everything, even a simple Generic Input to Output 3D
  • It's something to do with newly generated nodes having been freed, or already having a parent

If I change line 228 in concept_graph from:
if not node:
to
if not node or not is_instance_valid(node) or node.get_parent():
I can stop the 2 errors I listed above, as it's now checking that the node hasn't been freed, and if it already has a parent. And now the editor no longer crashes.

But this isn't actually fixing the real issue, as I still get these things happening:

  • Multiple instances of the same thing added to output, with numbers appended
    • Sometimes extras are added (name, name2)
    • Sometimes one or more disappear the next time (sometimes all)
    • But the number order is always ascending, and always different (2, 3, 6) (none, 2, 5)
  • In one scene I get visual output, but with no output showing in the scene tree

And now I sometimes get these errors (in different scenes) when clicking replay:
' res://addons/concept_graph/src/nodes/outputs/output.gd:481 - Left operand of 'is' was already freed.'
' res://addons/concept_graph/src/nodes/meshes/mesh_input.gd:481 - Left operand of 'is' was already freed.'
That line number shows that the error is actually in a different file, likely concept_node.gd:481
Where it's doing this if res is Node: in the function _reset_output()

And now I also sometimes get these errors when opening some scenes:
' res://addons/concept_graph/src/core/concept_graph.gd:202 - Attempt to call function 'get_edited_scene_root' in base 'null instance' on a null instance.'
' res://addons/concept_graph/src/core/concept_graph.gd:207 - Invalid call. Nonexistent function 'get_children' in base 'Nil'.'

Lastly, I've always been getting this (likely unrelated) error whenever I open a scene with a ConceptGraph:
' scene/main/node.h:281 - Condition "!data.tree" is true. Returned: __null'

Windows 8.1 with Godot 3.2 stable
Sorry to be a pain, but it'll be tricky to work on stuff until I can fix this issue.

@HungryProton
Copy link
Member

HungryProton commented Jun 3, 2020

Did these issues started after a specific commit?

Can't add child 'MeshInstance' to 'Output', already has a parent 'Output'. Is strange, I always make sure to return a copy of the output so even if the event was fired twice, it shouldn't try to operate on the same object multiple times.

The (previously freed instance) error worries me a bit because that's probably a race condition with the "garbage collector". I'll setup a Windows test rig and try to reproduce these bugs later today, maybe if I run it on a virtual machine and try to emulate different kind of hardware I'll manage to fix this.

The rest of the issues are mostly about GraphNodes (and yes the error message points to the parent class file, that's probably a gdscript limitation / bug). I have all these sometimes but I don't know why so I have to restart the editor and they're gone. Ssometimes, it's because there's an error in a node script so the node library fails to load it but the editor still tries to create connections with this node because it's mentioned on the cgraph file.

' scene/main/node.h:281 - Condition "!data.tree" is true. Returned: __null'
This last one is an old issue, there's a script that tries to access a node's global_transform but said node is not on a scene tree so it can't. I haven't looked into it yet but should be trivial to fix. Godot traces aren't really helpful on this one.

@BryceBubbles
Copy link
Author

I noticed it when I started playing with the Heightmap nodes you added.

But I'd say it was 9e6d16f where you enabled multithreaded - but it's something more core that changed when doing that.

Maybe run_simulation(), the timer and simulation delay, or yeah perhaps garbage collection?

@HungryProton
Copy link
Member

HungryProton commented Jun 3, 2020

Alright I'll take a look as soon as I can. I hope the issue is easy to replicate, if it's not platform dependent it's not going to be easy to track.

Edit - Managed to reproduce the issue, I'll let you know if I make any progress
Edit 2 - I think I partially figured it out but it doesn't make sense, there's no way it should have worked in the first place.
Edit 3 - Actually, there's another hidden bug in the garbage collection process

@HungryProton
Copy link
Member

Should be fixed in a3150d6, with or without multithreading.

Let me know if it still doesn't work

@BryceBubbles
Copy link
Author

Awesome, yup it's fixed, thanks :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants