Fix a crash when plugin tries to call make_mesh_previews on enable

[jsjtxietian]

Fixes #80333
When we update the plugin list, we should check the tree is blocked or not, if blocked, create_item will return nullptr and thus cause crash.

[YuriSizov]

That doesn't look like a good fix. We just end up not updating the list when it is requested because a tree is propagating mouse events. It seems to me that such situation should never arise in the first place, otherwise every tree is potentially error-prone (you either crash or miss an update that you need).

What the solution would look like I'm not sure. For this particular case, though, we should find a way to block make_mesh_previews from running in the same frame as the plugin node is getting ready.

[jsjtxietian]

I agree this is not the perfect solution for this, and such situation should never arise in the first place.

But I find it hard to break the chain, Tree::propagate_mouse_event -> EditorNode::add_editor_plugin -> _gdvirtual__ready_call -> make_mesh_previews -> make a progress bar and destruct -> the window receives NOTIFICATION_WM_WINDOW_FOCUS_IN -> call update_plugins .

It seems like delay any of this chain's function to next frame is dangerous and non trivial. make_mesh_previews is in _ready, and user may ask for it's result for later use, not sure how to do it the easy way.

And in this particular case I think my fix is fine because the update_plugins will be called many times, miss this one won't cause problem. Or maybe we should not call update_plugins so often? What this function actually do is rebuild plugin list instead of update. Have a filewatcher and only update the plugin list when files change instead of every time the window gains focus.

[YuriSizov]

It seems like delay any of this chain's function to next frame is dangerous and non trivial. make_mesh_previews is in _ready, and user may ask for it's result for later use, not sure how to do it the easy way.

That is a problem in and of itself. This method internally moves the main loop two frames ahead. This already can result in unexpected behavior.

[jsjtxietian]

Or we can remove the EditorProgress (maybe will result in usability problem) and the following code inside make_mesh_preview

DisplayServer::get_singleton()->process_events();
Main::iteration();
Main::iteration();

The code does look suspicious to me.

[YuriSizov]

We can remove EditorProgress, yeah. It's notorious and we had to do this in past to fix various issues. As a temporary solution this would be more acceptable, I think.

Ideally, make_mesh_preview should probably work asynchronously, just like the resource preview generator does. That would be a breaking compatibility change, but the current implementation is pretty problematic. cc @KoBeWi who's recently worked on make_mesh_preview.

[KoBeWi]

The easiest and least invasive fix is adding CONNECT_DEFERRED to this line:
https://github.com/godotengine/godot/blob/549fcce5f8f7beace3e5c90e9bbe4335d4fd1476/editor/editor_plugin_settings.cpp#L248C11-L248C11

[jsjtxietian]

Apply @KoBeWi 's suggestion, godot will complain about “Do not use progress dialog (task) while flushing the message queue or using call_deferred()!”, it seems we still need to remove EditorProgress.

Also,

This method internally moves the main loop two frames ahead. This already can result in unexpected behavior.

The code will make the mrp project with plugin enabled crash at start. Without them it's fine. Should I remove them too?

[KoBeWi]

Apply @KoBeWi 's suggestion, godot will complain about “Do not use progress dialog (task) while flushing the message queue or using call_deferred()!”

It does not crash though, so the error is harmless. I'd say the original issue comes from the wrong usage of make_mesh_previews(). We should make sure that the editor does not crash, but errors are fine.

[YuriSizov]

I'd say the original issue comes from the wrong usage of make_mesh_previews().

What do you think about making it asynchronous, like EditorResourcePreview? That should avoid any unexpectedness on the architectural level.

[KoBeWi]

That could work, but it would require a signal for finishing and it's a compatibility breakage (on behavior level).
The Main::iteration() thing caused me some problems in another PR, so I'd be in favor of removing it if it's possible.

[jsjtxietian]

Based on your discussion, I guess for this crash we can just add CONNECT_DEFERRED to update_plugins.

Should I remove Main::iteration() too? Why do we have it in the first place, to make the editor progress bar more responsive?

[KoBeWi]

Main::iteration is required to render the meshes. It forces the MainLoop to progress as if a frame has passed.
Removing it would make the deferred flag unnecessary (at least for the purpose of this fix), but it would also require adjusting the make_mesh_previews() behavior.

[YuriSizov]

Should I remove Main::iteration() too? Why do we have it in the first place, to make the editor progress bar more responsive?

For the record, the progress dialog makes itself responsive by also calling Main::iteration(); every time you progress it to the next step. Which only adds to the mess here.

[KoBeWi]

It's basically a temporary fix, but it's harmless, so probably fine.

[YuriSizov]

This fixes the crash, though I'm not sure make_mesh_previews is working as expected in the MRP. On the first call it doesn't seem to return anything.

But that's a bigger problem, as discussed. This should be fine for now to prevent the most problematic part of it.

[YuriSizov]

Thanks!

[YuriSizov]

Cherry-picked for 4.1.2.