First import fixes #155

manuq · 2024-07-23T13:23:24Z

https://phabricator.endlessm.com/T35574

wjt · 2024-07-23T13:30:49Z

addons/block_code/drag_manager/drag.gd

@@ -0,0 +1,195 @@
+extends Control


Does this not want:

@tool class_name Drag

?

Oh good catch!

OK based on the comment below, we want @tool but not class_name?

Exactly, I just added a fixup.

I would assume that since Drag is only loaded from DragManager, which is itself a @tool script, then it's not strictly necessary here. But I don't really understand the intricacies of @tool.

wjt · 2024-07-23T13:31:36Z

addons/block_code/ui/blocks/utilities/drag_drop_area/drag_drop_area.gd

@@ -1,5 +1,4 @@
 @tool
-class_name DragDropArea


What's the effect of this change – does it prevent the class being visible to the end user?

With this, the class is not registered by name in ClassDB and it can't be referenced directly in other scripts, it must be preloaded. Also, if the script extends Node or a subclass of Node, it won't appear in the UI to the end user when adding a node.

Another advantage: it won't collide with classes defined by the end user in the project. Our current Types class has a good chance to conflict given the generic name.

I'll add this explanation to the commit message.

dbnicholson · 2024-07-23T15:21:17Z

FWIW, I wrote a very similar series in https://github.com/endlessm/godot-block-coding/tree/T35494-godot-4.3-importing last week. With that I was able to import on 4.3 with no warnings or errors.

dbnicholson · 2024-07-23T15:24:56Z

addons/block_code/drag_manager/drag.gd

+var _preview_block: Control
+var _snap_points: Array[Node]
+var _delete_areas: Array[Rect2]
+var action: DragManager.DragAction:


Since DragAction is only used in this class, you can move the enum here and avoid a circular dependency between DragManager and Drag.

dbnicholson · 2024-07-23T15:30:08Z

addons/block_code/inspector_plugin/block_script_inspector.gd

@@ -1,5 +1,7 @@
 extends EditorInspectorPlugin

+const BlockCodePlugin = preload("res://addons/block_code/block_code_plugin.gd")


The reason for this is actually pretty ugly. It's to get access to the BlockCodePlugin.main_panel static singleton. It would be better to pass in the handler so that's not needed. I started working on it but set it aside for the moment.

manuq · 2024-07-23T15:39:03Z

FWIW, I wrote a very similar series in https://github.com/endlessm/godot-block-coding/tree/T35494-godot-4.3-importing last week. With that I was able to import on 4.3 with no warnings or errors.

Ouch! I didn't saw that branch, sorry. Which one would you like to favour?

dbnicholson · 2024-07-23T15:40:15Z

addons/block_code/ui/picker/categories/category_factory.gd

@@ -158,7 +160,7 @@ static func get_categories(blocks: Array[Block], extra_categories: Array[BlockCa
 	# Accessing a static Callable from a static function fails in 4.2.1.
 	# Use the fully qualified name.
 	# https://github.com/godotengine/godot/issues/86032
-	cats.sort_custom(CategoryFactory._category_cmp)
+	cats.sort_custom(_category_cmp)


This invalidates the comment above it. If I read godotengine/godot#86032, I think this will fail on 4.2.1 with an anonymous class. It would be worth testing in 4.2.1. Then I think either we keep the global CategoryFactory class or change the requirements to 4.2.2.

Ohh, I'm blind today. Let's keep the global class for now, with a comment.

These are now always preloaded in the scripts that depend on them. Makes the plugin work again after enabling it. https://phabricator.endlessm.com/T35574

dbnicholson · 2024-07-23T15:44:51Z

FWIW, I wrote a very similar series in https://github.com/endlessm/godot-block-coding/tree/T35494-godot-4.3-importing last week. With that I was able to import on 4.3 with no warnings or errors.

Ouch! I didn't saw that branch, sorry. Which one would you like to favour?

Oh, this is fine. I just pushed it this morning when I saw this PR. I had planned to poke at it a bit more, but since you've got it going here we might as well finish it off. I would consider cherry picking this. Also, as mentioned in a comment above, in my version of breaking out the Drag class, I also moved the enum DragAction to break a circular dependency between DragManager and Drag.

I can split out the changes to build SimpleCharacter and SimpleScoring without a scene file to a separate PR.

manuq · 2024-07-23T16:04:37Z

@dbnicholson thanks! Cherry-picked the instructions tree change. Tested everything again by copying to a new project and the plugin enables fine.

dbnicholson

Ugh. Apparently GUT does not exit with a failure on syntax errors:

[INFO]:  using [/home/runner/.local/share/godot/app_userdata/Block Coding Plugin] for temporary output.
Godot version:  4.2.2
GUT version:  9.2.1
SCRIPT ERROR: Parse Error: Could not find type "BlockCategory" in the current scope.
          at: GDScript::reload (res://tests/test_category_factory.gd:12)
ERROR: Failed to load script "res://tests/test_category_factory.gd" with error "Parse error".
   at: load (modules/gdscript/gdscript.cpp:2726)
[WARNING]:  Ignoring script res://tests/test_category_factory.gd because it does not extend GutTest


res://tests/test_instruction_tree.gd
* test_single_node_text
* test_root_depth_text
* test_child_node_text
* test_sibling_node_text
* test_tree_node_text
* test_script_no_nodes
* test_script_no_entry_blocks
* test_basic_script
SCRIPT ERROR: Invalid call. Nonexistent function 'insert_snapped_block' in base 'Nil'.
          at: test_basic_script (res://tests/test_instruction_tree.gd:[13](https://github.com/endlessm/godot-block-coding/actions/runs/10062323917/job/27814558872?pr=155#step:5:14)4)
    [Orphans]:  16 new orphans in test.
* test_multiple_entry_script
SCRIPT ERROR: Invalid call. Nonexistent function 'insert_snapped_block' in base 'Nil'.
          at: test_multiple_entry_script (res://tests/test_instruction_tree.gd:162)
    [Orphans]:  16 new orphans in test.
* test_signal_script
SCRIPT ERROR: Invalid call. Nonexistent function 'insert_snapped_block' in base 'Nil'.
          at: test_signal_script (res://tests/test_instruction_tree.gd:195)
    [Orphans]:  [14](https://github.com/endlessm/godot-block-coding/actions/runs/10062323917/job/27814558872?pr=155#step:5:15)5 new orphans in test.
[Orphans]:  177 new orphans in script.
10/10 passed.

I think all the Nonexistent function 'insert_snapped_block' in base 'Nil'. are due to reverting c59e48e but not adjusting the tests. That can be fixed separately. However, Could not find type "BlockCategory" in the current scope. is almost certainly caused by these changes and should be fixed. I'll try to look at making sure GUT fails in these scenarios.

manuq · 2024-07-23T18:06:21Z

@dbnicholson ouch, so the CI was giving a false positive. Let me fix the one corresponding to this PR.

dbnicholson · 2024-07-23T18:12:27Z

@dbnicholson ouch, so the CI was giving a false positive. Let me fix the one corresponding to this PR.

I found bitwes/Gut#210 upstream.

dbnicholson

Looks good now. I'll take a look at the SnapPoint fix separately.

Except for: - The resources: they must be classes - Block and subclasses: they are accessed by class using the ClassDB - SnapPoint: there is an "is SnapPoint" check - the CategoryFactory (bug in Godot 4.2.1) With this, the class is not registered by name in ClassDB and it can't be referenced directly in other scripts, it must be preloaded. Also, if the script extends Node or a subclass of Node, it won't appear in the UI to the end user when adding a node. Another advantage: it won't collide with classes defined by the end user in the project. Our current Types class has a good chance to conflict given the generic name. Also: delete PackedSceneTreeNode and PackedScene abandoned resources.

This static function has nothing to do with the DragManager class since it's simply walking a tree of Nodes and looking for a Block that has a non-empty scope property set. I'm not sure it really fits in InstructionTree either, but that's a better fit at least. This avoids some dependency issues between DragManager, its inner Drag class, and BlockCanvas. https://phabricator.endlessm.com/T35494

SimpleScoring: Remove print leftover

13cdca3

wjt reviewed Jul 23, 2024

View reviewed changes

dbnicholson reviewed Jul 23, 2024

View reviewed changes

DragManager: Move Drag class to its own file

a064abb

manuq force-pushed the first-import-fixes branch from 3b82304 to 036f5e9 Compare July 23, 2024 15:37

manuq marked this pull request as ready for review July 23, 2024 15:37

dbnicholson reviewed Jul 23, 2024

View reviewed changes

DragManager: Remove class_name from internal scripts

3267242

These are now always preloaded in the scripts that depend on them. Makes the plugin work again after enabling it. https://phabricator.endlessm.com/T35574

manuq force-pushed the first-import-fixes branch from 036f5e9 to c5290bb Compare July 23, 2024 15:44

manuq force-pushed the first-import-fixes branch from c5290bb to 69a7c6b Compare July 23, 2024 15:50

dbnicholson requested changes Jul 23, 2024

View reviewed changes

dbnicholson approved these changes Jul 23, 2024

View reviewed changes

manuq and others added 2 commits July 23, 2024 14:57

dbnicholson force-pushed the first-import-fixes branch from 23eccb7 to 626ab89 Compare July 23, 2024 20:57

dbnicholson merged commit 6ea45f7 into main Jul 23, 2024
2 checks passed

dbnicholson deleted the first-import-fixes branch July 23, 2024 20:59

		@@ -1,5 +1,7 @@
		extends EditorInspectorPlugin

		const BlockCodePlugin = preload("res://addons/block_code/block_code_plugin.gd")

First import fixes #155

First import fixes #155

Uh oh!

Conversation

manuq commented Jul 23, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dbnicholson commented Jul 23, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

manuq commented Jul 23, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dbnicholson commented Jul 23, 2024

Uh oh!

manuq commented Jul 23, 2024

Uh oh!

dbnicholson left a comment

Choose a reason for hiding this comment

Uh oh!

manuq commented Jul 23, 2024

Uh oh!

dbnicholson commented Jul 23, 2024

Uh oh!

dbnicholson left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!