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

Rework update_property for array #80706

Merged

Conversation

ajreckof
Copy link
Member

Fixes #75124

This is a huge rework of how array property editor is updated. Previously it would delete every editor for each element and recreate them. Now it first check whether the property editor will be the same.

This is not 100% tested but I couldn't find other bugs so decided to share to see if other people could find some bugs while I'm finishing testing otr if they had any suggestion on the code

Copy link
Member

@AThousandShips AThousandShips left a comment

Choose a reason for hiding this comment

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

You also need to rebase, your branch is far far behind, about a month and a half

editor/editor_properties_array_dict.cpp Outdated Show resolved Hide resolved
editor/editor_properties_array_dict.cpp Outdated Show resolved Hide resolved
@akien-mga akien-mga changed the title Rework update_property for array. Rework update_property for array. Aug 17, 2023
@ajreckof ajreckof force-pushed the rework-array-update-property branch from a52b26b to 132860f Compare August 17, 2023 16:13
@ajreckof
Copy link
Member Author

Just realised I forgot to tell. This fix only the array part of the issue linked and not the dictionary part I will deal with dictionnaries with my rework of the dictionary editor.

Copy link
Member

@Calinou Calinou left a comment

Choose a reason for hiding this comment

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

Tested locally (rebased on top of master 6758a7f), it works as expected (including on multi-page arrays).

Note that an issue that causes the entire array's contents to disappear (including in the saved data) still occurs if you happen to right-click the area on the left between the drag-and-drop icon and the property name then choose Paste Value.

@ajreckof
Copy link
Member Author

Note that an issue that causes the entire array's contents to disappear (including in the saved data) still occurs if you happen to right-click the area on the left between the drag-and-drop icon and the property name then choose Paste Value.

Was that happening before my PR or was it something introduced with this?
If it is not related I might know what is happening. I'll try to make a PR to fix it but if it is what I'm thinking then it will be a separated PR from this one as they are in fact not linked.

@Calinou
Copy link
Member

Calinou commented Aug 24, 2023

Was that happening before my PR or was it something introduced with this?

It's not introduced by the PR, I can reproduce it in master too: #80974

It's a separate issue and can be fixed in a separate PR.

@YuriSizov YuriSizov modified the milestones: 4.x, 4.2 Aug 25, 2023
@YuriSizov YuriSizov requested a review from a team August 25, 2023 13:15
@YuriSizov YuriSizov changed the title Rework update_property for array. Rework update_property for array Aug 25, 2023
@ajreckof ajreckof force-pushed the rework-array-update-property branch from 132860f to 0d0b401 Compare September 19, 2023 08:38
@KoBeWi
Copy link
Member

KoBeWi commented Sep 20, 2023

Dragging array elements may make the cursor disappear permanently.

Changing array type in code (in this case from Resource to Node) results in this error

 Attempted to set an object of type 'Node' into a TypedArray, which does not inherit from 'Resource'.
  C:\godot_source\core/variant/array.cpp:413 - Condition "!_p->typed.validate(value, "set")" is true.

You can't assign anything until reloading the scene.

editor/editor_properties_array_dict.h Outdated Show resolved Hide resolved
editor/editor_properties_array_dict.h Outdated Show resolved Hide resolved
editor/editor_properties_array_dict.h Outdated Show resolved Hide resolved
editor/editor_properties_array_dict.h Outdated Show resolved Hide resolved
@ajreckof
Copy link
Member Author

Changing array type in code (in this case from Resource to Node) results in this error

 Attempted to set an object of type 'Node' into a TypedArray, which does not inherit from 'Resource'.
  C:\godot_source\core/variant/array.cpp:413 - Condition "!_p->typed.validate(value, "set")" is true.

This is something already present in master. When testing my own PR I stumbled upon it too. The problem on the back end that it can still hold an array that is not of the right type. Normally returning to default is enough to fix it which is a bit less tenuous then re opening the scene.

@ajreckof
Copy link
Member Author

Dragging array elements may make the cursor disappear permanently.

I'm not sure about this one but it seems like it was present before my PR. But I'm not sure I'm looking at the same that causes the mouse to fully disappear. Mine is to focus ones of the button to drag element and then long press enter.

@ajreckof ajreckof force-pushed the rework-array-update-property branch from 0d0b401 to b42a48e Compare September 26, 2023 09:32
@KoBeWi
Copy link
Member

KoBeWi commented Sep 27, 2023

It's worse after the last update xd

@export var array: Array[int]

Change array size to 21, you'll get spammed with

ERROR: Condition "p_elem->_root != this" is true.
   at: SelfList<class Node>::List::remove (C:\godot_source\core/templates/self_list.h:80)

and the editor crashes when you unselect the node:


CrashHandlerException: Program crashed
Engine version: Godot Engine v4.2.dev.custom_build (545d1c0adbf340310e1531710eb31bd5267704d5)
Dumping the backtrace. Please include this when reporting the bug to the project developer.
[0] Viewport::_gui_remove_control (C:\godot_source\scene\main\viewport.cpp:2423)
[1] Control::_notification (C:\godot_source\scene\gui\control.cpp:3173)
[2] Control::_notificationv (C:\godot_source\scene\gui\control.h:48)
[3] TextureRect::_notificationv (C:\godot_source\scene\gui\texture_rect.h:37)
[4] Object::notification (C:\godot_source\core\object\object.cpp:844)
[5] Node::_propagate_exit_tree (C:\godot_source\scene\main\node.cpp:341)
[6] Node::_propagate_exit_tree (C:\godot_source\scene\main\node.cpp:332)
[7] Node::_propagate_exit_tree (C:\godot_source\scene\main\node.cpp:332)
[8] Node::_propagate_exit_tree (C:\godot_source\scene\main\node.cpp:332)
[9] Node::_propagate_exit_tree (C:\godot_source\scene\main\node.cpp:332)
[10] Node::_propagate_exit_tree (C:\godot_source\scene\main\node.cpp:332)
[11] Node::_propagate_exit_tree (C:\godot_source\scene\main\node.cpp:332)
[12] Node::_propagate_exit_tree (C:\godot_source\scene\main\node.cpp:332)
[13] Node::_propagate_exit_tree (C:\godot_source\scene\main\node.cpp:332)
[14] Node::_set_tree (C:\godot_source\scene\main\node.cpp:2920)
[15] Node::remove_child (C:\godot_source\scene\main\node.cpp:1453)
[16] Node::_notification (C:\godot_source\scene\main\node.cpp:210)
[17] Node::_notificationv (C:\godot_source\scene\main\node.h:49)
[18] CanvasItem::_notificationv (C:\godot_source\scene\main\canvas_item.h:45)
[19] Control::_notificationv (C:\godot_source\scene\gui\control.h:48)
[20] Container::_notificationv (C:\godot_source\scene\gui\container.h:37)
[21] BoxContainer::_notificationv (C:\godot_source\scene\gui\box_container.h:37)
[22] VBoxContainer::_notificationv (C:\godot_source\scene\gui\box_container.h:90)
[23] Object::notification (C:\godot_source\core\object\object.cpp:844)
[24] Object::_predelete (C:\godot_source\core\object\object.cpp:199)
[25] predelete_handler (C:\godot_source\core\object\object.cpp:2028)
[26] memdelete<Node> (C:\godot_source\core\os\memory.h:105)
[27] EditorInspector::_clear (C:\godot_source\editor\editor_inspector.cpp:3457)
[28] EditorInspector::edit (C:\godot_source\editor\editor_inspector.cpp:3482)
[29] EditorNode::push_item (C:\godot_source\editor\editor_node.cpp:2179)
[30] EditorNode::push_node_item (C:\godot_source\editor\editor_node.cpp:2172)
[31] SceneTreeDock::_push_item (C:\godot_source\editor\scene_tree_dock.cpp:1492)
[32] SceneTreeDock::_selection_changed (C:\godot_source\editor\scene_tree_dock.cpp:2294)
[33] call_with_variant_args_helper<SceneTreeDock> (C:\godot_source\core\variant\binder_common.h:308)
[34] call_with_variant_args<SceneTreeDock> (C:\godot_source\core\variant\binder_common.h:418)
[35] CallableCustomMethodPointer<SceneTreeDock>::call (C:\godot_source\core\object\callable_method_pointer.h:105)
[36] Callable::callp (C:\godot_source\core\variant\callable.cpp:51)
[37] Object::emit_signalp (C:\godot_source\core\object\object.cpp:1118)
[38] Object::emit_signal<> (C:\godot_source\core\object\object.h:920)
[39] EditorSelection::_emit_change (C:\godot_source\editor\editor_data.cpp:1299)
[40] call_with_variant_args_helper<EditorSelection> (C:\godot_source\core\variant\binder_common.h:308)
[41] call_with_variant_args_dv<EditorSelection> (C:\godot_source\core\variant\binder_common.h:451)
[42] MethodBindT<EditorSelection>::call (C:\godot_source\core\object\method_bind.h:337)
[43] Object::callp (C:\godot_source\core\object\object.cpp:767)
[44] Callable::callp (C:\godot_source\core\variant\callable.cpp:62)
[45] CallQueue::_call_function (C:\godot_source\core\object\message_queue.cpp:220)
[46] CallQueue::flush (C:\godot_source\core\object\message_queue.cpp:326)
[47] SceneTree::physics_process (C:\godot_source\scene\main\scene_tree.cpp:473)
[48] Main::iteration (C:\godot_source\main\main.cpp:3535)
[49] OS_Windows::run (C:\godot_source\platform\windows\os_windows.cpp:1474)
[50] widechar_main (C:\godot_source\platform\windows\godot_windows.cpp:182)
[51] _main (C:\godot_source\platform\windows\godot_windows.cpp:204)
[52] main (C:\godot_source\platform\windows\godot_windows.cpp:218)
[53] WinMain (C:\godot_source\platform\windows\godot_windows.cpp:232)
[54] __scrt_common_main_seh (D:\a\_work\1\s\src\vctools\crt\vcstartup\src\startup\exe_common.inl:288)
[55] <couldn't map PC to fn name>
-- END OF BACKTRACE --

@ajreckof ajreckof force-pushed the rework-array-update-property branch from b42a48e to a8e21ac Compare September 28, 2023 05:52
@ajreckof
Copy link
Member Author

I reversed the memfree changes and I can't reproduce. I'm not sure how this caused this bug so I don't know how to fix except keeping queue_free

@KoBeWi
Copy link
Member

KoBeWi commented Sep 28, 2023

I did more testing to the disappearing mouse bug (using Windows) and it seems to be introduced by this PR.
Without this PR:

YxigaWlelf.mp4

I can drag as much as I want and nothing happens.
With this PR:

U6y499hKJJ.mp4

It breaks after a few drags, consistently.

@akien-mga akien-mga modified the milestones: 4.2, 4.3 Oct 25, 2023
@ajreckof
Copy link
Member Author

I finally took the time to look at what was happenning with what kobewi reported. It was simply that in some condition I was deleting the slot that was being dragged around. I also added AThousandShips improvement.

@ajreckof ajreckof force-pushed the rework-array-update-property branch from e07cdb0 to 53ba4e1 Compare November 23, 2023 09:18
Apply suggestions from code review

Co-Authored-By: Tomek <kobewi4e@gmail.com>
@ajreckof ajreckof force-pushed the rework-array-update-property branch from 53ba4e1 to b4d96bc Compare December 21, 2023 09:00
@akien-mga akien-mga merged commit 0d88840 into godotengine:master Jan 2, 2024
15 checks passed
@akien-mga
Copy link
Member

Thanks!

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.

"Paste Value" does not refresh the displayed values of array/dict elements in inspector
6 participants