Godot errors/crashes after inspecting two different objects handled by the same plugin

Question

Godot errors/crashes after inspecting two different objects handled by the same plugin

Zylann opened this issue 2 years ago · 29 comments

Zylann commented 2 years ago

Godot version

Godot 4 rc2

System information

Windows 10 64 bits NVIDIA GeForce GTX 1060

Issue description

I was trying to reproduce a nasty error I started having since stuff got changed in some areas of editor plugins, but in RC2 instead of reproducing my issue, I ended up finding this crash. So I thought I'd report it...

I wrote a simple test plugin that adds a bottom panel with two buttons:

"Edit Sprite": edits the last selected sprite
"Edit Texture": edits the texture of the last selected sprite

This plugin handles BOTH Sprite2D and Texture2D.

Steps to reproduce

After testing the plugin a bit, I turned it off, then clicked on the Script tab: Godot crashes. No information popup or logs, just shuts down.

Minimal reproduction project

EditorPluginEditAnotherHandled.zip

This project has the plugin enabled by default.

Open the main scene and go to the 2D view
Select the sprite
In the bottom bar, click "Test plugin" to show the custom panel
In the custom panel, click "Edit Texture" and "Edit Sprite" alternatively a bunch of times
Go to Project Settings and turn off the plugin: Godot might crash already.
If no crash occurred, click on the "Script" tab: observe crash.

Secondary note:
I was told that one of the changes to EditorPlugin was that _edit(null) can be called when "there is no longer any selected object handled by this plugin" (seen in the doc). But if you look in the console while alternating the two buttons (which ask the editor to inspect two different objects handled by the plugin), you will notice that edit(null) occurs half of the time, I dont understand why.

Third note:
When testing this repro in master 9c960a8, if I select the sprite and then click on the "Edit Texture" button, I get an error:

ERROR: Condition "plugins_list.has(p_plugin)" is true.
   at: EditorPluginList::add_plugin (editor\editor_node.cpp:8119)

Which I dont understand either.

Answer 1 · 2023-02-20T22:09:45.000Z

if I select the sprite and then click on the "Edit Texture" button, I get an error:

#73512 (comment)

Answer 2 · 2023-02-20T22:46:48.000Z

Yet another note after seeing this suggestion #40166 (comment):
If I provide true as the last argument of inspect_object, which the doc describes as:

If inspector_only is true, plugins will not attempt to edit object.

Then my plugin's UI gets closed, as EditorNode::push_item still calls _edit_current and then make_visible(false) and edit(null). It didn't use to do that (in fact that was the point of it, seems?). This might have been a workaround since I saw other plugins inspect "sub-objects" to allow users to edit them via the inspector usin this boolean, but I had no luck trying that, I dont understand this option...

I searched how the VisualShaderEditorPlugin handles editing sub-resources inside nodes with the inspector, without getting bothered, and it uses InspectorDock::get_inspector_singleton()->edit(p_resource.ptr());, which is not available to scripts.

Answer 3 · 2023-02-24T16:38:28.000Z

I got a similar bug today, the engine sunddely crashes when I click on the TileMap node. =/

ERROR: The TileSetAtlasSource atlas has no tile at (2, 1).
at: (scene\resources\tile_set.cpp:4515)
ERROR: Condition "plugins_list.has(p_plugin)" is true.
at: EditorPluginList::add_plugin (editor\editor_node.cpp:8152)

Answer 4 · 2023-03-07T14:05:31.000Z

I am also getting plugins_list.has(p_plugin) errors with a TileSet editor inspector plugin. I'm having difficulty pinpointing what's causing it because my code isn't doing anything at the time the error occurs. The error appears when a user opens the TileSet bottom editor. I have placed a persistent helper node under base_control, but the plugin script doesn't return true from _can_handle() until it reaches AtlasTileProxyObject, which is after the error occurs.

In my case, the error is not associated with crashes or any other obvious problems.

Answer 5 · 2023-03-07T21:01:12.000Z

Never mind! A new project with no plugins still prints this error when opening the TileSet editor, so it's coming from the engine itself.

editor/editor_node.cpp:8152 - Condition "plugins_list.has(p_plugin)" is true.

Answer 6 · 2023-03-08T11:30:30.000Z

I'm seeing the same error after doing tile set/map for a moment. Using Godot v4.0.stable.official [92bee43]. No plugins added to the project.

Answer 7 · 2023-06-23T15:56:34.000Z

For the record I still see this kind of errors in the editor once in a while.

#74717 seems related.

Answer 8 · 2023-09-19T07:14:44.000Z

Godot 4.1.1. No plugins, worked with TileMaps for a day learning the engine. This is all that shows up when starting up my project:

Godot Engine v4.1.1.stable.mono.official (c) 2007-present Juan Linietsky, Ariel Manzur & Godot Contributors.
  modules/gltf/register_types.cpp:73 - Blend file import is enabled in the project settings, but no Blender path is configured in the editor settings. Blend files will not be imported.
--- Debug adapter server started ---
--- GDScript language server started ---
  editor/editor_node.cpp:8195 - Condition "plugins_list.has(p_plugin)" is true.
  editor/editor_node.cpp:8195 - Condition "plugins_list.has(p_plugin)" is true.
  editor/editor_node.cpp:8195 - Condition "plugins_list.has(p_plugin)" is true.
  editor/editor_node.cpp:8195 - Condition "plugins_list.has(p_plugin)" is true.

Answer 9 · 2023-09-19T10:57:58.000Z

^ This is fixed in 4.2.

Answer 10 · 2023-11-14T22:40:25.000Z

No longer crashes on master. I guess the issue is resolved?
You can't handle the same object by two plugins at the same time, this is expected limitation.

Answer 11 · 2023-11-15T16:06:30.000Z

Closing per the comment above. Please let me know if the issue is still reproduceable for you in 4.2. We'll probably need an updated MRP if it is.

Answer 12 · 2023-12-15T18:19:26.000Z

You can't handle the same object by two plugins at the same time,

Sorry but this is not the same:

This plugin handles BOTH Sprite2D and Texture2D.

It's ONE plugin handling two TYPES of objects. But I guess if you also include the editor's core plugins, that also makes two plugins handling the same classes (which prevents any extensibility/custom sprites??)

But I guess it's also documented as not being supported. But there is no explanation about how else it should be done... this is quite a severe limitation.

I've been using that for several reasons:

In GDScript "addon" plugins, you can't have more than one EditorPlugin in your cfg. So if your plugin contains more than one custom class needing a plugin, let's say a node and resources it can contain, then you're screwed.
Sometimes you want to handle a custom node (or resource), AND custom resources it may contain, in order to keep contextual editors active, which would otherwise result in terriffic usability because they would hide as soon as you select something else, however related (and they would do so because make_visible is litterally meant to do so, otherwise it's unclear to me how we're supposed to implement it).
Similarly, sometimes you want to handle a custom node, AND custom nodes that are meant to be children of it. In my heightmap terrain plugin, grass layers are child nodes. But to maintain the ability to paint their density, terrain tools have to remain active, therefore the plugin has to handle both the terrain and its custom children.
Not sure how/if that plays a role, but handling a base class could be seen as equivalent to handling all its child classes... therefore handling multiple types indirectly. It also means that two plugins can indirectly handle the same object at once. Maybe it's about the type itself and not subclasses? At best, the doc is unclear about that.
Some core plugins DO handle more than one object: Cast2DEditorPlugin, EditorTextureTooltipPlugin, Polygon3DEditorPlugin (indirectly), ScriptEditorPlugin, ShaderEditorPlugin, SpriteFramesEditorPlugin, TilesEditorPlugin (although maybe they arent edited at the same time, but it's also unclear what "same time" actually means)
It didn't use to be a problem. I've been doing this for a long time, this error started occurring at some point in the development of Godot 4.0 and now I guess the limitation is here to stay, but then I dont know how I'm supposed to refactor things (which were already hard to get working in some cases due to annoying behaviorial details of the editor). Condition "plugins_list.has(p_plugin)" is true. keeps getting printed, seemingly causing no issues, but then creating noise in every bug report, annoying users, while also not providing any information about its real cause, not even which plugins, and it doesn't always print, so it's also hard to link it directly to a plugin handling more than one type.

I'm still working on 4.1.3 where the error still prints, although I havent experienced crashes in my plugins (therefore I focused my limited time on more important things). Will eventually start testing 4.2, but regardless, handling the limitation is unclear at the moment.
I'm considering to stop implementing make_visible entirely, but then I don't know how I can hide tools at the right moment without relying on make_visible, handles and edit (which at this point have all become unreliable for my use case)

Answer 13 · 2023-12-15T19:43:28.000Z

You can manually determine edited object by using selection_changed() or edited_object_changed() signals. This way you don't need to rely on handles(), edit() etc., so your editor will not conflict with other editors. However you also can't take advantage of overlay draw and input, not sure how these can be handled manually. But that's where having 2 plugins would cause conflicts, because they'd both try to draw and handle input.

Answer 14 · 2023-12-15T19:50:03.000Z

Unfortunately, EditorSelection is only about nodes. I have resources too. Also, how is edited_object_changed behaving with sub-inspectors? It's ambiguous because they are technically open at the same time.

Answer 15 · 2023-12-15T20:03:54.000Z

It's emitted only for the main inspector object.

In GDScript "addon" plugins, you can't have more than one EditorPlugin in your cfg.

IIRC there is a proposal for more plugins per addon. It shouldn't be difficult to implement. That's how TileMap issues were fixed. There is 2 separate handler plugins now, dependent on each other to make sure you can edit both without problems.

it's also unclear what "same time" actually means)

The editor has a few editing contexts - the scene tree, inspector and sub-inspectors. They can operate independently, i.e. call handles() and edit() each for different objects. "same time" means handling something by a single plugin in multiple contexts at once. If your plugin handles a node and this node has a sub-resource handled by the same plugin then it's not allowed. If you want to edit 2 objects at the same time using the same plugin, you could make handles() return false when one object is already edited.

I dont know how I'm supposed to refactor things

Hmm, maybe I could look into your plugin to see how things are implemented and give some advice.

Answer 16 · 2023-12-15T20:46:08.000Z

My GDScript plugin isn't directly affected because it only handles nodes, however I have the issue in my voxel terrain module, which nests resources inside nodes at multiple levels.

If you want to have a look (although there is a lot going on):

For context, here is a screenshot showing one example of nesting:

Here showing the case of two of the plugins in my module:
The terrain plugin, which preferably should keep its controls shown while the user tweaks the generator, and the generator plugin allowing to edit a graph, which itself contains "sub-objects" that use the inspector as property editor, almost always causing the error to print when alternating between nodes of the graph and the graph resource itself.

I rely on make_visible, which means I ended up doing this in VoxelTerrainEditorPlugin:
https://github.com/Zylann/godot_voxel/blob/c8baeb95cecc4e8f23ce1dfc3bec96846967d8a7/editor/terrain/voxel_terrain_editor_plugin.cpp#L139C54-L167

And this in VoxelGraphEditorPlugin:
https://github.com/Zylann/godot_voxel/blob/c8baeb95cecc4e8f23ce1dfc3bec96846967d8a7/editor/graph/voxel_graph_editor_plugin.cpp#L58

There is of course more to it, notably the fact edit(null) is called either way when I switch between two handled objects, which means I get streams of unnecessary make_visible(false); make_visible(true); ... and some issues with saving. but these aren't directly related. Also very often when I deselect the terrain node to edit something completely unrelated, and select back the terrain, it also re-selects every nested inspector (sometimes even across restarts), causing further occurrences of the error.

Note: I'm intentionally not using any API that isn't available to GDExtensions. There are tricks the core editor does I can't rely on.

Answer 17 · 2023-12-16T18:33:40.000Z

I did a quick patch with 2 changes:
0001-makeshift-patch.patch

makes sure that handles() does not return true if something is already edited
prevents toggling visibility in quick succession (might be unnecessary with the above?)

The code is hacky, but you get the idea. With this fix I am able to see the Terrain button and graph editor at the same time without errors, so I think it works. I don't know the plugin enough to test it properly.

Answer 18 · 2024-01-06T20:06:33.000Z

Had a look at the patch, it really looks like a hack. Seeing stuff like this in those functions makes things confusing compared to what they are meant for. Notably handles, which in theory should only depend on the passed object, and not some state of the plugin, which now results in race conditions.
The hack in make_visible is now also a source of race conditions. Everytime there is a call_deferred, it opens the door for more problems in the future.
I wouldn't have come up with this idea because it goes against what the methods are meant to do and seems to rather exploit Godot's implementation details (which I'm not much aware of, I dont often inspect the editor's core logic).

I feel like Godot should have cleaner ways to code things for contextes like this. It constantly happens for me and it's tiring to keep adding hacks because it feels like I'm loosing control and could break some random day. But not sure what would have to change... as I described earlier, the logic seems too restrictive, partially ill-formed or unclear (both in docs and error messages). Because even though the API could still assume only one "thing" is edited at a time, in practice it doesn't always exist alone in a void, and may be nested in related resources, nodes or objects, which may be part of a whole context with interacting UIs. The sole fact you can have a node selected in the scene tree and a different object edited in the inspector is an example of this, but it goes beyond that of course.

I gave your patch a try, and seems to work when I select a graph resource in a terrain.
Although I suppose the same hack must be added in other places as well: errors keep happening after I select nodes of the graph: clicking on a node uses the inspector to "edit" a custom object representing it (mostly because we can't have extra inspectors). Then clicking on the background is supposed to "edit" the graph resource itself in the inspector. But when the graph gets edited that way, the error occurs everytime editor\editor_node.cpp:8240 - Condition "plugins_list.has(p_plugin)" is true.. I suppose that's because the graph editor plugin has to handle both graph resource and graph nodes to function properly but Godot doesnt like that (no idea why tho), but again I'm really confused about how else to cleanly handle that without messing up all the UIs that are supposed to be shown in this nested editing context.

Actually, that second case I just described is indirectly triggered by my own code, which simply needs to use the inspector to edit a specific "sub-object" of my graph resource, while keeping the graph editing context:
https://github.com/Zylann/godot_voxel/blob/cee2a86af4e6e9d76398856c868a20cf27b7052f/editor/graph/voxel_graph_editor_plugin.cpp#L195

get_editor_interface()->inspect_object(*wrapper);

Therefore, there might be a simpler solution than your hack here.
I noticed this had a parameter p_inspector_only, which the doc describes:

If inspector_only is true, plugins will not attempt to edit object.

Which sounds like it would solve this specific case, and I would be able to remove this sub-object class from handles, getting rid of the error I had before.
But when I provide true, it makes no difference... edit(null) and make_visible(false) are still called, so selecting a graph node now hides the whole graph editor (so plugins are NOT ignored), which defeats the whole point for me.
Though in this case I could workardound with this:

	_ignore_edit_null = true;
	_ignore_make_visible = true;
	get_editor_interface()->inspect_object(*wrapper, String(), true);
	_ignore_edit_null = false;
	_ignore_make_visible = false;

Which luckily seems to be working because neither edit nor make_visible are deferred calls. (but of course using inspector prev/next arrows still break and hide the editor)

Answer 19 · 2024-01-11T10:53:24.000Z

Can you check if this patch fixes your issue with inspector_only?
patch.txt
Though probably more changes to handle sub-resources.

The limitation of one object per plugin/context can't be lifted, because it makes the editor more predictable when unediting objects. However, assuming my above patch is working (not tested), we could have something like is_editing() method for EditorPlugin, which would help to determine whether to edit the object or just open it in the inspector.

Answer 20 · 2024-01-11T19:42:11.000Z

FYI, these are all the changes I went with for working around the errors so far:

Zylann/godot_voxel@f86ab5c
Zylann/godot_voxel@743984b

I noticed that completely removing the "side handling" for the terrain editor plugin actually didn't have harmful effects [anymore?] (at least in use cases I tested). I was quite curious because I thought selecting a sub-inspector would cause edit(null) and make_visible(false) called on the terrain plugin, but it didnt (which is good for me, but I don't know if that's by design in Godot or I'm accidentally exploiting a bug).

I tested your patch: VoxelGraphEditorPlugin still receives make_visible(false) and edit(null) when selecting nodes of the graph. Although, that doesn't happen when I click on the background (which selects the graph itself). Note, the code I use to select the graph again is still using inspect_object without specifying p_inspector_only. Also, inspector prev/next arrows still do the hiding.

Answer 21 · 2024-01-11T20:07:33.000Z

(which is good for me, but I don't know if that's by design in Godot or I'm accidentally exploiting a bug).

This is likely result of the editing context split, which happened in some 4.0 alpha IIRC. Scene tree and editor properties are separate editing contexts.

I tested your patch: VoxelGraphEditorPlugin still receives make_visible(false) and edit(null) when selecting nodes of the graph.

I'd have to look into it, but the module doesn't compile on master.

Answer 22 · 2024-01-11T21:46:29.000Z

I'd have to look into it, but the module doesn't compile on master.

Should be fixed now

Answer 23 · 2024-01-12T11:54:27.000Z

I tested the graph in the newest version. Clicking a graph node will open it in the inspector, with no error printed. Not sure what's the problem. The Terrain button in the toolbar is still visible, but VoxelTerrain is not in the inspector, because it's taken by graph node. That looks like expected behavior.

Answer 24 · 2024-01-12T18:30:27.000Z

I tested the graph in the newest version. Clicking a graph node will open it in the inspector, with no error printed

Forgot to mention: that's because there is still a workaround to prevent the issue from being noticed.
See this: https://github.com/Zylann/godot_voxel/blob/7ebf64cd7e9d5e95a7c2e7e4f13010091c7e72eb/editor/graph/voxel_graph_editor_plugin.cpp#L81-L84
When I say that edit(null) and make_visible(false) are still called even with p_inspector_only=true, I figure this out by enabling verbose output (-v) in my build, so I can see whether the problem still exist or not with the debug print (then my workaround code makes it so it looks like it doesnt happen).

VoxelTerrain is not in the inspector, because it's taken by graph node. That looks like expected behavior.

That's expected.

Answer 25 · 2024-01-15T14:58:45.000Z

When you click GraphNode, it will be edited in the inspector. Any plugin that uses inspector will lose its editing context and receive edit(null). If you want to keep graph editor and edit the graph node, you need to persist the bottom editor. Either permanently, like Shader Editor, or temporarily, using the recently added self-owning mechanism: #81523

Answer 26 · 2024-01-15T19:41:02.000Z

Is it adressing both calls to edit(null) and make_visible(false)?
Unfortunately I don't think I can use the "self owning" thing yet. It's not documented, not exposed to extensions, and when I look at the changes in the theme editor, it uses APIs that are also not exposed. Also not sure what get_next_edited_object means? Why wouldn't this just be passed to can_auto_hide in order to react to what's being edited?

Answer 27 · 2024-01-15T19:44:15.000Z

These two methods are called together. At least in the case that affects your plugin.
Also the API is not exposed, because it was added to fix an internal editor bug. So far no user has expressed a need to have it available, because it's quite advanced and corner case.

Answer 28 · 2024-01-24T16:01:49.000Z

@Zylann mentioned this error occurs when handling the same object by two different plugins, but I have the exact same error when handling subresources with just one plugin.

In my plugin I override _handles(object) like so:

func _handles(object: Object) -> bool:
	return object is Resource

But whenever I select a subresource in the inspector, I get:

editor/editor_node.cpp:8240 - Condition "plugins_list.has(p_plugin)" is true.

What's more, the subresource closes in the inspector, preventing me to edit it any further.

Answer 29 · 2024-01-24T16:11:37.000Z

Well that error appears when you try to edit multiple objects by the same plugin within the same editor context, which is very likely to happen when your plugin handles any resource.