godot
godot copied to clipboard
godotengine
Pass current value to `EditorInterface` node/property popups
Fixes #94322
As outlined in the issue, while these new methods are nice in Godot 4.3, they don't seem to hit the mark in terms of being fully user friendly without being able to specify a current value, should these be used in cases where the addon already has an existing value and would prefer to highlight that in the dialog when its shown.
You need to bind compatibility methods, see here
Even if these methods were introduced in an alpha/beta build of Godot 4.3?
The practical reason behind this PR in the first place was to address the shortcomings of the original implementation so that when it gets into a stable build, these APIs are reasonably feature complete and to avoid needing compatibility methods.
The current_value in popup_property_selector() seems to do nothing. Nothing is selected no matter what I tried to use.
The
current_valueinpopup_property_selector()seems to do nothing. Nothing is selected no matter what I tried to use.
I wonder if that is a bug in the actual base dialog implementation because all this PR allows you to pass the same value the editor would pass when using this dialog in the engine or C++ modules.
I'll take a look after work today and get back to you, @KoBeWi.
Hi @KoBeWi so I checked the code in the property_selector.h and property_selector.cpp files and it does seem that while one can pass the current "selected" value into the dialog, the logic within PropertySelector::_update_search does not actually do anything with the member variable selected.
Looking more closely at the uses of PropertySelector, there are 8 public methods that pass the current value and store that in the selected member variable; however, while the member variable's value is set by those 8 methods, it's not used internally in any of the presentation logic sadly.
So I think for completeness, we should at the very least keep the ability to pass this value on the EditorInterface. This minimizes the need to introduce any compatibility method after-the-fact. I think the question is really how do we want to address this bug, as a part of this PR or a follow-up.
I'm of the opinion that give where we are with 4.3, we should likely take care of addressing the PropertySelector implementation bug separately at the outset of 4.4 and then consider backporting that fix. That would give us enough time to battle test not only the variant exposed by the EditorInterface, but the other 7 public methods on the class in case the editor would like to make use of those in the future. What do others think? @akien-mga ?
Well, you added a new argument and it didn't require any compatibility code, so looks like adding arguments in the future is not a problem. I think this can be pushed to 4.4 and let you can finish it properly.
Well, you added a new argument and it didn't require any compatibility code, so looks like adding arguments in the future is not a problem. I think this can be pushed to 4.4 and let you can finish it properly.
It doesn't require compatibility code now because this is a new API in 4.3.
If a new argument is added in 4.4, it will indeed require a compat binding. But that's no big deal so if the added API doesn't work, I think it's better to take the time to do it right (otherwise we'd add something in 4.3 where the docs are actually wrong about what the new argument does).
We could say in the docs to say that the argument is unused 🤔 And then change it once it's fixed.
To me there's not much gain adding an argument that does nothing in 4.3, to fix it later. It still means that people who want to make EditorPlugins using this API will have to special case 4.3.0 because while they can use the API, it won't work.
And making it work in 4.3.x would be confusing IMO, documentation typically doesn't state in which version an API was made operative, and the docs will be updated once the bug is fixed to remove mention of it not working.
So I'd say this should wait for 4.4. We're in RC stage, it's no longer the time to change the behavior of APIs. Adding a compat method in 4.4 isn't a big deal, we do that all the time for other APIs.
Can someone point me to an example of compatibility methods, I plan to update this PR with the fix & I'd like to get this in merge shape asap. Thanks!
@KoBeWi added logic for the "current_value" matching for properties/methods & added compatibility methods. Would you mind giving this another pass from your PoV? Thanks!
Selecting property now works, but it would be nice if the dialog scrolled to the selected item.
Almost. The view is not centered on the property, but awkwardly cut at the top:
Almost. The view is not centered on the property, but awkwardly cut at the top
I'll take a look and see if I can reproduce it; however, I used scroll_to_item with center on item 🤔
Well my test code is this:
@tool
extends EditorScript
func _run() -> void:
EditorInterface.popup_property_selector(get_scene().get_node("Icon"), Callable(), [], "z_index")
(requires "Icon" node in the current scene)
Thanks @KoBeWi, fixed. It appears I needed to delay the scroll_to_item until the end of the frame since building of the list was still in-flight and the offset would obviously change due to additional items added.
You've added your own compatibility file, this isn't correct, it should be added to the existing 4.3-stable.expected (creation of these files is handled by maintainers, you don't need to do it yourself)
You've added your own compatibility file, this isn't correct, it should be added to the existing
4.3-stable.expected(creation of these files is handled by maintainers, you don't need to do it yourself)
Well, as you can see that file doesn't exist based on the HEAD of this PR, and so I based those changes on what I saw in other PRs. I'll rebase and amend the existing file. Is this documented somewhere ?
Don't think it's documented but it's a general maintenance thing, good to rebase after a release to make sure things are up to date