Skip to content
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

Inspector: Fix clearing array/dictionary element with <Object#null> #84237

Conversation

dalexeev
Copy link
Member

EditorPropertyArray and EditorPropertyDictionary use EditorPropertyResource and EditorResourcePicker, but array elements and dictionary keys/values can be Variant() (null), unlike Ref<Resource>.

case OBJ_MENU_CLEAR: {
edited_resource = Ref<Resource>();
emit_signal(SNAME("resource_changed"), edited_resource);
_update_resource();
} break;

@dalexeev dalexeev added this to the 4.3 milestone Oct 31, 2023
@dalexeev dalexeev requested a review from a team October 31, 2023 08:43
@KoBeWi
Copy link
Member

KoBeWi commented Nov 5, 2023

I don't really understand the issue. Assigning Variant() in EditorResourcePicker does not fix it 🤔 Why is it different initially?

@dalexeev
Copy link
Member Author

dalexeev commented Nov 5, 2023

Because a Variant can store both Variant() and Ref<Resource>(), but a Ref<Resource> cannot store Variant(), it will be implicitly cast to Ref<Resource>(). This is a flaw in the type system, we have three nulls: null (Variant()), Object#null and FreedObject. The first one is Variant::NIL, the last two are Variant::OBJECT.

@KoBeWi
Copy link
Member

KoBeWi commented Nov 5, 2023

But the array works correctly when first resized. So it has a different value than after using Clear it seems, and there is no way to restore it.

@dalexeev
Copy link
Member Author

dalexeev commented Nov 5, 2023

To resize, the editor probably calls the array method, which works correctly. Copying Object#null value from the editor only occurs on write actions (not on read actions), when you interact with the element's editor.

Copy link
Member

@KoBeWi KoBeWi left a comment

Choose a reason for hiding this comment

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

Maybe the comment should be more descriptive ("GH-#" is very specific, but it requires you to lookup the issue).

@YuriSizov YuriSizov added the cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release label Nov 6, 2023
@YuriSizov
Copy link
Contributor

@dalexeev Would you mind updating the comment to say a bit more about the reason for the code's existence? You can still add See GH-... at the end.

@dalexeev dalexeev force-pushed the inspector-fix-array-dict-elem-clearing branch from c751ae0 to ed86c33 Compare December 5, 2023 05:47
@dalexeev
Copy link
Member Author

dalexeev commented Dec 5, 2023

@YuriSizov Added comments and rebased the branch.

@akien-mga akien-mga merged commit 4bc4817 into godotengine:master Dec 5, 2023
15 checks passed
@akien-mga
Copy link
Member

Thanks!

@dalexeev dalexeev deleted the inspector-fix-array-dict-elem-clearing branch December 5, 2023 12:07
@YuriSizov
Copy link
Contributor

Cherry-picked for 4.2.1.

@YuriSizov YuriSizov removed the cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release label Dec 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants