ProjectSettings add dirty flag and project_settings_changed signal [3.x]

[lawnjelly]

Most frames there will be no change in project settings, and it makes no sense to read settings every frame in case of changes, as a large number of string compares are involved.

This PR adds a signal to ProjectSettings that can be used in most cases to drive reading settings.

In addition a function ProjectSettings::has_changes() is provided for objects outside the signal system (e.g. Rasterizers).

Alternative version of #45956 for 3.x
Addresses godotengine/godot-proposals#2155

Notes

• I had noticed this as an inefficiency a while ago and intended to fix it. The string comparisons etc in finding project settings makes it relatively slow, and appears in profiles of the editor when nothing should be running, needlessly using CPU.
• It turned out that it was already solved in master, however there are two potential concerns with that solution:

1. The signal in 4.x is confined to the editor. This signal is potentially generally useful outside the editor, in user tools and games (although other workarounds are possible, it's a nice general solution to have such a signal available).
2. In this PR, another mechanism is provided for systems that cannot receive signals, this is especially relevant in the Rasterizer, and means we can now practically update a lot more settings without needing an editor restart, because there is now no need for expensive checks on the fly.

• The use of an integer with 2 values may seem strange, but it is to cope with the situation where a project setting is set during iteration AFTER it is read. This could otherwise result in missed changes.
• This needs a look through by @reduz as there may be some considerations I'm not aware of in terms of total design.

[akien-mga]

I'm not super happy about having something like this injected in the utmost core Main::iteration. Feels hacky, and not just because it has "dirty" in the name :)

[lawnjelly]

Ah yes fair point, I agree. 👍 There's probably a better place for it I'll try and have a hunt (any suggestions welcome).

I might rename it to update() so it sounds less dirty. And can be used for anything else that requires an update.

[lawnjelly]

I've now moved the ProjectSettings::update() to the SceneTree::idle() rather than the main iteration. This is a bit neater but does mean that anyone writing their own SceneTree would have to call the ProjectSettings::update() function themselves.

I've also noticed that the new ImportDefaultsEditor was using its own bespoke "project_settings_update" signal, presumably because there was no global system for this implemented at the time. I've modified that logic to now use the new global mechanism.

I've also included some example code in comments in the ProjectSettings header, to make it more obvious how to use the new functionality.

[akien-mga]

I still feel that it's a weird pattern that the main loop (whether Main::iteration or SceneTree::idle) should be responsible for handling the dirty state of one specific component.

To me a dirty pattern would typically be implemented within the class and transparent to the rest of the codebase. It should update on demand if dirty (e.g. ProjectSettings::get_setting() would update if settings are dirty).

Alternatively, is there a reason why this can't go through MessageQueue with simply deferring update until the next idle frame?
Set dirty, then call_deferred("update") to handle the dirty state.

[lawnjelly]

I still feel that it's a weird pattern that the main loop (whether Main::iteration or SceneTree::idle) should be responsible for handling the dirty state of one specific component.

This may be a limitation on my knowledge / experience of Godot, there may well be a better way of doing this. As you say if the MessageQueue can be used given that ProjectSettings is derived from Object then that might be a good alternative, I'll have a look and see whether this can work. 👍 (there may end up being multiple updates if multiple settings are changed on a frame, but this may not be a problem in practice).

Most objects I've used before are part of the SceneTree, or updated via the servers, but the Singletons don't seem to get an "update", so there's no particular example that I can immediately think of for reference here, so I'm improvising! 😀

To me a dirty pattern would typically be implemented within the class and transparent to the rest of the codebase.

I'm a bit lost here, I don't know what you mean. There are two classes, ProjectSettings, and a class which wants to read updates to it. Multiple classes could potentially want to read the same ProjectSetting, so you can't keep a track of individual dirty states for each item in ProjectSettings. Perhaps if the ProjectSettings flag was named has_changes that would be easier?

It should update on demand if dirty (e.g. ProjectSettings::get_setting() would update if settings are dirty).

This is surely what we are trying to avoid - we shouldn't have to call get_setting() at all when not necessary. One check is essentially faster than multiple checks (i.e. the has_changes() method), but better still is to use a push mechanism (via signal) rather than pull, so absolutely nothing is done in normal operation, only when a setting is changed.

[akien-mga]

You can do something like (pseudo-code):

void ProjectSettings::emit_changed() {
	if (!dirty) {
		return;
	}
	emit_signal("project_settings_changed");
	dirty = false;
}

void ProjectSettings::set(...) {
	// blabla something caused a setting to change.
	dirty = true;
	call_deferred("emit_changed");
}

So possible multiple call_deferred("emit_changed"); will be queued in the MessageQueue, but when they get resolved (at the idle step) only the first one would actually trigger an emit, since the dirty flag is then reset.

[lawnjelly]

This does sound better. To be honest I wasn't aware that we could use call_deferred on something not in the SceneTree etc, if that works it seems a better solution than an update. 😀

[lawnjelly]

Ah no joy with call_deferred(). 🙁

It looks like the machinery for call_deferred() is created after there are calls to set project settings. So when it calls call_deferred() it crashes. That kind of makes sense, being a singleton we have to be careful of construction / destruction ordering issues. I'll keep trying but I don't think it will work.

Still welcome alternative approaches though.

[akien-mga]

Maybe you can bypass the whole dirty/changed system for as long as MessageQueue doesn't exist yet. That should only be on startup (possibly shutdown) and at that point we probably don't need to fully optimize propagating changes to not-yet-initialized components?

Otherwise I guess this is something that should be discussed with @reduz who would probably know how to do this in the cleanest way for the engine's current architecture. But that will likely have to wait since he's busy this week.

[lawnjelly]

Maybe you can bypass the whole dirty/changed system for as long as MessageQueue doesn't exist yet. That should only be on startup (possibly shutdown) and at that point we probably don't need to fully optimize propagating changes to not-yet-initialized components?

Yeah but on the other hand there's a big danger that this would end up so hacky, we might just as well have called update() and be done with it... 😀

So yes probably best to just wait till reduz is settled then get his input. There's no huge hurry on this as it's just an enhancement. 🙂

[reduz]

Seems ok to me

[akien-mga]

Needs a rebase.

[lawnjelly]

Rebased, just needed an extra:

const bool use_32_bpc_depth = ProjectSettings::get_singleton()->get("rendering/quality/depth/use_32_bpc_depth");
viewport->set_use_32_bpc_depth(use_32_bpc_depth);

which had been added since the PR was originally made.

So should be good to go now. 👍

[akien-mga]

Thanks!

[Calinou]

I wonder if NOTIFICATION_PROJECT_SETTINGS_CHANGED would be worth adding to simplify some C++ editor code handling. This way, you don't need to connect a signal to call functions when a project setting changes.

[lawnjelly]

I wonder if NOTIFICATION_PROJECT_SETTINGS_CHANGED would be worth adding to simplify some C++ editor code handling. This way, you don't need to connect a signal to call functions when a project setting changes.

Yes feel free to add this. reduz' PR and the proposal were for a signal but a notification can be simpler to follow in c++ code imo. 👍 (I expect this can be done in the same place it sends the signal.)