Add settings_changed signal to ProjectSettings

[KoBeWi]

Closes #62020

This is done differently than in 3.x and the signal is named settings_changed for consistency with EditorSettings.

For some reason I get a crash when closing though:

CrashHandlerException: Program crashed
Engine version: Godot Engine v4.0.alpha.custom_build (111a3ca09711862e08ced7fa445801e2b89ffe4c)
Dumping the backtrace.
[0] mtx_do_lock (d:\a01\_work\2\s\src\vctools\crt\github\stl\src\mutex.cpp:91)
[1] std::_Mutex_base::lock (C:\Program Files (x86)\Microsoft Visual Studio\2019\Community\VC\Tools\MSVC\14.29.30133\include\mutex:51)
[2] MutexImpl<std::recursive_mutex>::lock (C:\Users\Tomek\Desktop\Godot\godot\core\os\mutex.h:48)
[3] MutexLock<MutexImpl<std::recursive_mutex> >::MutexLock<MutexImpl<std::recursive_mutex> > (C:\Users\Tomek\Desktop\Godot\godot\core\os\mutex.h:67)
[4] GDScript::~GDScript (C:\Users\Tomek\Desktop\Godot\godot\modules\gdscript\gdscript.cpp:1244)
[5] GDScript::`scalar deleting destructor'
[6] memdelete<GDScript> (C:\Users\Tomek\Desktop\Godot\godot\core\os\memory.h:111)
[7] Ref<GDScript>::unref (C:\Users\Tomek\Desktop\Godot\godot\core\object\ref_counted.h:223)
[8] Ref<GDScript>::~Ref<GDScript> (C:\Users\Tomek\Desktop\Godot\godot\core\object\ref_counted.h:233)
[9] GDScriptLambdaCallable::~GDScriptLambdaCallable (C:\Users\Tomek\Desktop\Godot\godot\modules\gdscript\gdscript_lambda_callable.h:62)
[10] GDScriptLambdaCallable::`scalar deleting destructor'
[11] memdelete<CallableCustom> (C:\Users\Tomek\Desktop\Godot\godot\core\os\memory.h:111)
[12] Callable::~Callable (C:\Users\Tomek\Desktop\Godot\godot\core\variant\callable.cpp:312)
[13] VMap<Callable,Object::SignalData::Slot>::Pair::~Pair
[14] VMap<Callable,Object::SignalData::Slot>::Pair::`scalar deleting destructor'
[15] CowData<VMap<Callable,Object::SignalData::Slot>::Pair>::_unref (C:\Users\Tomek\Desktop\Godot\godot\core\templates\cowdata.h:214)
[16] CowData<VMap<Callable,Object::SignalData::Slot>::Pair>::~CowData<VMap<Callable,Object::SignalData::Slot>::Pair> (C:\Users\Tomek\Desktop\Godot\godot\core\templates\cowdata.h:409)
[17] VMap<Callable,Object::SignalData::Slot>::~VMap<Callable,Object::SignalData::Slot>
[18] Object::SignalData::~SignalData
[19] KeyValue<StringName,Object::SignalData>::~KeyValue<StringName,Object::SignalData>
[20] HashMapElement<StringName,Object::SignalData>::~HashMapElement<StringName,Object::SignalData>
[21] HashMapElement<StringName,Object::SignalData>::`scalar deleting destructor'
[22] memdelete<HashMapElement<StringName,Object::SignalData> > (C:\Users\Tomek\Desktop\Godot\godot\core\os\memory.h:111)
[23] DefaultTypedAllocator<HashMapElement<StringName,Object::SignalData> >::delete_allocation (C:\Users\Tomek\Desktop\Godot\godot\core\os\memory.h:205)
[24] HashMap<StringName,Object::SignalData,HashMapHasherDefault,HashMapComparatorDefault<StringName>,DefaultTypedAllocator<HashMapElement<StringName,Object::SignalData> > >::erase (C:\Users\Tomek\Desktop\Godot\godot\core\templates\hash_map.h:347)
[25] Object::~Object (C:\Users\Tomek\Desktop\Godot\godot\core\object\object.cpp:1882)
[26] _GLOBAL_DEF (C:\Users\Tomek\Desktop\Godot\godot\core\config\project_settings.cpp:1273)
[27] ProjectSettings::`scalar deleting destructor'
[28] memdelete<ProjectSettings> (C:\Users\Tomek\Desktop\Godot\godot\core\os\memory.h:111)
[29] Main::cleanup (C:\Users\Tomek\Desktop\Godot\godot\main\main.cpp:2941)
[30] widechar_main (C:\Users\Tomek\Desktop\Godot\godot\platform\windows\godot_windows.cpp:177)
[31] _main (C:\Users\Tomek\Desktop\Godot\godot\platform\windows\godot_windows.cpp:197)
[32] main (C:\Users\Tomek\Desktop\Godot\godot\platform\windows\godot_windows.cpp:211)
[33] WinMain (C:\Users\Tomek\Desktop\Godot\godot\platform\windows\godot_windows.cpp:225)
[34] __scrt_common_main_seh (d:\a01\_work\2\s\src\vctools\crt\vcstartup\src\startup\exe_common.inl:288)
[35] BaseThreadInitThunk
-- END OF BACKTRACE --

Not sure why.

[lawnjelly]

Would it make sense to modify #45956 at the same time, and use the name project_settings_changed consistently from ProjectSettings rather than EditorNode?

If we have decided that having such a signal available outside the editor is more useful than editor only, it might be prudent to remove the EditorNode signal (and maybe EditorPlugin, not sure on how this works) and keep a single consistent way of using this (for less confusion in the future). Better to break compatibility here before 4.0 imo. Something to check with @reduz ?

[lawnjelly]

It is probably crashing BTW because of call_deferred(). I tried this in #53296 and faced a similar problem, call_deferred() relies on machinery that is not available at the time of the call, which is why I went back to the more primitive update() approach for ProjectSettings.

See #53296 (comment)

[reduz]

The main problem with this IMO is that we don't want people writing plugins using this signal because it can be called a ton of times per frame by some of the editors that store their data there, as this endangers running into very inefficient updates. We already have one that saves the project settings and calls a signal a small amount of time after the last edit, so we should most likely expose this to EditorPlugin than adding a singal to ProjectSettings, which has little use.

[KoBeWi]

@reduz The signal is limited to emit up to once per frame.

[Calinou]

This PR could be helpful to implement godotengine/godot-proposals#2455 and other project settings to impact all BaseMaterial3Ds at run-time, as I don't see another way to do it without large changes to RenderingServer. User-provided shaders could also benefit from this to offer run-time settings that users can change in options menus.

[akien-mga]

Is the crash mentioned in the OP still happening with this PR? Should likely be a draft if so.

[KoBeWi]

Doesn't seem to crash anymore. I think I fixed it and forgot to comment.

[lawnjelly]

It looks fine to me. 👍

My only remaining thoughts are that the resulting situation is a little messy:

#45956 still causes the editor node to have a signal called project_settings_changed, when now both these use cases could seem more sensible to use the same signal from the actual project settings. But maybe the editor node signal is still needed for backward compatibility with addons.

One slight alternative would be to call them both project_settings_changed. Then maybe do a followup PR to change the editor stuff to use this PR's ProjectSettings signal, and just retain the EditorNode signal for backward compatibility with addons, and mark it as deprecated.

That's more an architectural decision though.

EDIT : Also - probably needs running by @reduz again to make sure he is ok with this approach, as he voiced concerns above.

[YuriSizov]

the editor node to have a signal called project_settings_changed, when now both these use cases could seem more sensible to use the same signal from the actual project settings. But maybe the editor node signal is still needed for backward compatibility with addons.

EditorNode is not exposed to the API, so the signal in it can be removed. However, EditorPlugin has project_settings_changed. We can't remove it because of compatibility, but it can use ProjectSettings directly now instead of relying on EditorNode.

One slight alternative would be to call them both project_settings_changed.

Don't think there is a point in that. When emitted by ProjectSettings it's just redundant, and in EditorPlugin we do need a qualifier because it could also refer to the editor settings. I think it's fine as is.

Also - probably needs running by @reduz again to make sure he is ok with this approach, as he voiced concerns above.

The concern was that it could be called multiple times a frame, but that's not the case.

[YuriSizov]

Looks good to me, but may be worth updating EditorPlugin to rely on this and removing the signal from EditorNode as discussed above.

[KoBeWi]

Should project_settings_changed be deprecated?

[YuriSizov]

In plugins? Yeah, I guess we can mark it as deprecated and suggest using the new signal.

[KoBeWi]

I looked into changes required to remove the EditorNode signal and it seems to be used a lot. I think it can be done in a separate PR; I'll make a follow-up soon.

[akien-mga]

Thanks!