macOS 4.2dev6 (regular and Mono) crash when opening a new project after Jolt installed.

[hawkwood]

Godot version

4.2dev6 regular and Mono

System information

macOS 13.4.1 M1 MacbookPro

Issue description

Dev6 crashes (fails to open) a project with Jolt physics installed.

Crash log attached Godot42d6-jolt-crash.txt

Steps to reproduce

1. Open Godot 4.2dev6 on macOS
2. create new project
3. install Godot Jolt from AssetLib
4. restart Godot to that project
5. see it crash

Minimal reproduction project

JoltTestDev6.zip

[hawkwood]

It's worth noting that dev4 does not exhibit this issue, but dev5 and dev6 do.

[akien-mga]

CC @mihe @dsnopek

[mihe]

It doesn't look to be an issue with GDExtension compatibility specifically, as it crashes in my editor plugin due to get_editor_interface()->get_base_control()->get_theme() now returning nullptr for some reason.

After doing some bisecting it appears that #81130 is the cause for this change in behavior.

Is this an intended breakage, or is it my questionable use of the theme API that's coming back to haunt me? Maybe @YuriSizov can shed some light on this?

[YuriSizov]

@mihe Yes, this is intended. You can fetch the editor theme directly through EditorInterface::get_editor_theme() now.

my questionable use of the theme API

It depends on why you're doing it this way. GDExtensions support defining icons for types via the configuration file, so if that's your need, then yes, this is pretty questionable 🙃 If there is some other reason, let me know! But in general I don't think we expect developers to manually modify the editor theme.

Edit: As for the duplication trick, whenever you modify the theme it sends the changed signal which updates the entire editor UI, so 5 set_icons, 5 whole UI updates. It should be avoided.

[mihe]

Yes, this is intended.

Right, then I guess this technically falls under me not supporting anything but latest stable, and this issue can be closed.

GDExtensions support defining icons for types via the configuration file, so if that's your need, then yes, this is pretty questionable

Yes, I am/was aware of this. I'm sure I won't make a very strong case for not just using this functionality, but in this particular case I was hoping to avoid bundling the extra resources with the extension and just use the existing icons, since these particular nodes are meant to substitute existing ones.

Funnily enough the project I borrowed this trick from has since transitioned to bundling .svg files instead, so maybe I ought to revisit this decision as well.

As for the duplication trick, whenever you modify the theme it sends the changed signal which updates the entire editor UI, so 5 set_icons, 5 whole UI updates. It should be avoided.

Right, yeah, I figured it was something along those lines. Even the one I update I'm causing right now is probably not ideal, so maybe that's reason enough to just do this properly with [icons] instead, assuming that would fare better.

[YuriSizov]

@mihe If you really want to do it directly via the theme, then there is a trick I recently figured, which limits all updates to exactly one. You can create a new theme on the fly, and then use Theme::merge_with on the editor theme to copy all definitions from your temporary one. merge_with() internally sets a flag which ensures that the changed notification only fires once.

You actually have to use this trick now, because you can no longer set the theme back on the control, since it's not on any control and is global now. And it's just cleaner regardless, since you only augment the editor theme this way.

In engine code it would be something like this:

    Ref<Theme> editor_theme = EditorInterface::get_singleton()->get_editor_theme();

    Ref<Theme> temp_theme;
    temp_theme.instantiate();

    Ref<Texture2D> icon_pin = editor_theme->get_icon("PinJoint3D", "EditorIcons");
    Ref<Texture2D> icon_hinge = editor_theme->get_icon("HingeJoint3D", "EditorIcons");
    Ref<Texture2D> icon_slider = editor_theme->get_icon("SliderJoint3D", "EditorIcons");
    Ref<Texture2D> icon_cone_twist = editor_theme->get_icon("ConeTwistJoint3D", "EditorIcons");
    Ref<Texture2D> icon_6dof = editor_theme->get_icon("Generic6DOFJoint3D", "EditorIcons");

    temp_theme->set_icon("JoltPinJoint3D", "EditorIcons", icon_pin);
    temp_theme->set_icon("JoltHingeJoint3D", "EditorIcons", icon_hinge);
    temp_theme->set_icon("JoltSliderJoint3D", "EditorIcons", icon_slider);
    temp_theme->set_icon("JoltConeTwistJoint3D", "EditorIcons", icon_cone_twist);
    temp_theme->set_icon("JoltGeneric6DOFJoint3D", "EditorIcons", icon_6dof);

    editor_theme->merge_with(temp_theme);

Right, then I guess this technically falls under me not supporting anything but latest stable, and this issue can be closed.

I'm not sure exactly how you would check for new API in the extension, but if you can call the new get_editor_theme method only if it exists, you could keep backwards compatibility.

Even the one I update I'm causing right now is probably not ideal, so maybe that's reason enough to just do this properly with [icons] instead, assuming that would fare better.

It's not a big deal... unless every plugin starts to do it, of course. Icons defined in the configuration file are used without relying on the theme entirely, so it should be more optimal indeed.

[mihe]

I just gave [icons] a try and was reminded of another reason why I chose not to use them, which is that they seem to require absolute res:// paths in order to work, thereby more or less requiring that the extension be installed to a specific location, which I'd prefer to avoid. This also differs from how [libraries] behave, which allows for relative paths.

Maybe I'll take you up on your Theme::merge_with solution after all, @YuriSizov. :)

Either way, that'll be for when 4.2-stable is out. Until then I suppose I can throw in a null check in that function and simply not add the icons if that's the case. It'll look a bit strange in the outliner, but at least it won't crash.

[akien-mga]

Closing as it's a downstream godot-jolt issue.

[YuriSizov]

This also differs from how [libraries] behave, which allows for relative paths.

I don't see why we can't do the same for the icons, by the way. If we can resolve a project local path from that, then we can support it for icons as well.

[mihe]

If we can resolve a project local path from that, then we can support it for icons as well.

I'll see if I can make a PR for it.

[YuriSizov]

@mihe I'm making one right now :) Done: https://github.com/godotengine/godot/pull/82842