Skip to content

Make settings_changed signal less emitted #106883

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

KoBeWi
Copy link
Member

@KoBeWi KoBeWi commented May 27, 2025

When testing another PR it kind of annoyed me that changing project's title will update 3D editor's viewports with every letter you type. I looked into it and discovered that:

  • ProjectSettings will try to emit the signal at launch for every defined setting (fortunately it's limited to once per frame)
  • The signal is emitted every time you change a setting, potentially many times per second
  • The signal is emitted after a timeout from changing settings
  • The signal is emitted every time you save

The last two actually have the same cause (reminder #100244), i.e. the settings are "changed" when saving, which emits changed signal needlessly.

I added a way to block emitting the signal and applied it in GLOBAL_DEF and in ProjectSettings' inspector. Now the signal is only emitted on the save Timer's timeout and when settings are changed from code. The downside is that when you change a setting in the editor, you may not see the effect immediately (e.g. when changing project title).

EditorSettings have similar problem, but almost nothing uses its equivalent signal. Most of the editor uses the notification, which is emitted by the settings dialog.

@KoBeWi KoBeWi added this to the 4.x milestone May 27, 2025
@KoBeWi KoBeWi requested a review from a team as a code owner May 27, 2025 21:09
@KoBeWi KoBeWi force-pushed the irresistible_urge_to_spam_changed_signal branch from c3cc196 to e90e60e Compare May 27, 2025 21:11
@lawnjelly
Copy link
Member

Great that you are working on this, I wasn't aware that #100244 didn't lead to a fix.

I'll try and review and test soon, and check there isn't a similar problem in 3.x. 👍

if (!ps->has_setting(p_var)) {
ps->set_block_changed(true); // Prevents emitting change during initialization.
ps->set(p_var, p_default);
ps->set_block_changed(false);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could this lead to order of construction bugs?

If the aim is to avoid signals during setup, could we alternatively set_block_changed explicitly during setup? (Or will this not deal with delayed loading)

My first thoughts would be, explicitly block during setup, then when starting the main loop, remove the block and send out a changed signal, to ensure all parts are up to date and no order of construction issues.

Also if a user intends to add their own ProjectSetting later, they may want to receive the signal on the first change.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem is that GLOBAL_DEFs are scattered all over the code, so it's more reliable to disable them here.

@lawnjelly
Copy link
Member

When I add a print_line() just before emitting the signal, I only see one signal when loading both a game, and the editor (for trucktown), suggesting it is working correctly already at startup for these.

Is there a particular scenario where this problem occurs at startup / loading?

@KoBeWi
Copy link
Member Author

KoBeWi commented May 28, 2025

Is there a particular scenario where this problem occurs at startup / loading?

The signal should not emit at startup at all, because the settings are being initialized, not changed. Unless something relies on this behavior, but I don't know anything like that.

@lawnjelly
Copy link
Member

lawnjelly commented May 29, 2025

The signal should not emit at startup at all, because the settings are being initialized, not changed. Unless something relies on this behavior, but I don't know anything like that.

Disagree here, I think:

  • When initializing, the settings have changed. Initialization is a type of change. Put another way, if there was no change, we would never need to initialize. 😛
  • The signal is a convenient place to update local stuff after all initialization is complete, and avoiding any order of construction issues

It is likely useful for third party code to just put their settings update code in one place and have it done like this, and without it, users are likely to end up scratching their heads thinking "why isn't it called? is this a bug?".

For the main engine, it should imo be tolerant to at least a single settings_changed signal at startup. Anything heavy should already have a noop check for cases where the setting has not changed (e.g. changing shadow atlas sizes etc). If this operation is not slick, then we have bigger problems to worry about.

Also, removing this signal at initialization is compat breaking I think.

@KoBeWi
Copy link
Member Author

KoBeWi commented May 29, 2025

The launch signal would be useful if it was guaranteed to be emitted for every node. If you enable a plugin after the editor has loaded, you won't receive it. So realistically user plugins can't rely on it, and core plugins risk having bugs if they happen to load too late (or too early).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants