From 269fdee860fd85432662f52cd019196ffebb59e5 Mon Sep 17 00:00:00 2001 From: Jamie Pate Date: Fri, 24 Jan 2025 11:10:48 -0800 Subject: [PATCH 1/5] Add CI workflow --- .github/workflows/ci.yaml | 50 +++++++++++++++++++++++++++++++++++++++ .gitmodules | 3 --- Example.tscn | 8 +++---- RootModel.gd | 1 + contrib/gdformat-plugin | 1 - contrib/gut | 2 +- tests/.gutconfig.json | 32 +++++++++++++++++++++++++ tests/test_bind_repeat.gd | 5 ++-- 8 files changed, 90 insertions(+), 12 deletions(-) create mode 100644 .github/workflows/ci.yaml delete mode 160000 contrib/gdformat-plugin create mode 100644 tests/.gutconfig.json diff --git a/.github/workflows/ci.yaml b/.github/workflows/ci.yaml new file mode 100644 index 0000000..a27ed5e --- /dev/null +++ b/.github/workflows/ci.yaml @@ -0,0 +1,50 @@ +on: [push, pull_request] + +name: Continuous integration + +jobs: + gdlint: + name: gdlint scripts + runs-on: ubuntu-latest + + steps: + # Check out the repository + - name: Checkout + uses: actions/checkout@v4 + + # Ensure python is installed + - name: Set up Python + uses: actions/setup-python@v4 + with: + python-version: '3.10' + + # Install gdtoolkit + - name: Install Dependencies + run: | + python -m pip install --upgrade pip + python -m pip install 'gdtoolkit==4.3.2' + + # Lint the godot-xr-tools addon + - name: Lint and Format Checks + run: | + ./check.sh + test: + name: Test + runs-on: ubuntu-latest + container: + image: barichello/godot-ci:latest + steps: + - name: Checkout + uses: actions/checkout@v2 + with: + submodules: true + - run: './update_addons.sh' + - name: Godot Import + run: godot --headless --import + - name: Gut Tests + uses: jamie-pate/run-gut-tests-action@v2.0.3 + with: + useContainer: false + gutConfigPath: tests/.gutconfig.json + + diff --git a/.gitmodules b/.gitmodules index 4b4aa4c..4bb0907 100644 --- a/.gitmodules +++ b/.gitmodules @@ -1,6 +1,3 @@ -[submodule "contrib/gdformat-plugin"] - path = contrib/gdformat-plugin - url = https://gitlab.com/DonatienBdR/gdformat-plugin.git [submodule "contrib/gut"] path = contrib/gut url = https://github.com/bitwes/Gut.git diff --git a/Example.tscn b/Example.tscn index 278da8e..1b42879 100644 --- a/Example.tscn +++ b/Example.tscn @@ -1,10 +1,10 @@ [gd_scene load_steps=7 format=3 uid="uid://ck6v3p607f1pn"] -[ext_resource type="Script" path="res://addons/DataBindControls/Binds.gd" id="1"] -[ext_resource type="Script" path="res://Example.gd" id="2"] -[ext_resource type="Script" path="res://addons/DataBindControls/BindRepeat.gd" id="3"] +[ext_resource type="Script" uid="uid://glgueq8jhg8w" path="res://addons/DataBindControls/Binds.gd" id="1"] +[ext_resource type="Script" uid="uid://c8ukhtstmopxx" path="res://Example.gd" id="2"] +[ext_resource type="Script" uid="uid://dny4y15clfmyv" path="res://addons/DataBindControls/BindRepeat.gd" id="3"] [ext_resource type="PackedScene" uid="uid://ddkq3rwuwxo1d" path="res://RepeatedExample.tscn" id="4"] -[ext_resource type="Script" path="res://addons/DataBindControls/BindItems.gd" id="5"] +[ext_resource type="Script" uid="uid://dmupqmii2hhkr" path="res://addons/DataBindControls/BindItems.gd" id="5"] [sub_resource type="StyleBoxFlat" id="1"] bg_color = Color(0.196078, 0.196078, 0.196078, 1) diff --git a/RootModel.gd b/RootModel.gd index ed5c016..49d4cef 100644 --- a/RootModel.gd +++ b/RootModel.gd @@ -8,6 +8,7 @@ var array: Array var time: String var icon: Texture = null + func array_callable(): return array diff --git a/contrib/gdformat-plugin b/contrib/gdformat-plugin deleted file mode 160000 index 81143c9..0000000 --- a/contrib/gdformat-plugin +++ /dev/null @@ -1 +0,0 @@ -Subproject commit 81143c9a47709e082716bd0a2e41b3c14d5fb0c5 diff --git a/contrib/gut b/contrib/gut index 5782983..52e6d7a 160000 --- a/contrib/gut +++ b/contrib/gut @@ -1 +1 @@ -Subproject commit 57829837811946439648b635be231549c2dc6f06 +Subproject commit 52e6d7a1fae2ce92ba78de3126df2b868d56bfbd diff --git a/tests/.gutconfig.json b/tests/.gutconfig.json new file mode 100644 index 0000000..2e4fb48 --- /dev/null +++ b/tests/.gutconfig.json @@ -0,0 +1,32 @@ +{ + "background_color": "ff262626", + "compact_mode": false, + "configured_dirs": [ + "res://tests" + ], + "dirs": [ + "res://tests" + ], + "disable_colors": false, + "double_strategy": 1, + "errors_do_not_cause_failure": false, + "font_color": "ffcccccc", + "font_name": "CourierPrime", + "font_size": 11, + "gut_on_top": true, + "hide_orphans": false, + "ignore_pause": false, + "include_subdirs": false, + "junit_xml_file": "", + "junit_xml_timestamp": false, + "log_level": 3, + "opacity": 100, + "paint_after": 0.1, + "post_run_script": "", + "pre_run_script": "", + "prefix": "test_", + "should_exit": true, + "should_exit_on_success": true, + "should_maximize": true, + "suffix": ".gd" +} \ No newline at end of file diff --git a/tests/test_bind_repeat.gd b/tests/test_bind_repeat.gd index 26e27ff..5c44b31 100644 --- a/tests/test_bind_repeat.gd +++ b/tests/test_bind_repeat.gd @@ -8,13 +8,12 @@ func test_bind_repeat(): add_child_autoqfree(repeated_control_host) # not sure why this takes 2 frames to call the deferred method - await RenderingServer.frame_post_draw - await RenderingServer.frame_post_draw + await wait_frames(1) assert_eq(repeated_control_host.get_child_count(), 0) repeated_control_host.model.append("text") DataBindings.detect_changes() - await RenderingServer.frame_post_draw + await wait_frames(1) assert_eq(repeated_control_host.get_child_count(), 1) var label = repeated_control_host.get_child(0) assert_eq(label.text if label else null, "text") From 78ae2c5fdf8e8e24c793e880c2e89602325307af Mon Sep 17 00:00:00 2001 From: Jamie Pate Date: Sun, 26 Jan 2025 12:35:16 -0800 Subject: [PATCH 2/5] Rename Files to snake_case and change and some variable names in the example --- Example.gd | 102 ------------------ RootModel.gd | 17 --- .../{BindEditor.gd => bind_editor.gd} | 0 .../{BindItems.gd => bind_items.gd} | 0 .../{BindRepeat.gd => bind_repeat.gd} | 4 +- .../{BindTarget.gd => bind_target.gd} | 0 .../DataBindControls/{Binds.gd => binds.gd} | 4 +- ...dingsGlobal.gd => data_bindings_global.gd} | 2 +- ...InspectorPlugin.gd => inspector_plugin.gd} | 4 +- addons/DataBindControls/plugin.gd | 4 +- addons/DataBindControls/{Util.gd => util.gd} | 0 example.gd | 98 +++++++++++++++++ Example.tscn => example.tscn | 38 +++---- ItemModel.gd => example_item.gd | 2 +- BaseModel.gd => example_model_base.gd | 0 Main.tscn => main.tscn | 2 +- project.godot | 4 +- repeated_example.gd | 11 ++ RepeatedExample.tscn => repeated_example.tscn | 33 ++++-- test.sh | 45 ++++++++ tests/{TestUtil.gd => gut_ex.gd} | 0 ...ontrolHost.gd => repeated_control_host.gd} | 0 ...olHost.tscn => repeated_control_host.tscn} | 4 +- tests/{Subscriber.gd => subscriber.gd} | 0 tests/test_bind_repeat.gd | 2 +- 25 files changed, 212 insertions(+), 164 deletions(-) delete mode 100644 Example.gd delete mode 100644 RootModel.gd rename addons/DataBindControls/{BindEditor.gd => bind_editor.gd} (100%) rename addons/DataBindControls/{BindItems.gd => bind_items.gd} (100%) rename addons/DataBindControls/{BindRepeat.gd => bind_repeat.gd} (97%) rename addons/DataBindControls/{BindTarget.gd => bind_target.gd} (100%) rename addons/DataBindControls/{Binds.gd => binds.gd} (98%) rename addons/DataBindControls/{DataBindingsGlobal.gd => data_bindings_global.gd} (99%) rename addons/DataBindControls/{InspectorPlugin.gd => inspector_plugin.gd} (83%) rename addons/DataBindControls/{Util.gd => util.gd} (100%) create mode 100644 example.gd rename Example.tscn => example.tscn (84%) rename ItemModel.gd => example_item.gd (83%) rename BaseModel.gd => example_model_base.gd (100%) rename Main.tscn => main.tscn (89%) create mode 100644 repeated_example.gd rename RepeatedExample.tscn => repeated_example.tscn (80%) create mode 100755 test.sh rename tests/{TestUtil.gd => gut_ex.gd} (100%) rename tests/{RepeatedControlHost.gd => repeated_control_host.gd} (100%) rename tests/{RepeatedControlHost.tscn => repeated_control_host.tscn} (84%) rename tests/{Subscriber.gd => subscriber.gd} (100%) diff --git a/Example.gd b/Example.gd deleted file mode 100644 index eaf062a..0000000 --- a/Example.gd +++ /dev/null @@ -1,102 +0,0 @@ -extends Panel - -signal remove(model) - -# Model classes here are named with the Model suffix but you could use any naming scheme -const BaseModel = preload("./BaseModel.gd") -const RootModel = preload("./RootModel.gd") -const ItemModel = preload("./ItemModel.gd") - -var model = RootModel.new({text = "root1", pressed = false, array = [], time = "0", path = ""}): - set = _set_model -var model_yaml := {value = ""} - -var _next_id = 0 - - -# Sample callable that returns only item models with an even value -func sample_callable(): - var sub_array = model.array.filter(func(n): return n.value % 2 == 0) - return sub_array - - -func _ready(): - var a := model.array as Array - model.path = str(get_path()) - a.append(ItemModel.new({text = "repeat0", pressed = false, icon = _get_icon(0), value = 1})) - a.append(ItemModel.new({text = "repeat1", pressed = true, icon = _get_icon(1), value = 0})) - a.append(ItemModel.new({text = "repeat2", pressed = true, icon = _get_icon(1), value = 0})) - _next_id = 2 - print("model.array[0] = %s : %s" % [get_path(), a[0]]) - - -func _set_model(value): - model = value - assert(model, "Model can't be null, invalid type assignment?") - - -func _get_icon(i: int): - var icons = [ - preload("res://addons/DataBindControls/icons/link.svg"), - preload("res://addons/DataBindControls/icons/links.svg"), - preload("res://addons/DataBindControls/icons/list.svg") - ] - return icons[i % len(icons)] - - -func _yaml(model, indent := "", indent_first := false) -> String: - var result := PackedStringArray() - if model is BaseModel: - var first = true - for k in model.keys(): - result.append( - ( - "%s%s: %s" - % [ - indent if !first || indent_first else "", - k, - _yaml(model[k], indent + " ", false) - ] - ) - ) - first = false - elif model is Array: - if !indent_first: - result.append("") - for item in model: - result.append("%s- %s" % [indent, _yaml(item, indent + " ", false)]) - else: - return "%s%s" % [indent if indent_first else "", model] - return "\n".join(result) - - -func _on_Button_pressed(): - emit_signal("remove", model) - - -func _on_RepeatPanel_remove(value): - model.array.erase(value) - - -func _on_AddButton_pressed(): - model.array.append( - ItemModel.new({text = "repeat%s" % [_next_id], pressed = false, icon = _get_icon(_next_id)}) - ) - _next_id += 1 - - -func _on_Timer_timeout(): - ## this is an example of when you need to call change detection. - ## Ideally we would be able to hook into some of these at the engine level - ## to automatically call detect_changes whenever a timeout or other async - ## action happens similar to NgZone - model.time = str(Time.get_ticks_msec()) - DataBindings.detect_changes() - - -func _process(_delta: float): - var new_value = _yaml(model) - # we don't want to flood change detection - if model_yaml.value != new_value: - model_yaml.value = new_value - DataBindings.detect_changes() diff --git a/RootModel.gd b/RootModel.gd deleted file mode 100644 index 49d4cef..0000000 --- a/RootModel.gd +++ /dev/null @@ -1,17 +0,0 @@ -## Model for the root of the example -extends "./BaseModel.gd" - -var text: String -var path: String -var pressed: bool -var array: Array -var time: String -var icon: Texture = null - - -func array_callable(): - return array - - -func _init(initial_value = {}): - super(initial_value) diff --git a/addons/DataBindControls/BindEditor.gd b/addons/DataBindControls/bind_editor.gd similarity index 100% rename from addons/DataBindControls/BindEditor.gd rename to addons/DataBindControls/bind_editor.gd diff --git a/addons/DataBindControls/BindItems.gd b/addons/DataBindControls/bind_items.gd similarity index 100% rename from addons/DataBindControls/BindItems.gd rename to addons/DataBindControls/bind_items.gd diff --git a/addons/DataBindControls/BindRepeat.gd b/addons/DataBindControls/bind_repeat.gd similarity index 97% rename from addons/DataBindControls/BindRepeat.gd rename to addons/DataBindControls/bind_repeat.gd index 653f4d3..6206e1e 100644 --- a/addons/DataBindControls/BindRepeat.gd +++ b/addons/DataBindControls/bind_repeat.gd @@ -6,8 +6,8 @@ extends Node ## Bind items in an array to be repeated using the parent template. ## Each array item will instance the template once in the grandparent control. -const Util := preload("./Util.gd") -const BindTarget := preload("./BindTarget.gd") +const Util := preload("./util.gd") +const BindTarget := preload("./bind_target.gd") @export var array_bind: String: set = _set_array_bind diff --git a/addons/DataBindControls/BindTarget.gd b/addons/DataBindControls/bind_target.gd similarity index 100% rename from addons/DataBindControls/BindTarget.gd rename to addons/DataBindControls/bind_target.gd diff --git a/addons/DataBindControls/Binds.gd b/addons/DataBindControls/binds.gd similarity index 98% rename from addons/DataBindControls/Binds.gd rename to addons/DataBindControls/binds.gd index 4946d83..07549ef 100644 --- a/addons/DataBindControls/Binds.gd +++ b/addons/DataBindControls/binds.gd @@ -5,8 +5,8 @@ extends Node ## Bind properties against a single control -const BindTarget := preload("./BindTarget.gd") -const Util := preload("./Util.gd") +const BindTarget := preload("./bind_target.gd") +const Util := preload("./util.gd") const NUM_TYPES: Array[Variant.Type] = [TYPE_INT, TYPE_FLOAT] const PASSTHROUGH_PROPS := [ diff --git a/addons/DataBindControls/DataBindingsGlobal.gd b/addons/DataBindControls/data_bindings_global.gd similarity index 99% rename from addons/DataBindControls/DataBindingsGlobal.gd rename to addons/DataBindControls/data_bindings_global.gd index 8447abf..d5677c4 100644 --- a/addons/DataBindControls/DataBindingsGlobal.gd +++ b/addons/DataBindControls/data_bindings_global.gd @@ -1,6 +1,6 @@ extends Node -const Util = preload("./Util.gd") +const Util = preload("./util.gd") const MAX_CHANGES = 100 const MAX_CHANGES_LOGGED = 20 diff --git a/addons/DataBindControls/InspectorPlugin.gd b/addons/DataBindControls/inspector_plugin.gd similarity index 83% rename from addons/DataBindControls/InspectorPlugin.gd rename to addons/DataBindControls/inspector_plugin.gd index eb05ff6..d3432b7 100644 --- a/addons/DataBindControls/InspectorPlugin.gd +++ b/addons/DataBindControls/inspector_plugin.gd @@ -1,7 +1,7 @@ extends EditorInspectorPlugin -const BindEditor := preload("./BindEditor.gd") -const Binds := preload("./Binds.gd") +const BindEditor := preload("./bind_editor.gd") +const Binds := preload("./binds.gd") func can_handle(object: Object): diff --git a/addons/DataBindControls/plugin.gd b/addons/DataBindControls/plugin.gd index a96a7c9..bfbae1d 100644 --- a/addons/DataBindControls/plugin.gd +++ b/addons/DataBindControls/plugin.gd @@ -1,7 +1,7 @@ @tool extends EditorPlugin -const InspectorPlugin := preload("./InspectorPlugin.gd") +const InspectorPlugin := preload("./inspector_plugin.gd") var inspector: EditorInspectorPlugin = null @@ -12,7 +12,7 @@ func _enter_tree(): inspector = InspectorPlugin.new() add_inspector_plugin(inspector) var path = get_script().resource_path.get_base_dir() - add_autoload_singleton("DataBindings", "%s/DataBindingsGlobal.gd" % [path]) + add_autoload_singleton("DataBindings", "%s/data_bindings_global.gd" % [path]) func _exit_tree(): diff --git a/addons/DataBindControls/Util.gd b/addons/DataBindControls/util.gd similarity index 100% rename from addons/DataBindControls/Util.gd rename to addons/DataBindControls/util.gd diff --git a/example.gd b/example.gd new file mode 100644 index 0000000..09fd9fb --- /dev/null +++ b/example.gd @@ -0,0 +1,98 @@ +extends Panel + +const ExampleItem = preload("./example_item.gd") + +var text = "root1" +var array: Array[ExampleItem] +var time = "0" +var path = "" +var model_yaml := "" +var pressed := false + +var _next_id = 0 + + +# Sample callable that returns only item models with an even value +func filtered_array(): + var sub_array: Array[ExampleItem] = array.filter(func(n): return n.value % 2 == 0) + return sub_array + + +func _ready(): + path = str(get_path()) + array.append( + ExampleItem.new({text = "repeat0", pressed = false, icon = _get_icon(0), value = 1}) + ) + array.append( + ExampleItem.new({text = "repeat1", pressed = true, icon = _get_icon(1), value = 0}) + ) + array.append( + ExampleItem.new({text = "repeat2", pressed = true, icon = _get_icon(1), value = 0}) + ) + _next_id = 2 + print("model.array[0] = %s : %s" % [get_path(), array[0]]) + + +func _get_icon(i: int): + var icons = [ + preload("res://addons/DataBindControls/icons/link.svg"), + preload("res://addons/DataBindControls/icons/links.svg"), + preload("res://addons/DataBindControls/icons/list.svg") + ] + return icons[i % len(icons)] + + +func _yaml(obj, indent := "", indent_first := false) -> String: + var result := PackedStringArray() + if obj is ExampleItem or obj is Dictionary: + var first = true + for k in obj.keys(): + result.append( + ( + "%s%s: %s" + % [ + indent if !first || indent_first else "", + k, + _yaml(obj[k], indent + " ", false) + ] + ) + ) + first = false + elif obj is Array: + if !indent_first: + result.append("") + for item in obj: + result.append("%s- %s" % [indent, _yaml(item, indent + " ", false)]) + else: + return "%s%s" % [indent if indent_first else "", obj] + return "\n".join(result) + + +func _on_RepeatPanel_remove(value): + array.erase(value) + + +func _on_AddButton_pressed(): + array.append( + ExampleItem.new( + {text = "repeat%s" % [_next_id], pressed = false, icon = _get_icon(_next_id)} + ) + ) + _next_id += 1 + + +func _on_Timer_timeout(): + ## this is an example of when you need to call change detection. + ## Ideally we would be able to hook into some of these at the engine level + ## to automatically call detect_changes whenever a timeout or other async + ## action happens similar to NgZone + time = str(Time.get_ticks_msec()) + DataBindings.detect_changes() + + +func _process(_delta: float): + var new_value = _yaml({text = text, time = time, path = path, array = array}) + # we don't want to flood change detection + if model_yaml != new_value: + model_yaml = new_value + DataBindings.detect_changes() diff --git a/Example.tscn b/example.tscn similarity index 84% rename from Example.tscn rename to example.tscn index 1b42879..7529741 100644 --- a/Example.tscn +++ b/example.tscn @@ -1,10 +1,10 @@ [gd_scene load_steps=7 format=3 uid="uid://ck6v3p607f1pn"] -[ext_resource type="Script" uid="uid://glgueq8jhg8w" path="res://addons/DataBindControls/Binds.gd" id="1"] -[ext_resource type="Script" uid="uid://c8ukhtstmopxx" path="res://Example.gd" id="2"] -[ext_resource type="Script" uid="uid://dny4y15clfmyv" path="res://addons/DataBindControls/BindRepeat.gd" id="3"] -[ext_resource type="PackedScene" uid="uid://ddkq3rwuwxo1d" path="res://RepeatedExample.tscn" id="4"] -[ext_resource type="Script" uid="uid://dmupqmii2hhkr" path="res://addons/DataBindControls/BindItems.gd" id="5"] +[ext_resource type="Script" path="res://addons/DataBindControls/binds.gd" id="1"] +[ext_resource type="Script" path="res://example.gd" id="1_so716"] +[ext_resource type="Script" path="res://addons/DataBindControls/bind_repeat.gd" id="3"] +[ext_resource type="PackedScene" uid="uid://ddkq3rwuwxo1d" path="res://repeated_example.tscn" id="4"] +[ext_resource type="Script" path="res://addons/DataBindControls/bind_items.gd" id="5"] [sub_resource type="StyleBoxFlat" id="1"] bg_color = Color(0.196078, 0.196078, 0.196078, 1) @@ -17,7 +17,7 @@ grow_horizontal = 2 grow_vertical = 2 size_flags_horizontal = 3 size_flags_vertical = 3 -script = ExtResource("2") +script = ExtResource("1_so716") [node name="VBoxContainer" type="VBoxContainer" parent="."] layout_mode = 0 @@ -46,7 +46,7 @@ process_thread_group = "" physics_interpolation_mode = "" auto_translate_mode = "" script = ExtResource("1") -text = "model.text" +text = "text" [node name="CheckBox" type="CheckBox" parent="VBoxContainer/HBoxContainer/VBoxContainer/VBoxContainer"] layout_mode = 2 @@ -59,7 +59,7 @@ process_thread_group = "" physics_interpolation_mode = "" auto_translate_mode = "" script = ExtResource("1") -button_pressed = "model.pressed" +button_pressed = "pressed" [node name="CheckButton" type="CheckButton" parent="VBoxContainer/HBoxContainer/VBoxContainer/VBoxContainer"] layout_mode = 2 @@ -72,7 +72,7 @@ process_thread_group = "" physics_interpolation_mode = "" auto_translate_mode = "" script = ExtResource("1") -button_pressed = "model.pressed" +button_pressed = "pressed" [node name="TextEdit" type="TextEdit" parent="VBoxContainer/HBoxContainer/VBoxContainer/VBoxContainer"] custom_minimum_size = Vector2(0, 38) @@ -86,7 +86,7 @@ process_thread_group = "" physics_interpolation_mode = "" auto_translate_mode = "" script = ExtResource("1") -text = "model.text" +text = "text" [node name="LineEdit" type="LineEdit" parent="VBoxContainer/HBoxContainer/VBoxContainer/VBoxContainer"] layout_mode = 2 @@ -99,7 +99,7 @@ process_thread_group = "" physics_interpolation_mode = "" auto_translate_mode = "" script = ExtResource("1") -text = "model.text" +text = "text" [node name="HBoxContainer" type="HBoxContainer" parent="VBoxContainer/HBoxContainer/VBoxContainer"] layout_mode = 2 @@ -122,7 +122,7 @@ process_thread_group = "" physics_interpolation_mode = "" auto_translate_mode = "" script = ExtResource("1") -text = "model_yaml.value" +text = "model_yaml" [node name="VBoxContainer2" type="VBoxContainer" parent="VBoxContainer/HBoxContainer"] layout_mode = 2 @@ -144,7 +144,7 @@ process_thread_group = "" physics_interpolation_mode = "" auto_translate_mode = "" script = ExtResource("5") -array_bind = "model.array" +array_bind = "array" item_text = "text" item_icon = "icon" item_selected = "pressed" @@ -158,15 +158,15 @@ layout_mode = 2 size_flags_horizontal = 3 size_flags_vertical = 3 -[node name="RepeatPanel" parent="VBoxContainer/HBoxContainer/VBoxContainer2/ScrollContainer/VBCRepeat" instance=ExtResource("4")] +[node name="RepeatedControl" parent="VBoxContainer/HBoxContainer/VBoxContainer2/ScrollContainer/VBCRepeat" instance=ExtResource("4")] custom_minimum_size = Vector2(0, 145) layout_mode = 2 theme_override_styles/panel = SubResource("1") -[node name="BindRepeat" type="Node" parent="VBoxContainer/HBoxContainer/VBoxContainer2/ScrollContainer/VBCRepeat/RepeatPanel"] +[node name="BindRepeat" type="Node" parent="VBoxContainer/HBoxContainer/VBoxContainer2/ScrollContainer/VBCRepeat/RepeatedControl"] script = ExtResource("3") -array_bind = "model.array_callable()" -target_property = "model" +array_bind = "filtered_array()" +target_property = "item" [node name="HBoxContainer2" type="HBoxContainer" parent="VBoxContainer"] layout_mode = 2 @@ -182,12 +182,12 @@ process_thread_group = "" physics_interpolation_mode = "" auto_translate_mode = "" script = ExtResource("1") -text = "model.time" +text = "time" [node name="Timer" type="Timer" parent="VBoxContainer/HBoxContainer2"] wait_time = 10.0 autostart = true [connection signal="pressed" from="VBoxContainer/HBoxContainer/VBoxContainer/HBoxContainer/AddButton" to="." method="_on_AddButton_pressed"] -[connection signal="remove" from="VBoxContainer/HBoxContainer/VBoxContainer2/ScrollContainer/VBCRepeat/RepeatPanel" to="." method="_on_RepeatPanel_remove"] +[connection signal="remove" from="VBoxContainer/HBoxContainer/VBoxContainer2/ScrollContainer/VBCRepeat/RepeatedControl" to="." method="_on_RepeatPanel_remove"] [connection signal="timeout" from="VBoxContainer/HBoxContainer2/Timer" to="." method="_on_Timer_timeout"] diff --git a/ItemModel.gd b/example_item.gd similarity index 83% rename from ItemModel.gd rename to example_item.gd index 1fbadd4..edca9ec 100644 --- a/ItemModel.gd +++ b/example_item.gd @@ -1,5 +1,5 @@ ## Model for an item in the example list -extends "./BaseModel.gd" +extends "./example_model_base.gd" var text: String var pressed: bool diff --git a/BaseModel.gd b/example_model_base.gd similarity index 100% rename from BaseModel.gd rename to example_model_base.gd diff --git a/Main.tscn b/main.tscn similarity index 89% rename from Main.tscn rename to main.tscn index a3e4e99..52d6207 100644 --- a/Main.tscn +++ b/main.tscn @@ -1,6 +1,6 @@ [gd_scene load_steps=2 format=3 uid="uid://byx3qm2ipsmxj"] -[ext_resource type="PackedScene" uid="uid://ck6v3p607f1pn" path="res://Example.tscn" id="1_fpbrb"] +[ext_resource type="PackedScene" uid="uid://ck6v3p607f1pn" path="res://example.tscn" id="1_fpbrb"] [node name="Node2D" type="Node2D"] diff --git a/project.godot b/project.godot index 7d7186b..a8f688c 100644 --- a/project.godot +++ b/project.godot @@ -11,13 +11,13 @@ config_version=5 [application] config/name="DataBinds" -run/main_scene="res://Main.tscn" +run/main_scene="res://main.tscn" config/features=PackedStringArray("4.3") config/icon="res://icon.png" [autoload] -DataBindings="*res://addons/DataBindControls/DataBindingsGlobal.gd" +DataBindings="*res://addons/DataBindControls/data_bindings_global.gd" [editor_plugins] diff --git a/repeated_example.gd b/repeated_example.gd new file mode 100644 index 0000000..ea406b8 --- /dev/null +++ b/repeated_example.gd @@ -0,0 +1,11 @@ +extends Panel + +signal remove(item: ExampleItem) + +const ExampleItem = preload("res://example_item.gd") + +var item: ExampleItem + + +func _on_Button_pressed(): + emit_signal("remove", item) diff --git a/RepeatedExample.tscn b/repeated_example.tscn similarity index 80% rename from RepeatedExample.tscn rename to repeated_example.tscn index 7dd7def..9fd6950 100644 --- a/RepeatedExample.tscn +++ b/repeated_example.tscn @@ -1,9 +1,10 @@ [gd_scene load_steps=3 format=3 uid="uid://ddkq3rwuwxo1d"] -[ext_resource type="Script" path="res://Example.gd" id="1"] -[ext_resource type="Script" path="res://addons/DataBindControls/Binds.gd" id="2"] +[ext_resource type="Script" path="res://repeated_example.gd" id="1_qsp2h"] +[ext_resource type="Script" path="res://addons/DataBindControls/binds.gd" id="2"] -[node name="RepeatPanel" type="Panel"] +[node name="RepeatedControl" type="Panel"] +custom_minimum_size = Vector2(0, 200) anchors_preset = 15 anchor_right = 1.0 anchor_bottom = 1.0 @@ -12,7 +13,7 @@ grow_horizontal = 2 grow_vertical = 2 size_flags_horizontal = 3 size_flags_vertical = 3 -script = ExtResource("1") +script = ExtResource("1_qsp2h") [node name="VBoxContainer" type="VBoxContainer" parent="."] layout_mode = 1 @@ -35,8 +36,10 @@ _import_path = "" unique_name_in_owner = "" process_physics_priority = "" process_thread_group = "" +physics_interpolation_mode = "" +auto_translate_mode = "" script = ExtResource("2") -texture = "model.icon" +texture = "item.icon" [node name="Button" type="Button" parent="VBoxContainer/HBoxContainer"] layout_mode = 2 @@ -50,8 +53,10 @@ _import_path = "" unique_name_in_owner = "" process_physics_priority = "" process_thread_group = "" +physics_interpolation_mode = "" +auto_translate_mode = "" script = ExtResource("2") -text = "model.text" +text = "item.text" [node name="CheckBox" type="CheckBox" parent="VBoxContainer"] layout_mode = 2 @@ -61,8 +66,10 @@ _import_path = "" unique_name_in_owner = "" process_physics_priority = "" process_thread_group = "" +physics_interpolation_mode = "" +auto_translate_mode = "" script = ExtResource("2") -button_pressed = "model.pressed" +button_pressed = "item.pressed" [node name="CheckButton" type="CheckButton" parent="VBoxContainer"] layout_mode = 2 @@ -72,8 +79,10 @@ _import_path = "" unique_name_in_owner = "" process_physics_priority = "" process_thread_group = "" +physics_interpolation_mode = "" +auto_translate_mode = "" script = ExtResource("2") -button_pressed = "model.pressed" +button_pressed = "item.pressed" [node name="TextEdit3" type="TextEdit" parent="VBoxContainer"] custom_minimum_size = Vector2(0, 38) @@ -84,8 +93,10 @@ _import_path = "" unique_name_in_owner = "" process_physics_priority = "" process_thread_group = "" +physics_interpolation_mode = "" +auto_translate_mode = "" script = ExtResource("2") -text = "model.text" +text = "item.text" [node name="LineEdit" type="LineEdit" parent="VBoxContainer"] layout_mode = 2 @@ -95,8 +106,10 @@ _import_path = "" unique_name_in_owner = "" process_physics_priority = "" process_thread_group = "" +physics_interpolation_mode = "" +auto_translate_mode = "" script = ExtResource("2") -text = "model.text" +text = "item.text" [node name="TextEdit" type="TextEdit" parent="VBoxContainer"] layout_mode = 2 diff --git a/test.sh b/test.sh new file mode 100755 index 0000000..fc1acbb --- /dev/null +++ b/test.sh @@ -0,0 +1,45 @@ +#!/usr/bin/env bash +version=4.3-stable + +run_godot() { + # this got a bit complicated because gut doesn't detect SCRIPT ERRORs :( + # https://github.com/bitwes/Gut/issues/210 + local errfile="$(mktemp)" + "$1" --headless -v -s addons/gut/gut_cmdln.gd -gconfig tests/.gutconfig.json 2> "$errfile" & + pid=$! + tail -f "$errfile" & + tailpid=$! + wait $pid + local result=$? + kill $tailpid + local errors=$(grep "SCRIPT ERROR:" "$errfile") + rm "$errfile" + if [ -n "$errors" ]; then + echo "" >&2 + echo "ERRORS DETECTED:" >&2 + echo "$errors" >&2 + return 1 + fi + return $result +} +suffixes=( + _linux.x86_64 + _win64.exe + .app/Contents/MacOS/Godot + "" +) +set -eu +for s in "${suffixes[@]}"; do + if [ -n "$s" ]; then + bin=Godot_v${version}${s} + else + bin=godot + fi + if [ -x "$(which $bin)" ]; then + run_godot "$bin" + exit $? + fi +done + +echo "Godot not found" >&2 +exit 2 diff --git a/tests/TestUtil.gd b/tests/gut_ex.gd similarity index 100% rename from tests/TestUtil.gd rename to tests/gut_ex.gd diff --git a/tests/RepeatedControlHost.gd b/tests/repeated_control_host.gd similarity index 100% rename from tests/RepeatedControlHost.gd rename to tests/repeated_control_host.gd diff --git a/tests/RepeatedControlHost.tscn b/tests/repeated_control_host.tscn similarity index 84% rename from tests/RepeatedControlHost.tscn rename to tests/repeated_control_host.tscn index 5baade7..b7ba179 100644 --- a/tests/RepeatedControlHost.tscn +++ b/tests/repeated_control_host.tscn @@ -1,7 +1,7 @@ [gd_scene load_steps=3 format=3 uid="uid://dtu0iv2dbkinx"] -[ext_resource type="Script" path="res://tests/RepeatedControlHost.gd" id="1_say2d"] -[ext_resource type="Script" path="res://addons/DataBindControls/BindRepeat.gd" id="3_w8au5"] +[ext_resource type="Script" path="res://tests/repeated_control_host.gd" id="1_say2d"] +[ext_resource type="Script" path="res://addons/DataBindControls/bind_repeat.gd" id="3_w8au5"] [node name="RepeatedControlHost" type="Control"] layout_mode = 3 diff --git a/tests/Subscriber.gd b/tests/subscriber.gd similarity index 100% rename from tests/Subscriber.gd rename to tests/subscriber.gd diff --git a/tests/test_bind_repeat.gd b/tests/test_bind_repeat.gd index 5c44b31..c83f76b 100644 --- a/tests/test_bind_repeat.gd +++ b/tests/test_bind_repeat.gd @@ -1,6 +1,6 @@ extends GutTest -const REPEATED_CONTROL_HOST = preload("./RepeatedControlHost.tscn") +const REPEATED_CONTROL_HOST = preload("./repeated_control_host.tscn") func test_bind_repeat(): From e0a2ed1ba099bd32a97ce68115a693fbf6f71444 Mon Sep 17 00:00:00 2001 From: Jamie Pate Date: Sun, 26 Jan 2025 14:09:20 -0800 Subject: [PATCH 3/5] Refactor bind_target to do less work every time Save BindTargets for each bound property at ready The only work done during change detection is `get_target()`, `set_value()` and `get_value()` --- addons/DataBindControls/bind_items.gd | 139 ++++++++++++++----------- addons/DataBindControls/bind_repeat.gd | 12 ++- addons/DataBindControls/bind_target.gd | 51 +++++---- addons/DataBindControls/binds.gd | 38 ++++--- repeated_example.tscn | 1 + 5 files changed, 140 insertions(+), 101 deletions(-) diff --git a/addons/DataBindControls/bind_items.gd b/addons/DataBindControls/bind_items.gd index 5b33bf6..c98ee65 100644 --- a/addons/DataBindControls/bind_items.gd +++ b/addons/DataBindControls/bind_items.gd @@ -38,6 +38,9 @@ extends Binds var _script_property_list: Array[Dictionary] = get_script().get_script_property_list().filter( func(p): return p.name.begins_with("item_") ) +var _bound_array: BindTarget +var _bound_selected_item: BindTarget +var _bound_item_props: Dictionary func _get_configuration_warnings() -> PackedStringArray: @@ -97,12 +100,19 @@ func _set_item_selected(value: String) -> void: func _ready() -> void: if Engine.is_editor_hint(): return - var value = _get_value() + _bound_array = BindTarget.new(array_bind, owner) + _bound_selected_item = BindTarget.new(selected_item, owner) + for p in _script_property_list: + var bind_expr = self[p.name] + if bind_expr: + var bt := BindTarget.new(bind_expr, null) + _bound_item_props[bind_expr] = bt + var value = _get_array_value() if value: detect_changes(value) -func _get_target(): +func _get_items_target(): ## target can be the parent, but if it's a MenuButton or something similar ## try the `get_popup()` method to get the 'real' target var parent := get_parent() @@ -111,72 +121,75 @@ func _get_target(): return parent.get_popup() if parent.has_method("get_popup") else parent -func _get_value(silent := false): - var bt = BindTarget.new(array_bind, owner, silent) - return bt.get_value() +func _get_array_value(silent := false): + var target = _bound_array.get_target() + return _bound_array.get_value(target) if target else [] func detect_changes(new_array := []) -> bool: var change_log := [] if len(new_array) == 0: - var v = _get_value() + var v = _get_array_value() new_array = v if v != null else [] var size = len(new_array) - var t = _get_target() + var it = _get_items_target() # TODO: maybe just check for has_method(`get_item_*`)? - assert(t is ItemList || t is PopupMenu || t is OptionButton) + assert(it is ItemList || it is PopupMenu || it is OptionButton) # Todo: track items so we don't have to assign the entire array var change_detected = false - while size > t.get_item_count(): + while size > it.get_item_count(): change_detected = true - t.add_item("") - while size < t.get_item_count(): + it.add_item("") + while size < it.get_item_count(): change_detected = true - t.remove_item(t.get_item_count() - 1) + it.remove_item(it.get_item_count() - 1) for i in range(size): - change_detected = _assign_item(t, i, new_array[i]) || change_detected + change_detected = _assign_item(it, i, new_array[i]) || change_detected var parent = get_parent() if selected_item && "selected" in parent: - var bt := BindTarget.new(selected_item, owner) - var item = new_array[parent.selected] if parent.selected >= 0 else null - var model_item = bt.get_value() - if !_equal_approx(model_item, item): - var idx = new_array.find(model_item) - parent.selected = idx - var new_item = new_array[idx] if idx >= 0 else null - bt.set_value(new_item) - model_item = bt.get_value() - if _equal_approx(new_item, model_item): - change_detected = true - change_log.append("%s: %s != %s" % [bt.full_path, model_item, item]) - else: - printerr( - ( - "WARNING: %s.selected_item %s: %s != %s (could not be assigned?)" - % [get_path(), bt.full_path, model_item, new_item] + var bt := _bound_selected_item + var target = bt.get_target() + if target: + var item = new_array[parent.selected] if parent.selected >= 0 else null + var model_item = bt.get_value(target) + if !_equal_approx(model_item, item): + var idx = new_array.find(model_item) + parent.selected = idx + var new_item = new_array[idx] if idx >= 0 else null + bt.set_value(target, new_item) + model_item = bt.get_value(target) + if _equal_approx(new_item, model_item): + change_detected = true + change_log.append("%s: %s != %s" % [bt.full_path, model_item, item]) + else: + printerr( + ( + "WARNING: %s.selected_item %s: %s != %s (could not be assigned?)" + % [get_path(), bt.full_path, model_item, new_item] + ) ) - ) change_detected = change_detected || super() _detected_change_log.append_array(change_log) return change_detected func _on_item_selected(_idx: int) -> void: - var bt := BindTarget.new(array_bind, owner) - var array_model = bt.get_value() if bt else null - var target = _get_target() + var bt := _bound_array + var target = bt.get_target() + var array_model = bt.get_value(target) if bt && target else null + var items_target = _get_items_target() var selected = PackedByteArray() - selected.resize(target.get_item_count()) - if target.has_method("get_selected_items"): - for i in target.get_selected_items(): + selected.resize(items_target.get_item_count()) + if items_target.has_method("get_selected_items"): + for i in items_target.get_selected_items(): selected[i] = 1 else: for i in len(selected): selected[i] = 1 if _idx == i else 0 if array_model && item_selected: - for i in range(target.get_item_count()): + for i in range(items_target.get_item_count()): var model = array_model[i] var value = selected[i] == 1 if model[item_selected] != value: @@ -186,7 +199,7 @@ func _on_item_selected(_idx: int) -> void: _on_parent_prop_changed0("selected") if selected_item: bt = BindTarget.new(selected_item, owner) - bt.set_value(array_model[_idx]) + bt.set_value(target, array_model[_idx]) DataBindings.detect_changes() @@ -194,7 +207,7 @@ func _on_multi_selected(idx: int, _selected: bool) -> void: _on_item_selected(idx) -func _assign_item(target: Node, i: int, item) -> bool: +func _assign_item(items_target: Node, i: int, item) -> bool: var change_detected := false var pl = _script_property_list for p in pl: @@ -207,13 +220,13 @@ func _assign_item(target: Node, i: int, item) -> bool: get_method_name = "is_selected" var bind_expr = self[p.name] - - var bt := BindTarget.new(bind_expr, item) if bind_expr else null - if bt && bt.target && target.has_method(get_method_name): - var new_value = bt.get_value() + var bt = _bound_item_props[bind_expr] if bind_expr else null + var target = bt.get_target(item) if bt else null + if bt && target && items_target.has_method(get_method_name): + var new_value = bt.get_value(target) var update := true - if target.has_method(set_method_name): - var old_value = target.call(get_method_name, i) + if items_target.has_method(set_method_name): + var old_value = items_target.call(get_method_name, i) update = typeof(old_value) != typeof(new_value) || old_value != new_value change_detected = change_detected || update if update: @@ -224,11 +237,11 @@ func _assign_item(target: Node, i: int, item) -> bool: if set_method_name == "select": if new_value: # singleselect = false - target.select(i, false) + items_target.select(i, false) else: - target.deselect(i) + items_target.deselect(i) else: - target.call(set_method_name, i, new_value) + items_target.call(set_method_name, i, new_value) return change_detected @@ -241,27 +254,27 @@ func _exit_tree(): func _bind_item_control(): - var target = _get_target() - if target.has_signal("multi_selected"): - var err = target.multi_selected.connect(_on_multi_selected) + var items_target = _get_items_target() + if items_target.has_signal("multi_selected"): + var err = items_target.multi_selected.connect(_on_multi_selected) assert(err == OK) - if target.has_signal("item_selected"): - var err = target.item_selected.connect(_on_item_selected) + if items_target.has_signal("item_selected"): + var err = items_target.item_selected.connect(_on_item_selected) assert(err == OK) - elif target.has_signal("index_pressed"): + elif items_target.has_signal("index_pressed"): # PopupMenu - var err = target.index_pressed.connect(_on_item_selected) + var err = items_target.index_pressed.connect(_on_item_selected) assert(err == OK) func _unbind_item_control(): - var target = _get_target() - if target.has_signal("multi_selected"): - target.multi_selected.disconnect(_on_multi_selected) - if target.has_signal("item_selected"): - target.item_selected.disconnect(_on_item_selected) - elif target.has_signal("index_pressed"): - target.index_pressed.disconnect(_on_item_selected) + var items_target = _get_items_target() + if items_target.has_signal("multi_selected"): + items_target.multi_selected.disconnect(_on_multi_selected) + if items_target.has_signal("item_selected"): + items_target.item_selected.disconnect(_on_item_selected) + elif items_target.has_signal("index_pressed"): + items_target.index_pressed.disconnect(_on_item_selected) func get_desc(): diff --git a/addons/DataBindControls/bind_repeat.gd b/addons/DataBindControls/bind_repeat.gd index 6206e1e..c821321 100644 --- a/addons/DataBindControls/bind_repeat.gd +++ b/addons/DataBindControls/bind_repeat.gd @@ -18,6 +18,7 @@ var _template: Control = null var _template_connections := [] var _owner var _detected_change_log := [] +var _bound_array: BindTarget func _init(): @@ -28,6 +29,7 @@ func _ready(): if Engine.is_editor_hint(): return call_deferred("_deferred_ready") + _bound_array = BindTarget.new(array_bind, owner) func _deferred_ready(): @@ -53,15 +55,15 @@ func _deferred_ready(): _template.remove_child(self) tparent.add_child(self, false, Node.INTERNAL_MODE_BACK) tparent.remove_child(_template) - var value = _get_value(true) + var value = _get_array_value() if value != null: detect_changes(value) -func _get_value(silent := false): +func _get_array_value(): if array_bind && target_property: - var bt = BindTarget.new(array_bind, owner, silent) - return bt.get_value() if bt.target else null + var target = _bound_array.get_target() + return _bound_array.get_value(target) if target else null return null @@ -81,7 +83,7 @@ func detect_changes(new_value: Array = []) -> bool: if !_template: return false if len(new_value) == 0: - var v = _get_value() + var v = _get_array_value() new_value = v if v != null else [] var p := get_parent() var size = len(new_value) diff --git a/addons/DataBindControls/bind_target.gd b/addons/DataBindControls/bind_target.gd index 6779e5f..d5f4cc8 100644 --- a/addons/DataBindControls/bind_target.gd +++ b/addons/DataBindControls/bind_target.gd @@ -2,22 +2,41 @@ extends RefCounted var full_path: String var root -var target = null var prop := "" -var callable_str := "" +var method_name := "" +var _path: Array[String] +var _silent: bool -func _init(_path: String, _root, silent := false): - full_path = _path + +## Create a BindTarget for the selected path and root +## Root may be null, in which case it must be passed when calling get_target() +func _init(path: String, _root, silent := false): + full_path = path root = _root - var path := Array(_path.split(".")) + _silent = silent + _path.assign(path.split(".")) + if len(_path) >= 1: + var last := _path.back() + if last.ends_with("()"): + method_name = last.trim_suffix("()") + else: + prop = last + + +## Get the target relative to the root object. +## if this BindTarget has no root you must provide one here. +func get_target(target_root = null): var t = root - while len(path) > 1: - var p = path.pop_front() + if root == null: + assert(target_root) + t = target_root + for i in range(len(_path) - 1): + var p = _path[i] if t != null && t is Object && p in t: t = t[p] else: - if !silent: + if !_silent: printerr( ( "Unable to find bind %s on %s" @@ -25,23 +44,19 @@ func _init(_path: String, _root, silent := false): ) ) break - if len(path) == 1: - if path[0].ends_with("()"): - callable_str = path[0].trim_suffix("()") - else: - prop = path[0] - target = t + return t -func get_value(): +## call get_target() first to get the target and pass it in +func get_value(target): assert(target) - if callable_str: - var callable = Callable(target, callable_str) + if method_name: + var callable = Callable(target, method_name) return callable.call() assert(prop in target, "%s not found in %s (%s)" % [prop, target, full_path]) return target.get(prop) -func set_value(value): +func set_value(target, value): assert(target && prop) target.set(prop, value) diff --git a/addons/DataBindControls/binds.gd b/addons/DataBindControls/binds.gd index 07549ef..f15ebe7 100644 --- a/addons/DataBindControls/binds.gd +++ b/addons/DataBindControls/binds.gd @@ -8,6 +8,7 @@ extends Node const BindTarget := preload("./bind_target.gd") const Util := preload("./util.gd") const NUM_TYPES: Array[Variant.Type] = [TYPE_INT, TYPE_FLOAT] +const OBJ_TYPES: Array[Variant.Type] = [TYPE_NIL, TYPE_OBJECT] const PASSTHROUGH_PROPS := [ "editor_description", "process_mode", "process_priority", "script", "import_path" @@ -22,6 +23,7 @@ const SIGNAL_PROPS := { selected = "item_selected" } +var _bound_targets := {} var _binds := {} var _detected_change_log := [] @@ -154,8 +156,10 @@ func _bind_target(p: String, parent: Node) -> void: var b := _binds[p] as String if b: var bt = BindTarget.new(b, owner) - if bt.target: - parent[p] = bt.get_value() + var target = bt.get_target() + _bound_targets[b] = bt + if target: + parent[p] = bt.get_value(target) var sig_map = Util.get_sig_map(parent) if p in SIGNAL_PROPS: var sig = SIGNAL_PROPS[p] @@ -176,7 +180,7 @@ func _unbind_targets(): assert(parent) for p in _binds: _unbind_target(p, parent) - + _bound_targets = {} if parent.has_signal("visibility_changed"): parent.visibility_changed.disconnect(_on_parent_visibility_changed) @@ -209,12 +213,11 @@ func _on_parent_prop_changed1(value, prop_name: String): if prop_name != "visible" && !parent.is_visible_in_tree(): return value = parent[prop_name] - var path = _binds[prop_name] - if path: - var bt = BindTarget.new(path, owner) - if bt.get_value() != value: - bt.set_value(value) - DataBindings.detect_changes() + var bt = _bound_targets[_binds[prop_name]] + var target = bt.get_target() + if bt.get_value(target) != value: + bt.set_value(target, value) + DataBindings.detect_changes() func detect_changes() -> bool: @@ -225,11 +228,13 @@ func detect_changes() -> bool: continue var b := _binds[p] as String if b: - var bt = BindTarget.new(b, owner) - if bt.target: + var bt = _bound_targets[b] + assert(bt.root == owner) + var target = bt.get_target() + if target: var parent = get_parent() if parent.is_visible_in_tree() || p == "visible": - var value = bt.get_value() + var value = bt.get_value(target) if !_equal_approx(parent[p], value): _detected_change_log.append( "%s: %s != %s" % [bt.full_path, parent[p], value] @@ -247,9 +252,12 @@ func detect_changes() -> bool: func _equal_approx(a, b): var a_type := typeof(a) var b_type := typeof(b) - # only compare different types if they are both numbers - if a_type != b_type && !(a_type in NUM_TYPES && b_type in NUM_TYPES): - return false + if a_type != b_type: + # compare different types if they are both numbers + var numbers = a_type in NUM_TYPES && b_type in NUM_TYPES + var objects = a_type in OBJ_TYPES && b_type in OBJ_TYPES + if !numbers && !objects: + return false if a_type == TYPE_FLOAT || b_type == TYPE_FLOAT: return is_equal_approx(a, b) return a == b diff --git a/repeated_example.tscn b/repeated_example.tscn index 9fd6950..1c32287 100644 --- a/repeated_example.tscn +++ b/repeated_example.tscn @@ -112,6 +112,7 @@ script = ExtResource("2") text = "item.text" [node name="TextEdit" type="TextEdit" parent="VBoxContainer"] +custom_minimum_size = Vector2(0, 80) layout_mode = 2 text = "text: root1 pressed: False From 5dece9a1f713f24bbb4f1227978bab013c8d05f4 Mon Sep 17 00:00:00 2001 From: Jamie Pate Date: Sun, 26 Jan 2025 17:47:19 -0800 Subject: [PATCH 4/5] Add change detection benchmark including visibility optimizations --- .github/workflows/ci.yaml | 25 +++- .gitignore | 4 +- addons/DataBindControls/bind_items.gd | 2 +- addons/DataBindControls/bind_repeat.gd | 30 +++- addons/DataBindControls/binds.gd | 46 +++--- .../DataBindControls/data_bindings_global.gd | 131 +++++++++++++++--- addons/DataBindControls/util.gd | 2 - repeated_example.tscn | 1 + test.sh | 33 ++++- tests/benchmark.gd | 124 +++++++++++++++++ tests/benchmark.tscn | 15 ++ tests/visibility/visibility_rig.gd | 102 ++++++++++++++ tests/visibility/visibility_rig.tscn | 38 +++++ tests/visibility/visibility_rig_content.gd | 17 +++ tests/visibility/visibility_rig_content.tscn | 29 ++++ 15 files changed, 541 insertions(+), 58 deletions(-) create mode 100644 tests/benchmark.gd create mode 100644 tests/benchmark.tscn create mode 100644 tests/visibility/visibility_rig.gd create mode 100644 tests/visibility/visibility_rig.tscn create mode 100644 tests/visibility/visibility_rig_content.gd create mode 100644 tests/visibility/visibility_rig_content.tscn diff --git a/.github/workflows/ci.yaml b/.github/workflows/ci.yaml index a27ed5e..4f1d2a7 100644 --- a/.github/workflows/ci.yaml +++ b/.github/workflows/ci.yaml @@ -4,30 +4,45 @@ name: Continuous integration jobs: gdlint: - name: gdlint scripts + name: Lint and Format checks runs-on: ubuntu-latest steps: - # Check out the repository - name: Checkout uses: actions/checkout@v4 - # Ensure python is installed - name: Set up Python uses: actions/setup-python@v4 with: python-version: '3.10' - # Install gdtoolkit - name: Install Dependencies run: | python -m pip install --upgrade pip python -m pip install 'gdtoolkit==4.3.2' - # Lint the godot-xr-tools addon - name: Lint and Format Checks run: | ./check.sh + + benchmark: + name: Performance Benchmark + runs-on: ubuntu-latest + container: + image: barichello/godot-ci:latest + + steps: + - name: Checkout + uses: actions/checkout@v4 + + - name: Godot Import + run: godot --headless --import + + # TODO: run with the previous version and compare the results! + - name: Benchmark + run: | + ./test.sh --benchmark-only + test: name: Test runs-on: ubuntu-latest diff --git a/.gitignore b/.gitignore index f794fee..bccabb5 100644 --- a/.gitignore +++ b/.gitignore @@ -1,5 +1,7 @@ /.godot -addons/* +/addons/* !addons/DataBindControls +/benchmark.json + .py_venv .DS_STORE diff --git a/addons/DataBindControls/bind_items.gd b/addons/DataBindControls/bind_items.gd index c98ee65..630f6d3 100644 --- a/addons/DataBindControls/bind_items.gd +++ b/addons/DataBindControls/bind_items.gd @@ -121,7 +121,7 @@ func _get_items_target(): return parent.get_popup() if parent.has_method("get_popup") else parent -func _get_array_value(silent := false): +func _get_array_value(): var target = _bound_array.get_target() return _bound_array.get_value(target) if target else [] diff --git a/addons/DataBindControls/bind_repeat.gd b/addons/DataBindControls/bind_repeat.gd index c821321..6bd5222 100644 --- a/addons/DataBindControls/bind_repeat.gd +++ b/addons/DataBindControls/bind_repeat.gd @@ -21,10 +21,6 @@ var _detected_change_log := [] var _bound_array: BindTarget -func _init(): - add_to_group(Util.BIND_GROUP) - - func _ready(): if Engine.is_editor_hint(): return @@ -108,15 +104,18 @@ func detect_changes(new_value: Array = []) -> bool: return change_detected -func _assign_item(child, item, i): +func _assign_item(child, item, i) -> bool: + var result := false if array_bind && target_property in child: var m = child[target_property] var current_value = child[target_property] if typeof(current_value) != typeof(item) || current_value != item: + result = true _detected_change_log.append( "[%s].%s: %s != %s" % [i, target_property, current_value, item] ) child[target_property] = item + return result func _notification(what): @@ -126,6 +125,10 @@ func _notification(what): _template = null +func _parent_visibility_changed(): + DataBindings.update_bind_visibility(self) + + func _enter_tree(): if Engine.is_editor_hint(): return @@ -133,6 +136,23 @@ func _enter_tree(): owner = _owner _owner = null assert(owner) + DataBindings.add_bind(self) + var p := get_parent() + if p && p.has_signal("visibility_changed"): + p.visibility_changed.connect(_parent_visibility_changed) + + +func _exit_tree(): + if Engine.is_editor_hint(): + return + DataBindings.remove_bind(self) + var p := get_parent() + if p && p.has_signal("visibility_changed"): + p.visibility_changed.disconnect(_parent_visibility_changed) + + +func change_count(): + return len(_detected_change_log) func get_desc(): diff --git a/addons/DataBindControls/binds.gd b/addons/DataBindControls/binds.gd index f15ebe7..8cdd6f4 100644 --- a/addons/DataBindControls/binds.gd +++ b/addons/DataBindControls/binds.gd @@ -28,10 +28,6 @@ var _binds := {} var _detected_change_log := [] -func _init(): - add_to_group(Util.BIND_GROUP) - - func _get_property_list(): # it seems impossible to do an inherited call of _get_property_list() directly. return _binds_get_property_list() @@ -138,12 +134,14 @@ func _enter_tree(): if Engine.is_editor_hint(): return _bind_targets() + DataBindings.add_bind(self) func _bind_targets(): var parent := get_parent() for p in _binds: _bind_target(p, parent) + # visibility changed notifications don't propagate to non-controls if parent.has_signal("visibility_changed"): parent.visibility_changed.connect(_on_parent_visibility_changed) @@ -172,6 +170,7 @@ func _bind_target(p: String, parent: Node) -> void: func _exit_tree(): if Engine.is_editor_hint(): return + DataBindings.remove_bind(self) _unbind_targets() @@ -185,6 +184,10 @@ func _unbind_targets(): parent.visibility_changed.disconnect(_on_parent_visibility_changed) +func _on_parent_visibility_changed(): + DataBindings.update_bind_visibility(self) + + func _unbind_target(p: String, parent: Node): if p in PASSTHROUGH_PROPS: return @@ -198,20 +201,12 @@ func _unbind_target(p: String, parent: Node): parent.disconnect(SIGNAL_PROPS[p], Callable(self, method)) -func _on_parent_visibility_changed(): - # If visibility changes we need to redetect changes because - # changes are ignored when controls are hidden. - DataBindings.detect_changes() - - func _on_parent_prop_changed0(prop_name: String): _on_parent_prop_changed1(null, prop_name) func _on_parent_prop_changed1(value, prop_name: String): var parent := get_parent() - if prop_name != "visible" && !parent.is_visible_in_tree(): - return value = parent[prop_name] var bt = _bound_targets[_binds[prop_name]] var target = bt.get_target() @@ -233,19 +228,16 @@ func detect_changes() -> bool: var target = bt.get_target() if target: var parent = get_parent() - if parent.is_visible_in_tree() || p == "visible": - var value = bt.get_value(target) - if !_equal_approx(parent[p], value): - _detected_change_log.append( - "%s: %s != %s" % [bt.full_path, parent[p], value] - ) - changes_detected = true - var cp - if "caret_column" in parent: - cp = parent.caret_column - parent[p] = value - if "caret_column" in parent: - parent.caret_column = cp + var value = bt.get_value(target) + if !_equal_approx(parent[p], value): + _detected_change_log.append("%s: %s != %s" % [bt.full_path, parent[p], value]) + changes_detected = true + var cp + if "caret_column" in parent: + cp = parent.caret_column + parent[p] = value + if "caret_column" in parent: + parent.caret_column = cp return changes_detected @@ -263,5 +255,9 @@ func _equal_approx(a, b): return a == b +func change_count(): + return len(_detected_change_log) + + func get_desc(): return "%s\n%s" % [get_path(), "\n".join(_detected_change_log)] diff --git a/addons/DataBindControls/data_bindings_global.gd b/addons/DataBindControls/data_bindings_global.gd index d5677c4..d7a5596 100644 --- a/addons/DataBindControls/data_bindings_global.gd +++ b/addons/DataBindControls/data_bindings_global.gd @@ -6,19 +6,33 @@ const MAX_CHANGES = 100 const MAX_CHANGES_LOGGED = 20 var slow_detection_threshold := 2 if EngineDebugger.is_active() && !OS.has_feature("mobile") else 8 +var vp_info := ViewportInfo.new() var _change_detection_queued := false +var _vp_visibility_update_queued := false +var _changes_detected := 0 +var _detection_iterations := 0 +var _detection_count := 0 +var _visible_binds: Dictionary +var _binds: Dictionary +# count how many bind visibility updates happened +var _vbind_plus: int +var _vbind_minus: int +var _vbind_time: int class DrawDetector: extends Control + signal draw_requested + const META_KEY = "data_bindings_draw_detector" var last_frame := 0 - static func ensure(vp: SubViewport): + static func ensure(vp: SubViewport, callable: Callable): if !vp.has_meta(META_KEY): var dd := DrawDetector.new() + dd.draw_requested.connect(callable) vp.set_meta(META_KEY, dd.get_instance_id()) vp.add_child(dd) @@ -36,23 +50,27 @@ class DrawDetector: return dd.last_frame >= fd - 2 return false - func _notification(what: int) -> void: - if what == NOTIFICATION_DRAW: - last_frame = Engine.get_frames_drawn() - - -static var vp_info := ViewportInfo.new() + func _draw() -> void: + last_frame = Engine.get_frames_drawn() + draw_requested.emit() class ViewportInfo: extends RefCounted + signal vp_changed + const OFFSET := 100000 const MIN_SIZE := 128 var _cache := {} + var _seen := {} + var _were_visible := {} var _frame := 0 + func get_were_visible(): + return _were_visible.duplicate() + func is_visible(vp: SubViewport) -> bool: var new_frame := Engine.get_frames_drawn() var rid := vp.get_viewport_rid() @@ -63,7 +81,26 @@ class ViewportInfo: _cache.clear() var size := vp.size var result = 0 - var parent := vp.get_parent() as Node3D + # SubViewport must be a child of Node3D or CanvasItem + var parent = vp.get_parent() as Node3D + if !parent: + parent = vp.get_parent() as CanvasItem + assert( + parent, + "Any SubViewport that contains Binds must have a Node3D or CanvasItem for a parent" + ) + var pid: int = parent.get_instance_id() + var old_pid = _seen.get(rid, -1) + if old_pid == -1: + vp.size_changed.connect(_vp_changed) + if pid != old_pid: + if old_pid != -1: + var obj = instance_from_id(old_pid) + if obj && obj.has_signal("visibility_changed"): + obj.visibility_changed.disconnect(_vp_changed) + if parent.has_signal("visibility_changed"): + parent.visibility_changed.connect(_vp_changed) + _seen[rid] = pid if parent && !parent.is_visible_in_tree(): result = 0 elif size.x < MIN_SIZE && size.y < MIN_SIZE: @@ -74,13 +111,15 @@ class ViewportInfo: if mode == SubViewport.UPDATE_DISABLED: result = 0 elif mode in [SubViewport.UPDATE_WHEN_VISIBLE, SubViewport.UPDATE_WHEN_PARENT_VISIBLE]: - DrawDetector.ensure(vp) + DrawDetector.ensure(vp, _vp_changed) result = 1 if DrawDetector.drawn(vp) else 0 - if result == 0: - pass _cache[rid] = result + _were_visible[rid] = result > 0 return result > 0 + func _vp_changed(): + vp_changed.emit() + func summary(): var always := len( _cache.values().filter(func(v): return v == OFFSET + SubViewport.UPDATE_ALWAYS) @@ -89,19 +128,77 @@ class ViewportInfo: return "%s/%s viewports (%s ALWAYS)" % [count, len(_cache), always] +func _init(): + vp_info.vp_changed.connect(_vp_visibility_updated) + + +func _vp_visibility_updated(): + if !_vp_visibility_update_queued: + _vp_visibility_update_queued = true + _vp_visibility_update.call_deferred(vp_info.get_were_visible()) + + +func _vp_visibility_update(were_visible: Dictionary): + # this can happen frequently even though we debounce it. + # Check to make sure the visibility of each vp actually changed since the last time + # it was checked during detect_changes + var vp_vis_same := {} + for bind in _binds: + var vp := bind.get_viewport() as SubViewport + var same = vp_vis_same.get(vp, null) if vp else null + if !vp || same: + continue + if same == null: + # we don't want to do this part for every bind, try to do it once per viewport + var was_visible = were_visible.get(vp.get_viewport_rid(), null) + same = was_visible == vp_info.is_visible(vp) + vp_vis_same[vp] = same + if same: + continue + update_bind_visibility(bind) + _vp_visibility_update_queued = false + + +func add_bind(bind): + _binds[bind] = true + update_bind_visibility(bind) + + +func remove_bind(bind): + _binds.erase(bind) + _visible_binds.erase(bind) + + +func update_bind_visibility(bind): + var start = Time.get_ticks_usec() + var p = bind.get_parent() + var vp = bind.get_viewport() as SubViewport + if p && p.is_visible_in_tree() && (!vp || vp_info.is_visible(vp)): + _vbind_plus += 1 + if bind not in _visible_binds: + _visible_binds[bind] = true + detect_changes() + else: + _vbind_minus += 1 + _visible_binds.erase(bind) + _vbind_time += Time.get_ticks_usec() - start + + ## queue change detection func detect_changes() -> void: if _change_detection_queued: return _change_detection_queued = true - call_deferred("_detect_changes") + _detect_changes.call_deferred() func _detect_changes(): + _detection_count += 1 # TODO: queue change detection per viewport root or control root? # each piece of 2d UI change detection could happen on a separate frame, spreading out the load.. # 50 binds can take 1ms to check _change_detection_queued = false + _changes_detected = 0 var change_log := [] var i := 0 var changes_detected := true @@ -111,8 +208,7 @@ func _detect_changes(): _change_detection_queued = false changes_detected = false var timings: Array[String] - var binds := get_tree().get_nodes_in_group(Util.BIND_GROUP) - for bind in binds: + for bind in _visible_binds.keys(): var b_start := Time.get_ticks_usec() if !_should_detect_changes(bind): continue @@ -121,6 +217,7 @@ func _detect_changes(): while len(change_log) > MAX_CHANGES_LOGGED: change_log.pop_front() change_log.append(bind.get_desc()) + _changes_detected += bind.change_count() changes_detected = changes_detected || cd var duration = float(Time.get_ticks_usec() - b_start) * 0.001 timings.append("%.2f %s" % [duration, bind.get_desc()]) @@ -132,10 +229,11 @@ func _detect_changes(): timings.reverse() printerr( ( - "Change detection was slow. %s/%s changes took %.2fms!\n%s\n%s" + "Change detection was slow. %s/%s/%s changes took %.2fms!\n%s\n%s" % [ len(timings), - len(binds), + len(_visible_binds), + len(_binds), duration, vp_info.summary(), "\n".join(timings.slice(0, 3)) @@ -152,6 +250,7 @@ func _detect_changes(): breakpoint result = true break + _detection_iterations = i _change_detection_queued = false return result diff --git a/addons/DataBindControls/util.gd b/addons/DataBindControls/util.gd index 03198ef..013d035 100644 --- a/addons/DataBindControls/util.gd +++ b/addons/DataBindControls/util.gd @@ -1,7 +1,5 @@ extends RefCounted -const BIND_GROUP = "__DataBindingBind__" - static func get_sig_map(obj) -> Dictionary: var sig_map := {} diff --git a/repeated_example.tscn b/repeated_example.tscn index 1c32287..bd066cf 100644 --- a/repeated_example.tscn +++ b/repeated_example.tscn @@ -112,6 +112,7 @@ script = ExtResource("2") text = "item.text" [node name="TextEdit" type="TextEdit" parent="VBoxContainer"] +visible = false custom_minimum_size = Vector2(0, 80) layout_mode = 2 text = "text: root1 diff --git a/test.sh b/test.sh index fc1acbb..babf3c8 100755 --- a/test.sh +++ b/test.sh @@ -1,7 +1,7 @@ #!/usr/bin/env bash version=4.3-stable -run_godot() { +run_test() { # this got a bit complicated because gut doesn't detect SCRIPT ERRORs :( # https://github.com/bitwes/Gut/issues/210 local errfile="$(mktemp)" @@ -22,6 +22,23 @@ run_godot() { fi return $result } + +run_bench() { + "$1" --headless res://tests/benchmark.tscn -- "${2:-benchmark.json}" + return $? +} + +bench_flag=1 +test_flag=1 + +if [ "$1" == "--benchmark-only" ]; then + test_flag=0 + shift +elif [ "$1" == "--test-only" ]; then + bench_flag=0 + shift +fi + suffixes=( _linux.x86_64 _win64.exe @@ -36,8 +53,18 @@ for s in "${suffixes[@]}"; do bin=godot fi if [ -x "$(which $bin)" ]; then - run_godot "$bin" - exit $? + set +e + bench_result=0 + test_result=0 + if [ $test_flag == 1 ]; then + run_test "$bin" + test_result=$? + fi + if [ $bench_flag == 1 ]; then + run_bench "$bin" "$@" + bench_result=$? + fi + exit $(( $test_result + $bench_result )) fi done diff --git a/tests/benchmark.gd b/tests/benchmark.gd new file mode 100644 index 0000000..d1a0dd2 --- /dev/null +++ b/tests/benchmark.gd @@ -0,0 +1,124 @@ +extends Panel + +const ITEM_COUNT = 100 +const US_TO_MS = 1e-3 + + +func _run_bench(force_changes: bool): + await get_tree().process_frame + assert(!DataBindings._change_detection_queued) + DataBindings._detect_changes() + if force_changes: + $VisibilityRig.reverse_items() + # Force a new frame to ensure that the viewport_info cache is cleared + await get_tree().process_frame + assert(!DataBindings._change_detection_queued) + var d: int + var start := Time.get_ticks_usec() + DataBindings._detect_changes() + d = Time.get_ticks_usec() - start + return { + it = DataBindings._detection_iterations, + ch = DataBindings._changes_detected, + duration_ms = d * US_TO_MS + } + + +func _ready(): + assert(ITEM_COUNT > 1) + var vp := get_viewport() + if vp.size.x < 1024 || vp.size.y < 600: + # --headless mode gives us a tiny window for some reason + # even if we specify --resolution + vp.size = Vector2i(1024, 600) + assert(vp.size.x > 64) + DataBindings.slow_detection_threshold = 0xFFFFFFF + var times = {} + var timestr = {} + var results = [] + $VisibilityRig.fill_items(ITEM_COUNT) + await get_tree().process_frame + + assert(!DataBindings._change_detection_queued) + DataBindings._detect_changes() + var binds = DataBindings._binds.duplicate() + var bind_count = len(binds) + + times.no_changes = await _run_bench(false) + + times.with_changes = await _run_bench(true) + + $VisibilityRig.reverse_items() + times.vp_vis = await $VisibilityRig.rig_visibility(true, false) + times.hidden_viewport = await _run_bench(true) + + # reset visibility + $VisibilityRig.reverse_items() + times.all_vis1 = await $VisibilityRig.rig_visibility(true, true) + # change detection maybe triggered by visibility changes! + await get_tree().process_frame + + $VisibilityRig.reverse_items() + times.hc_vis = await $VisibilityRig.rig_visibility(false, true) + times.hidden_control = await _run_bench(true) + + $VisibilityRig.reverse_items() + times.all_vis2 = await $VisibilityRig.rig_visibility(true, true) + + # Display results and write to file + var max_name = times.keys().reduce(func(acc, n): return maxi(len(n), acc), 0) + for t in times: + var r = {name = t} + r.merge(times[t]) + results.append(r) + var dict = times[t].duplicate() + dict.d = "%.2f" % [times[t].duration_ms] + dict.name = t.rpad(max_name + 1) + timestr[t] = "{name}: {it}it {ch}ch {d}ms".format(dict) + print( + ( + "CPU: %s %s %s threads" + % [Engine.get_architecture_name(), OS.get_processor_name(), OS.get_processor_count()] + ) + ) + print("Time taken (%s binds)\n%s" % [bind_count, "\n".join(timestr.values())]) + var max_ch = times.with_changes.ch + var min_ch = times.no_changes.ch + var hc_ch = times.hidden_control.ch + var hv_ch = times.hidden_viewport.ch + var result = true + + # some validation to make sure we're benchmarking what we think we are + if hc_ch <= min_ch || hc_ch >= max_ch: + printerr( + ( + "ERROR: Hidden Control test should detect %s < %s < %s changes" + % [min_ch, hc_ch, max_ch] + ) + ) + result = false + if hv_ch <= min_ch || hv_ch >= max_ch: + printerr( + ( + "ERROR: Hidden Viewport test should detect %s < %s < %s changes" + % [min_ch, hv_ch, max_ch] + ) + ) + result = false + if times.all_vis1.it == 0 || times.all_vis1.ch == 0: + printerr("Making viewport binds visible should trigger change detection") + result = false + if times.all_vis2.it == 0 || times.all_vis2.ch == 0: + printerr("Making binds visible should trigger change detection") + result = false + if len(OS.get_cmdline_user_args()): + var filename = OS.get_cmdline_user_args()[0] + print("Writing results to %s" % [filename]) + var f := FileAccess.open(filename, FileAccess.WRITE) + if !f: + printerr("Unable to open %s: %s" % [filename, FileAccess.get_open_error()]) + else: + f.store_string(JSON.stringify(results, " ", false)) + f.close() + + get_tree().quit(0 if result else 1) diff --git a/tests/benchmark.tscn b/tests/benchmark.tscn new file mode 100644 index 0000000..703dc52 --- /dev/null +++ b/tests/benchmark.tscn @@ -0,0 +1,15 @@ +[gd_scene load_steps=3 format=3 uid="uid://bmd7vewvitvxo"] + +[ext_resource type="Script" path="res://tests/benchmark.gd" id="1_tf50s"] +[ext_resource type="PackedScene" uid="uid://s647ot4qd3vq" path="res://tests/visibility/visibility_rig.tscn" id="2_liiis"] + +[node name="Benchmark" type="Panel"] +anchors_preset = 15 +anchor_right = 1.0 +anchor_bottom = 1.0 +grow_horizontal = 2 +grow_vertical = 2 +script = ExtResource("1_tf50s") + +[node name="VisibilityRig" parent="." instance=ExtResource("2_liiis")] +layout_mode = 1 diff --git a/tests/visibility/visibility_rig.gd b/tests/visibility/visibility_rig.gd new file mode 100644 index 0000000..5876d4a --- /dev/null +++ b/tests/visibility/visibility_rig.gd @@ -0,0 +1,102 @@ +extends Panel + +const ExampleItem = preload("res://example_item.gd") +const US_TO_MS = 1e-3 + +var icons = [ + preload("res://addons/DataBindControls/icons/link.svg"), + preload("res://addons/DataBindControls/icons/links.svg"), + preload("res://addons/DataBindControls/icons/list.svg") +] + + +func _ready(): + fill_items(1) + DataBindings.detect_changes() + + +func fill_items(count: int): + %VisibilityRigContent.items.clear() + %VpVisibilityRigContent.items.clear() + for i in range(count): + var item = ExampleItem.new( + { + text = "%sth item" % [count], + pressed = i % 2 == 0, + icon = icons[i % len(icons)], + value = i + } + ) + %VisibilityRigContent.items.append(item) + %VpVisibilityRigContent.items.append(item) + + +func reverse_items(): + %VisibilityRigContent.items.reverse() + %VpVisibilityRigContent.items.reverse() + + +func rig_visibility(content: bool, viewport: bool): + DataBindings._vbind_minus = 0 + DataBindings._vbind_plus = 0 + DataBindings._vbind_time = 0 + var d: int + var start = Time.get_ticks_usec() + var vr_content = %VisibilityRigContent + var vp_container = %SubViewportContainer + var make_visible = content && !vr_content.visible || viewport && !vp_container.visible + var last_dc := DataBindings._detection_count + vr_content.visible = content + vp_container.visible = viewport + d = Time.get_ticks_usec() - start + var vbind_time = DataBindings._vbind_time + # reset _vbind_time so we don't double count time taken before this line + DataBindings._vbind_time = 0 + var change_detected = ( + DataBindings._change_detection_queued || DataBindings._detection_count > last_dc + ) + if !change_detected: + # may take 2 frames if it's a viewport update + await get_tree().process_frame + change_detected = ( + DataBindings._change_detection_queued || DataBindings._detection_count > last_dc + ) + assert(!make_visible || change_detected) + if change_detected: + await get_tree().process_frame + assert(!DataBindings._change_detection_queued) + + # Print how many binds are checked for visibility changes + # Note that viewport ancestor binds make be checked more than expected + print_verbose( + ( + "rig_visibility, making visible: %s %s" + % [ + make_visible, + " ".join( + [ + "+:", + DataBindings._vbind_plus, + "-:", + DataBindings._vbind_minus, + "us:", + DataBindings._vbind_time + vbind_time + ] + ) + ] + ) + ) + d += DataBindings._vbind_time + return { + desc = "Time to toggle visibility (not counting change detection)", + it = DataBindings._detection_iterations if change_detected else 0, + ch = DataBindings._changes_detected if change_detected else 0, + dc = DataBindings._detection_count - last_dc, + duration_ms = d * US_TO_MS + } + + +## Check binds for items that haven't been updated to match. +## Each non-matching item appends it's path to the result +func check_binds() -> Array[String]: + return %VisibilityRigContent.check_binds() + %VpVisibilityRigContent.check_binds() diff --git a/tests/visibility/visibility_rig.tscn b/tests/visibility/visibility_rig.tscn new file mode 100644 index 0000000..8ecbc52 --- /dev/null +++ b/tests/visibility/visibility_rig.tscn @@ -0,0 +1,38 @@ +[gd_scene load_steps=3 format=3 uid="uid://s647ot4qd3vq"] + +[ext_resource type="Script" path="res://tests/visibility/visibility_rig.gd" id="1_kgjgr"] +[ext_resource type="PackedScene" uid="uid://dkary3a5e5dp6" path="res://tests/visibility/visibility_rig_content.tscn" id="2_r3pvu"] + +[node name="VisibilityRig" type="Panel"] +anchors_preset = 15 +anchor_right = 1.0 +anchor_bottom = 1.0 +grow_horizontal = 2 +grow_vertical = 2 +script = ExtResource("1_kgjgr") + +[node name="BoxContainer" type="BoxContainer" parent="."] +layout_mode = 1 +anchors_preset = 15 +anchor_right = 1.0 +anchor_bottom = 1.0 +grow_horizontal = 2 +grow_vertical = 2 + +[node name="VisibilityRigContent" parent="BoxContainer" instance=ExtResource("2_r3pvu")] +unique_name_in_owner = true +layout_mode = 2 + +[node name="SubViewportContainer" type="SubViewportContainer" parent="BoxContainer"] +unique_name_in_owner = true +layout_mode = 2 +size_flags_horizontal = 3 +stretch = true + +[node name="SubViewport" type="SubViewport" parent="BoxContainer/SubViewportContainer"] +handle_input_locally = false +size = Vector2i(574, 648) +render_target_update_mode = 4 + +[node name="VpVisibilityRigContent" parent="BoxContainer/SubViewportContainer/SubViewport" instance=ExtResource("2_r3pvu")] +unique_name_in_owner = true diff --git a/tests/visibility/visibility_rig_content.gd b/tests/visibility/visibility_rig_content.gd new file mode 100644 index 0000000..e2fd337 --- /dev/null +++ b/tests/visibility/visibility_rig_content.gd @@ -0,0 +1,17 @@ +extends ScrollContainer + +const ExampleItem = preload("res://example_item.gd") + +var items: Array[ExampleItem] + + +## Check binds for items that haven't been updated to match. +## Each non-matching item appends it's path to the result +## we can just call detect_changes() because DataBindings._detect_changes() +## would skip any hidden controls and that's what we're interested in. +func check_binds() -> Array[String]: + var result: Array[String] + for bind in DataBindings._binds: + if bind.detect_changes(): + result.append(bind.get_desc()) + return result diff --git a/tests/visibility/visibility_rig_content.tscn b/tests/visibility/visibility_rig_content.tscn new file mode 100644 index 0000000..e9ad75b --- /dev/null +++ b/tests/visibility/visibility_rig_content.tscn @@ -0,0 +1,29 @@ +[gd_scene load_steps=4 format=3 uid="uid://dkary3a5e5dp6"] + +[ext_resource type="Script" path="res://tests/visibility/visibility_rig_content.gd" id="1_5aaji"] +[ext_resource type="PackedScene" uid="uid://ddkq3rwuwxo1d" path="res://repeated_example.tscn" id="1_luv4n"] +[ext_resource type="Script" path="res://addons/DataBindControls/bind_repeat.gd" id="2_3ixi3"] + +[node name="VisibilityRigContent" type="ScrollContainer"] +anchors_preset = 15 +anchor_right = 1.0 +anchor_bottom = 1.0 +grow_horizontal = 2 +grow_vertical = 2 +size_flags_horizontal = 3 +script = ExtResource("1_5aaji") + +[node name="VBoxContainer" type="VBoxContainer" parent="."] +layout_mode = 2 +size_flags_horizontal = 3 + +[node name="RepeatedControl" parent="VBoxContainer" instance=ExtResource("1_luv4n")] +unique_name_in_owner = true +layout_mode = 2 +size_flags_horizontal = 1 +size_flags_vertical = 1 + +[node name="BindRepeat" type="Node" parent="VBoxContainer/RepeatedControl"] +script = ExtResource("2_3ixi3") +array_bind = "items" +target_property = "item" From 3ce99ca6c01c2e3f3641477a3092153b3bce90d4 Mon Sep 17 00:00:00 2001 From: Jamie Pate Date: Fri, 31 Jan 2025 12:34:35 -0800 Subject: [PATCH 5/5] Add gut tests to confirm that visibility culling is working Confirm that change detection is called properly when binds become visible in the tree Confirm that change detection does not get called for invisible items --- .../DataBindControls/data_bindings_global.gd | 16 +- check.sh | 2 +- test.sh | 10 +- tests/test_visibility_culling.gd | 183 ++++++++++++++++++ tests/visibility/visibility_rig.gd | 20 +- 5 files changed, 210 insertions(+), 21 deletions(-) create mode 100644 tests/test_visibility_culling.gd diff --git a/addons/DataBindControls/data_bindings_global.gd b/addons/DataBindControls/data_bindings_global.gd index d7a5596..612882d 100644 --- a/addons/DataBindControls/data_bindings_global.gd +++ b/addons/DataBindControls/data_bindings_global.gd @@ -11,7 +11,9 @@ var vp_info := ViewportInfo.new() var _change_detection_queued := false var _vp_visibility_update_queued := false var _changes_detected := 0 +## Number of iterations taken in the most recent change detection var _detection_iterations := 0 +## number of times _detect_changes() has been called var _detection_count := 0 var _visible_binds: Dictionary var _binds: Dictionary @@ -210,8 +212,6 @@ func _detect_changes(): var timings: Array[String] for bind in _visible_binds.keys(): var b_start := Time.get_ticks_usec() - if !_should_detect_changes(bind): - continue var cd := bind.detect_changes() as bool if cd: while len(change_log) > MAX_CHANGES_LOGGED: @@ -253,15 +253,3 @@ func _detect_changes(): _detection_iterations = i _change_detection_queued = false return result - - -func _should_detect_changes(bind: Node): - var p := bind.get_parent() - if p: - var gp := p.get_parent() - if (gp is Node3D || gp is Control) && !gp.is_visible_in_tree(): - return false - var vp := p.get_viewport() as SubViewport - if vp: - return vp_info.is_visible(vp) - return true diff --git a/check.sh b/check.sh index 74ca42e..b071546 100755 --- a/check.sh +++ b/check.sh @@ -72,7 +72,7 @@ if ! [[ -d "$dir/.py_venv" ]]; then PYTHON_KEYRING_BACKEND=keyring.backends.null.Keyring python -m pip install 'virtualenv' --user python -m virtualenv .py_venv activate_venv - PYTHON_KEYRING_BACKEND=keyring.backends.null.Keyring python -m pip install 'gdtoolkit==4.3.2' + PYTHON_KEYRING_BACKEND=keyring.backends.null.Keyring python -m pip install 'gdtoolkit==4.3.3' else activate_venv fi diff --git a/test.sh b/test.sh index babf3c8..71e001b 100755 --- a/test.sh +++ b/test.sh @@ -5,7 +5,9 @@ run_test() { # this got a bit complicated because gut doesn't detect SCRIPT ERRORs :( # https://github.com/bitwes/Gut/issues/210 local errfile="$(mktemp)" - "$1" --headless -v -s addons/gut/gut_cmdln.gd -gconfig tests/.gutconfig.json 2> "$errfile" & + local cmd="$1" + shift + "$cmd" --headless -v -s addons/gut/gut_cmdln.gd -gconfig tests/.gutconfig.json "$@" 2> "$errfile" & pid=$! tail -f "$errfile" & tailpid=$! @@ -24,7 +26,9 @@ run_test() { } run_bench() { - "$1" --headless res://tests/benchmark.tscn -- "${2:-benchmark.json}" + local cmd="$1" + local output_json="${2:-benchmark.json}" + "$cmd" --headless res://tests/benchmark.tscn -- "$output_json" return $? } @@ -57,7 +61,7 @@ for s in "${suffixes[@]}"; do bench_result=0 test_result=0 if [ $test_flag == 1 ]; then - run_test "$bin" + run_test "$bin" "$@" test_result=$? fi if [ $bench_flag == 1 ]; then diff --git a/tests/test_visibility_culling.gd b/tests/test_visibility_culling.gd new file mode 100644 index 0000000..55e3c10 --- /dev/null +++ b/tests/test_visibility_culling.gd @@ -0,0 +1,183 @@ +extends GutTest + +const VISIBILITY_RIG_SCENE = preload("./visibility/visibility_rig.tscn") +const VisibilityRig = preload("./visibility/visibility_rig.gd") + +var visibility_rig: VisibilityRig + + +func before_each(): + if get_viewport().size.x < 1024 || get_viewport().size.y < 600: + get_viewport().size = Vector2i(1024, 600) + var layer = CanvasLayer.new() + visibility_rig = VISIBILITY_RIG_SCENE.instantiate() + layer.add_child(visibility_rig) + add_child_autofree(layer) + await get_tree().process_frame + assert(!DataBindings._change_detection_queued) + + +func _all_text_changed(changed_vrc, changed_svc = ""): + if !changed_svc: + changed_svc = changed_vrc + return [ + "/VisibilityRigContent/../Label/Binds\nitem.text: " + changed_vrc, + "/VisibilityRigContent/../TextEdit3/Binds\nitem.text: " + changed_vrc, + "/VisibilityRigContent/../LineEdit/Binds\nitem.text: " + changed_vrc, + ( + "/SubViewportContainer/SubViewport/VpVisibilityRigContent/../Label/Binds\nitem.text: " + + changed_svc + ), + ( + "/SubViewportContainer/SubViewport/VpVisibilityRigContent/../TextEdit3/Binds\nitem.text: " + + changed_svc + ), + ( + "/SubViewportContainer/SubViewport/VpVisibilityRigContent/../LineEdit/Binds\nitem.text: " + + changed_svc + ), + ] + + +func test_visibility_culling(): + var vrc_label = visibility_rig.get_node("%VisibilityRigContent").find_child( + "Label", true, false + ) + assert_true(vrc_label.is_visible_in_tree(), "Label is visible in tree") + assert_true( + vrc_label.get_node("Binds") in DataBindings._visible_binds, + "Label/Binds is in _visible_binds" + ) + + var items = visibility_rig.get_items(false) + items[0].text = "TEST1" + DataBindings._detect_changes() + assert_eq(vrc_label.text, "TEST1") + var changed = visibility_rig.check_binds_ignoring_visibility() + + assert_eq(changed, [], "No .text binds should be 'changed'") + assert_eq_deep(changed, []) + + # Hide /VisibilityRigContent + await visibility_rig.rig_visibility(false, true) + assert(!DataBindings._change_detection_queued) + + assert_false(vrc_label.is_visible_in_tree(), "Label is visible in tree") + assert_false( + vrc_label.get_node("Binds") in DataBindings._visible_binds, + "Label/Binds is in _visible_binds" + ) + + items[0].text = "TEST2" + DataBindings._detect_changes() + assert_eq(vrc_label.text, "TEST1") + + items[0].text = "TEST2a" + changed = visibility_rig.check_binds_ignoring_visibility() + assert_eq( + changed, + _all_text_changed("TEST1 != TEST2a", "TEST2 != TEST2a"), + "Hidden .text binds should not have been 'changed'" + ) + assert_eq_deep(changed, _all_text_changed("TEST1 != TEST2a", "TEST2 != TEST2a")) + + items[0].text = "TEST3" + await visibility_rig.rig_visibility(true, true, true) + assert_true(vrc_label.is_visible_in_tree(), "Label is visible in tree") + assert_true( + vrc_label.get_node("Binds") in DataBindings._visible_binds, + "Label/Binds is in _visible_binds" + ) + + assert_true( + DataBindings._change_detection_queued, + "Change detection should be queued by visibility changes" + ) + await get_tree().process_frame + assert_false(DataBindings._change_detection_queued, "Change detection should be done") + + assert_eq(vrc_label.text, "TEST3") + items[0].text = "TEST3a" + changed = visibility_rig.check_binds_ignoring_visibility() + assert_eq_deep(changed, _all_text_changed("TEST3 != TEST3a")) + # wait for change detection to happen before ending the test + await get_tree().process_frame + assert(!DataBindings._change_detection_queued) + + +func test_vp_visibility_culling(): + DataBindings._detect_changes() + var svp_label = visibility_rig.get_node("%SubViewportContainer").find_child( + "Label", true, false + ) + + assert_true(svp_label.is_visible_in_tree(), "Label is visible in tree") + assert_true( + svp_label.get_node("Binds") in DataBindings._visible_binds, + "Label/Binds is in _visible_binds" + ) + + var items = visibility_rig.get_items(false) + items[0].text = "TEST1" + DataBindings._detect_changes() + assert_eq(svp_label.text, "TEST1") + var changed = visibility_rig.check_binds_ignoring_visibility() + + assert_eq(changed, [], "No .text binds should be 'changed'") + assert_eq_deep(changed, []) + + # Hide /SubViewportContainer + await visibility_rig.rig_visibility(true, false) + assert(!DataBindings._change_detection_queued) + assert(!DataBindings._vp_visibility_update_queued) + assert_false( + svp_label.get_viewport().get_parent().is_visible_in_tree(), + "Label's viewport is not visible in tree" + ) + assert_false( + svp_label.get_node("Binds") in DataBindings._visible_binds, + "Label/Binds should not be in _visible_binds" + ) + + items[0].text = "TEST2" + DataBindings._detect_changes() + assert_eq(svp_label.text, "TEST1") + + items[0].text = "TEST2a" + changed = visibility_rig.check_binds_ignoring_visibility() + assert_eq( + changed, + _all_text_changed("TEST2 != TEST2a", "TEST1 != TEST2a"), + "Hidden .text binds should not have been 'changed'" + ) + assert_eq_deep(changed, _all_text_changed("TEST2 != TEST2a", "TEST1 != TEST2a")) + + items[0].text = "TEST3" + await visibility_rig.rig_visibility(true, true, true) + assert_true(svp_label.is_visible_in_tree(), "Label is visible in tree") + + var dc = DataBindings._detection_count + if !DataBindings._change_detection_queued: + await get_tree().process_frame + # it doesn't seem like we can actually catch the queued change detection when a viewport + # queues it, but we can check _detection_count to ensure it's actually happened. + if !DataBindings._change_detection_queued: + assert_lt( + dc, + DataBindings._detection_count, + "Change detection should be queued by visibility changes" + ) + await get_tree().process_frame + assert_true( + svp_label.get_node("Binds") in DataBindings._visible_binds, + "Label/Binds should be in _visible_binds" + ) + assert_false(DataBindings._change_detection_queued, "Change detection should be done") + + assert_eq(svp_label.text, "TEST3") + items[0].text = "TEST3a" + changed = visibility_rig.check_binds_ignoring_visibility() + assert_eq_deep(changed, _all_text_changed("TEST3 != TEST3a")) + # wait for change detection to happen before ending the test + await get_tree().process_frame + assert(!DataBindings._change_detection_queued) diff --git a/tests/visibility/visibility_rig.gd b/tests/visibility/visibility_rig.gd index 5876d4a..b51d200 100644 --- a/tests/visibility/visibility_rig.gd +++ b/tests/visibility/visibility_rig.gd @@ -36,7 +36,13 @@ func reverse_items(): %VpVisibilityRigContent.items.reverse() -func rig_visibility(content: bool, viewport: bool): +func get_items(vr: bool) -> Array[ExampleItem]: + if vr: + return %VpVisibilityRigContent.items + return %VisibilityRigContent.items + + +func rig_visibility(content: bool, viewport: bool, dont_wait := false): DataBindings._vbind_minus = 0 DataBindings._vbind_plus = 0 DataBindings._vbind_time = 0 @@ -49,6 +55,8 @@ func rig_visibility(content: bool, viewport: bool): vr_content.visible = content vp_container.visible = viewport d = Time.get_ticks_usec() - start + if dont_wait: + return var vbind_time = DataBindings._vbind_time # reset _vbind_time so we don't double count time taken before this line DataBindings._vbind_time = 0 @@ -98,5 +106,11 @@ func rig_visibility(content: bool, viewport: bool): ## Check binds for items that haven't been updated to match. ## Each non-matching item appends it's path to the result -func check_binds() -> Array[String]: - return %VisibilityRigContent.check_binds() + %VpVisibilityRigContent.check_binds() +func check_binds_ignoring_visibility() -> Array[String]: + var result = %VisibilityRigContent.check_binds() + %VpVisibilityRigContent.check_binds() + var shorten = func(s): + return s.replace(str($BoxContainer.get_path()), "").replace( + "VBoxContainer/RepeatedControl/VBoxContainer", ".." + ) + result.assign(result.map(shorten)) + return result