godotengine / godot

Godot Engine – Multi-platform 2D and 3D game engine
https://godotengine.org
MIT License
89.06k stars 20.19k forks source link

Error [ Condition "p_elem->_root" is true. ] spamming the console #96339

Open inhalt120g opened 2 weeks ago

inhalt120g commented 2 weeks ago

Tested versions

v4.3.stable.official [77dcf97d8]

System information

Mac Mini M1 with newest MacOS

Issue description

I have a very simple script attached to a TileLayer Node. As soon as it gets triggered, it spams the console with Condition "p_elem->_root" is true. errors. The weird part is that if I add (as built in) or attach (as a gdscript file) the same script to another TileLayer node, it works fine. The project is so simple that I can't see what could be causing it and was unable to isolate it.

Steps to reproduce

This is the script that's causing the errors:

extends TileMapLayer

# metadata container
#@export var data : Node

# metadata array names that hold the relevant data
#@export var slide_tiles : String # LayerTransformPlayer
#@export var slide_phase : String # LayerTransformPlayerTimer

#const Tile_Size = 100

@warning_ignore("unused_parameter")
func _use_tile_data_runtime_update( C : Vector2i ): return true # Coordinates, Vector2i

@warning_ignore("unused_parameter")
func _process ( delta ) : notify_runtime_tile_data_update()

@warning_ignore("unused_parameter")
func _tile_data_runtime_update ( C: Vector2i, TD: TileData):
    pass

As it can be seen it does nothing (I commented out and deleted everything). From my testing it looks like just using "_tile_data_runtime_update" makes it go bad.

Minimal reproduction project (MRP)

Unfortunately I can't retrigger it outside of my own project (which is a super simple test project with barely a few nodes in it). The problem is that the script works fine on other TileLayer nodes. There's nothing special about the Node that can't handle this script. I'm not using any terrains, physics, pathfinding, nothing. It's just a super simple TileLayer.

inhalt120g commented 2 weeks ago

Forgot to confirm, when I fill in the actual code that I use (so the script actually does some stuff with tiles), it all works just fine even with this problematic Node… but it still floods the console with errors.

AThousandShips commented 2 weeks ago

While it probably shouldn't spam a lot of errors this code is far from actually useful or realistic code, so it isn't something the class can necessarily reasonably be expected to behave well with, but should probably communicate this better unless something is wrong, but the code here looks like it's asking for trouble, as per the documentation:

Updating the TileMapLayer is computationally expensive and may impact performance. Try to limit the number of calls to this function to avoid unnecessary update.

inhalt120g commented 2 weeks ago

Not sure if that's what you meant, but just to confirm: in my project I am making sure to update only the tiles that need updating. I just cut all checks out for the sake of the example because there seems to be no difference with or without any checking, ie the script works on one TileMapLayer but not another.

The TileMapLayers in my case are three TileMapLayers each 10 by 10 tiles all using the same TileSet, if that matters. Just for a test I turned off all checks (so the script updates everything all the time), added more tiles to each TileMapLayer and it still worked without any performance issues that I was able to detect.

Other than that, is there something else I can do to update my tiles / what would be "useful" or "realistic" code?

AThousandShips commented 2 weeks ago

The code you gave isn't useful code, but if it's triggered by more useful code that doesn't just force reloading every frame and updates everything

Please provide an example that causes the issues and is representative of your use, because the code you showed isn't what anyone should or would use, but if it's happen when not doing updates every single frame without being needed that's more critical, but it soense seem at all like calling the notify method every frame is something you should do, which is documented

But in the end it's not easy to test without some concrete examples so please try making an MRP

Calandiel commented 2 days ago

Here's an example from my project:

class_name TileMapHandler
extends TileMapLayer

var modulated_cells: Dictionary;

func _tile_data_runtime_update(coords: Vector2i, tile_data: TileData) -> void:
    tile_data.modulate = modulated_cells.get(coords, Color.MAGENTA)

func _use_tile_data_runtime_update(coords: Vector2i) -> bool:
    return modulated_cells.has(coords)

func color_tile(color: Color, coords: Vector2i):
    modulated_cells[coords] = color
    notify_runtime_tile_data_update()

color_tile is called every frame due to the high amount of changes happening all over the place. Same behavior as observed by the OP.

To reproduce, attach the script to a tile map layer and create a second script that calls color_tile with random inputs every frame. Happens on v4.3 official, both Linux and Windows.

I'd add that this happens even when notify_runtime_tile_data_update isn't called every frame. In fact, it seems calling it at all causes it. To verify that, use call deferred instead of attempting it every frame.

@AThousandShips

AThousandShips commented 2 days ago

@Calandiel Please upload an MRP

However your code does also seem broken as it won't ever stop updating because you never remove elements from modulated_cells, try:

func _tile_data_runtime_update(coords: Vector2i, tile_data: TileData) -> void:
    tile_data.modulate = modulated_cells.get(coords, Color.MAGENTA)
    modulated_cells.erase(coords)

Otherwise you'll keep updating even when the cell is supposed to have been done

Calandiel commented 2 days ago

In here: as it won't ever stop updating are you saying you expect this to enter an infinite loop and get stuck? It only seems to attempt to modify the data for tiles that already had the data set before.

The behavior is the same regardless of whether modulated_cells are erased or not - seems tangential to the matter at hand (and if it wasn't, then docs ought to be updated to be more explicit about it).

I'm not in a position to upload a MRP in the immediate future. I'll look into it if I get a chance later.

AThousandShips commented 2 days ago

No I'm saying it will keep updating that tile, because you never clear out the check, so it's the same as if you'd just do:

func _use_tile_data_runtime_update(coords: Vector2i) -> bool:
    return true

I'm just saying that your code doesn't seem right either and would be expected to not perform well, but if you can upload an MRP soon that'd help, but otherwise there's nothing we can do, and unless you or the OP uploads one this will be closed

Calandiel commented 2 days ago

My understanding is that both of us aren't reporting it to not perform well, we are reporting it to flood the console with error messages. That's quite different and (imho) not expected regardless of how frequently the method is called. I think you're getting lost on the details.

I can probably supply a MRP at some point in the future but not in the immediate future - I have other responsibilities besides Godot. If the issue is closed by then, I'll make a new one once I have a project at hand, assuming I still encounter this issue.

AThousandShips commented 2 days ago

I think you are misunderstanding what I'm doing, I'm trying to pin down what's wrong, I never said the error was good, but trying to:

These details are very important to establish what is going on, and how to approach this

So if you or anyone else can provide an MRP that would speed this along and once one is made I will test this and try to solve this!

Calandiel commented 1 day ago

Thank you for clarifying - you're right, I misinterpreted your intentions. That makes a lot more sense.

I will do my best to provide a MRP but the most likely time for that to happen is next weekend.