Avoid notifying editor-only property changes

[ArneStenkrona]

Related issue: #61343

Ensures that Control::_set_layout_mode() only sets
"_edit_layout_mode" meta if the layout mode is LAYOUT_MODE_ANCHORS
because the alternative, LAYOUT_MODE_POSITION, is the default
value and not needed in the meta.

This fixes #61343

[KoBeWi]

Makes sense and fixes the issue, but not sure if it won't break something with LAYOUT_MODE_CONTAINER or LAYOUT_MODE_UNCONTROLLED as they remove the meta too.

CC @YuriSizov

[YuriSizov]

@KoBeWi If your original suspicion was correct, then keeping the position layout mode that has been removed 4 commits ago would be a sufficient fix. However, later you yourself changed how checks work and removed has_* checks, so I'm not sure this would even work the same at this point. It's bad that the code is so brittle, sorry for that. But some checks were there specifically to prevent unnecessary updates and to account for possible ambiguity between runtime values and stored values (and stored values may also be missing).

All in all, the logic has changed too much for me to be sure, so I trust your testing more. I'd have to retest every commit done since that touched the meta to figure out possible regressions. In the original code the container and uncontrolled modes should be determinable at runtime regardless of the stored values.

PS. I'm not even sure why this change fixes anything. For position mode meta was immediately removed, so the code should be equivalent, and the change is superficial.

[YuriSizov]

Yeah, okay, I'm a bit more confident now that this PR doesn't change the behavior at all, so I'm not sure that the issue has been correctly identified. I agree with KoBeWi's original thought that #59185 is the culprit, and I think the fix is to actually not remove layout mode meta for position. That PR seems to be the only one that actually changed behavior, while others seem to be preserving it.

So the proper fix should be to remove remove_meta("_edit_layout_mode"); and test the reproduction then.

[KoBeWi]

So the proper fix should be to remove remove_meta("_edit_layout_mode"); and test the reproduction then.

This would reintroduce #58940 though.

[YuriSizov]

This would reintroduce #58940 though.

We already store this meta if it's not the position mode. It just seems that storing it when it is the position mode is not actually unnecessary 🙃

Edit: If we can make the code clearer and avoid using meta, it'd be better. I'm just thinking short term fixing it to remove the regression. Like we've discussed, using some sort of editor metadata would probably be the way to solve this without affecting users anymore.

[KoBeWi]

This PR fixes the regression though, even if it doesn't make sense that it works. Unless you find some new issue it creates, it should be good to merge.

[YuriSizov]

This PR fixes the regression though, even if it doesn't make sense that it works.

I have an idea. Metadata was exposed to the inspector after the initial layout PR, right? So setting it, at any point, may trigger the inspector to update. Which isn't great, given that editor-only meta properties shouldn't even be editable in the inspector in the first place. And while this PR doesn't change the logic, it reduces the number of calls to set_meta().

Unless you find some new issue it creates, it should be good to merge.

I'm not a fan of changing code further from the state that worked without understanding the changes. I won't block this PR if you insist on this solution, but this makes it harder and harder to maintain this delicate piece of logic based on hacks and good will. 👀

[KoBeWi]

I actually found some flaws in the current code. They don't seem to break anything (in an obvious way at least), but we should probably fix it.

First thing is that we need a default value for _edit_layout_mode. When setting layout mode to this value, don't store the meta. Then the code should consistently use the default value when it doesn't exist. I checked other layout modes and they are determined in a different way, so meta value is irrelevant there.

The problem I found in the current code is that it doesn't use default value.
Here:

godot/scene/gui/control.cpp

Line 1502 in 9b78d68

if ((int)get_meta("_edit_layout_mode", p_mode) != (int)p_mode) {

If the value doesn't exist, it will use the current value as default. That's not how defaults work...
Similar thing here:

godot/scene/gui/control.cpp

Line 1591 in 9b78d68

if (get_meta("_edit_layout_mode", LayoutMode::LAYOUT_MODE_ANCHORS).operator int() != LayoutMode::LAYOUT_MODE_ANCHORS) {

It uses LAYOUT_MODE_ANCHORS as default, while the code changes in this PR assume LAYOUT_MODE_POSITION as default.

So here's what I propose: add const int DEFAULT_LAYOUT_MODE = LayoutMode::LAYOUT_MODE_POSITION then change every get_meta for _edit_layout_mode to get_meta("_edit_layout_mode", DEFAULT_LAYOUT_MODE).

As a topping to these changes, change this thing:

godot/scene/gui/control.cpp

Lines 1540 to 1544 in 9b78d68

	if (has_meta("_edit_layout_mode")) {
	return (LayoutMode)(int)get_meta("_edit_layout_mode");
	}
	// Or fallback on default.
	return LayoutMode::LAYOUT_MODE_POSITION;

To return (LayoutMode)int(get_meta("_edit_layout_mode", DEFAULT_LAYOUT_MODE));

It changes some logic, but I did a quick test and doesn't seem to break things. Will test again after you apply these changes.

see above comment

[YuriSizov]

If the value doesn't exist, it will use the current value as default. That's not how defaults work...
It uses LAYOUT_MODE_ANCHORS as default, while the code changes in this PR assume LAYOUT_MODE_POSITION as default.

I believe you've introduced this code when you removed has_* checks. This preserved the original logic, as the logic used has_* checks differently in different contexts. So you use defaults here to return values that would let the code to work as expected. (E.g. returning LAYOUT_MODE_ANCHORS as default allows it to work correctly when there is no stored value). None of these checks actually need to rely on the default value.

I agree with the last suggestion, though, that can be simplified.

[KoBeWi]

I believe you've introduced this code when you removed has_* checks. This preserved the original logic, as the logic used has_* checks differently in different contexts.

I know why the code was there, I just questioned whether keeping the old logic is actually necessary. _edit_layout_mode isn't used much, so it's likely it won't break anything.

[ArneStenkrona]

@KoBeWi I've read through this discussion and applied the changes you suggested. Though when playing around in the editor to make sure the issue is still resolved I noticed this: When starting to drag a ColorRect, the first movement seem subject to a noticable lag/delay, after which the movement becomes smooth.

Now I know to little to guess why this is and don't want to speculate. Though I did try the 3.4.2 stable release which I had sitting around and it does not have an initial lag when dragging control nodes. Though I'll check if this laging existed before #59185.

[KoBeWi]

Something is still wrong. When layout mode is anchors, the slowdowns are still happening :/

[ArneStenkrona]

@KoBeWi are you sure that the performance issue was introduced in #59185? I checked out this PR and dragging is smooth for me. When you check out PR #59185 do you experience the lagging?

[YuriSizov]

I have an idea. Metadata was exposed to the inspector after the initial layout PR, right? So setting it, at any point, may trigger the inspector to update. Which isn't great, given that editor-only meta properties shouldn't even be editable in the inspector in the first place. And while this PR doesn't change the logic, it reduces the number of calls to set_meta().

You can see if this ^ is plausible. That would explain why your initial fix has an effect, for one, and why regression hasn't been noticed before.

[KoBeWi]

are you sure that the performance issue was introduced in #59185? I checked out this PR and dragging is smooth for me. When you check out PR #59185 do you experience the lagging?

Not sure what you mean by "checking out the PR", I just re-added the 2 lines it removed and it fixed the lag. I didn't do much more testing.

Metadata was exposed to the inspector after the initial layout PR, right? So setting it, at any point, may trigger the inspector to update.

That would be interesting. Editor metadata is not visible in the inspector, so if it was causing inspector refresh then it's a bug.

[ArneStenkrona]

Not sure what you mean by "checking out the PR", I just re-added the 2 lines it removed and it fixed the lag. I didn't do much more testing.

Checking out as in check out the resulting commit of the PR. I checked it out and dragging was smooth. So I guess while re-adding the lines fixes the issue, they did not cause the performance issue by themselves. I guess this can be explained by what YuriSizov said about setting metadata triggering inspector update.

I'm currently running git bisect to see where I first notice the performance regression but it will take some time.

EDIT: I misspoke (or rather, was confused). The merge commit of PR 59185 on godotengine/godot does indeed introduce the lag. But on the fork that created the PR, the PR does not introduce the lagging. I'll keep investigating.

[ArneStenkrona]

I have an idea. Metadata was exposed to the inspector after the initial layout PR, right? So setting it, at any point, may trigger the inspector to update. Which isn't great, given that editor-only meta properties shouldn't even be editable in the inspector in the first place. And while this PR doesn't change the logic, it reduces the number of calls to set_meta().

I looked into this and it seems like the correct hypothesis. The commit 09b951b, which makes properties editable in inspector, is the first that causes the regression if combined with the changes introduced in 59185. Specifically, 09b951b adds calls to notify_property_list_changed() in Object::set_meta() (in object.cpp) which if commented removes the regression.

I agree that this PR is just a band-aid and not an actual solution. As said earlier, editor-only properties should not be editable in the inspector. So if these were not visible in the inspector, there would be no need to call notify_property_list_changed() when setting editor-only meta. Is there a way to identify if a property is editor-only? 09b951b's commit message states "Properties are editable in inspector (unless metadata name begins with _)", does that imply that properties that start with "_" are editor-only? If so it feels like there is a straight forward way to resolve the issue, by simply not triggering the inspector updates in these cases.

@YuriSizov @KoBeWi do you think I have the right idea here? Then I'd be happy to draw up a new solution and create a PR. Also, since this current PR is not viable should I use the original issue for further discussion?

[YuriSizov]

Yes, I think it's reasonable that the same logic that filters properties in the inspector should be used to remove a call for update. Just to be sure, look up the code that filters meta properties, and replicate it in set_meta().

You can also use this PR still, remaking it for the new solution, changing the commit message and the title. But if you want to preserve it, sure, we can move back to the issue and close this.

[KoBeWi]

does that imply that properties that start with "_" are editor-only?

Yes, the metadata starting with _ don't show in the inspector, so changing them shouldn't affect the inspector.

[ArneStenkrona]

It's been a hectic weekend but I finally have some time to work on this. There seems to be something weird going on with EditorInspector (editor_inspector.cpp). notify_property_list_changed() triggers a call to _edit_request_change() which sets the update_tree_pending flag that triggers inspector updates. This method does take a parameter for the changed property, which should make it easy for us to only set the flag if the property isn't editor-only. But this parameter is set to an empty string in practice so not sure what that is about.

Anyways, I have some ideas what's going on and don't anticipate the solution will be too hard.. Just wanted to let people know that this is still being worked on.

[YuriSizov]

There seems to be something weird going on with EditorInspector (editor_inspector.cpp). notify_property_list_changed() triggers a call to _edit_request_change() which sets the update_tree_pending flag that triggers inspector updates. This method does take a parameter for the changed property, which should make it easy for us to only set the flag if the property isn't editor-only. But this parameter is set to an empty string in practice so not sure what that is about.

There is no need to go so in-depth with this. notify_property_list_changed() is the culprit, and you should simply avoid calling it in set_meta() for meta properties that won't be displayed. I looked at how #59452 filters them in inspector, and all that is checked there is if name.begins_with("metadata/_"). So for set_meta() you can check if p_name begins with an underscore, and avoid calling notify_property_list_changed() if that's the case.

And thanks for doing this!

[ArneStenkrona]

There is no need to go so in-depth with this. notify_property_list_changed() is the culprit, and you should simply avoid calling it in set_meta() for meta properties that won't be displayed

That was my first thought. However, I considered that there might be subscribers other than EditorInspector that are listening to property_list_change that might have interest in changes to editor-only properties. However, if you think that this is not a concern I will go with the simple option.

And thanks for the encouragement! I'm happy to help

[ArneStenkrona]

I assume we are waiting for a review from the core group. Or is there anything I change in the PR?

[reduz]

Looks like this is patching something that is hacky, should not not be better to fix whatever is causing the problem?

[YuriSizov]

Discussed with @reduz and decided it's better to move away from metadata all together to solve the problem with controls updating. The change that we've agreed upon in this discussion before affects core in a way that we don't really want (it's fine to have this hack in editor code, but not in core code).

So we should replace control.cpp's use of metadata with hidden properties. I'll work on it this week. Sorry for taking so long, and thanks for contributing nonetheless!

[KoBeWi]

Isn't this change useful anyway? The inspector doesn't need to be refreshed for _meta properties. This also applies to user code.

[YuriSizov]

@KoBeWi I agree, but @reduz finds this solution hacky and doesn't want to include hacky code in core. It's okay to have such code in editor, but not in core.