Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Scripts fail to load with a parse error on exported projects if it uses editor classes #91713

Open
ydeltastar opened this issue May 8, 2024 · 12 comments

Comments

@ydeltastar
Copy link
Contributor

ydeltastar commented May 8, 2024

Tested versions

v4.3.dev.custom_build [2042420]

System information

Windows 11

Issue description

Tool scripts using an editor class such as EditorInterface will fail to load with a parse error only on exported projects.

@tool
extends Node3D

func _init() -> void:
	if Engine.is_editor_hint():
		var editor_filesystem = EditorInterface.get_resource_filesystem()
		if not editor_filesystem.resources_reimported.is_connected(_on_resource_reimport):
			editor_filesystem.resources_reimported.connect(_on_resource_reimport)

func _on_resource_reimport(resources:PackedStringArray):
	pass

Output of debug template console wrapper:

SCRIPT ERROR: Parse Error: Identifier "EditorInterface" not declared in the current scope.
          at: GDScript::reload (res://node_3d.gd:6)
ERROR: Failed to load script "res://node_3d.gd" with error "Parse error".
   at: ResourceFormatLoaderGDScript::load (modules\gdscript\gdscript.cpp:2894)

Well, it makes sense. Editor classes aren't included in distribution builds but there is no conditional compiling in GDScript so tool scripts with game and editor code can't be used at all in a project.

Workarounds:

  • Reject types, return to dynamic: var editor_interface = Engine.get_singleton("EditorInterface").
  • Distribute the source project with godot.exe.

Steps to reproduce

Export a project that uses the script above with a debug template and run the .console.exe version.

Minimal reproduction project (MRP)

N/A

@AThousandShips
Copy link
Member

Related:

@KoBeWi
Copy link
Member

KoBeWi commented May 8, 2024

Reject types, return to dynamic: var editor_interface = Engine.get_singleton("EditorInterface").

IMO this is the way. You can do this indirectly for partial safety:

func _ready():
	if Engine.is_editor_hint():
		var editor = load("res://some_script_using_editor_interface.gd")
		editor.connect_reimported(_on_resource_reimport)

where the loaded script does what you quoted above. It can provide a type-safe interface with only the script in the scene doing unsafe calls.

@ydeltastar ydeltastar changed the title Scripts fail to load with a parse error on export projects if it uses editor classes Scripts fail to load with a parse error on exported projects if it uses editor classes May 8, 2024
@dalexeev
Copy link
Member

dalexeev commented May 8, 2024

It can provide a type-safe interface with only the script in the scene doing unsafe calls.

This will not work with static typing, only with dynamic typing (if you use Engine.get_singleton() and so on for all editor classes).

As a workaround, I would suggest my plugin gdscript-preprocessor (sorry for the self-promotion).

We could try to solve this out of the box by adding an exception for editor classes when analyzing/compiling the script. However, at runtime this should still give an error if execution reaches this point. So I think that this is a rather complex solution, which also adds inconsistency.

@KoBeWi
Copy link
Member

KoBeWi commented May 8, 2024

This will not work with static typing, only with dynamic typing (if you use Engine.get_singleton() and so on for all editor classes).

No I mean this (for some_script_using_editor_interface.gd from my example):

func connect_reimported(callback: Callable):
	var editor_filesystem := EditorInterface.get_resource_filesystem()
	if not editor_filesystem.resources_reimported.is_connected(callback):
		editor_filesystem.resources_reimported.connect(callback)

This code is type-safe, but it won't cause problems, because it's loaded using load(). The parent script does a dynamic call on this method. Hence I said "partially safe".

@ydeltastar
Copy link
Contributor Author

Also, this isn't mentioned in the documentation and works when running from the editor leaving it to be discovery possible late in development only after exporting.

@KoBeWi
Copy link
Member

KoBeWi commented May 8, 2024

Also related: #56798 (I just remembered)

@ydeltastar
Copy link
Contributor Author

ydeltastar commented May 8, 2024

Related: #86013

@github-project-automation github-project-automation bot moved this to For team assessment in GDScript Issue Triage May 17, 2024
@vnen vnen moved this from For team assessment to Up for grabs in GDScript Issue Triage May 18, 2024
tcoxon added a commit to tcoxon/Terrain3D that referenced this issue Jul 15, 2024
… exported build.

Even though the script doesn't do anything at runtime, it still exists, and its use of the EditorInterface class
causes a load failure in Godot builds that don't have editor classes.
godotengine/godot#91713
TokisanGames pushed a commit to TokisanGames/Terrain3D that referenced this issue Jul 17, 2024
… exported build.

Even though the script doesn't do anything at runtime, it still exists, and its use of the EditorInterface class
causes a load failure in Godot builds that don't have editor classes.
godotengine/godot#91713
@lucassene
Copy link

Got this happening with me on exported games on 4.3.

The first workaround suggested by @ydeltastar , worked so far.

@TokisanGames
Copy link
Contributor

Just discovered this. I have tool scripts in my game that do stuff in the editor, and already filter their actions based on the editor hint. We made EditorInterface static #75694 to make tool scripts easier. But to break an exported game because a script mentions, but doesn't use the class isn't good.

What about replacing editor classes with dummy classes?

How about recognizing a code block that starts with if Engine.is_editor_hint(): and not including it in the export?

What about giving us preprocessor defines that demark editor only code, to replace if Engine.is_editor_hint() entirely?

@ydeltastar
Copy link
Contributor Author

ydeltastar commented Dec 19, 2024

Taking Python as an example, the following will execute without errors when evaluating to false even if EditorInterface doesn't exist.

print("Input anything besides 'editor' to succeeded")
x = input()

if "editor" in x:
    print(EditorInterface.get_stuff())
else:
    print("it works")

Python probably replaces it with a placeholder or an error opcode. No need for workarounds on the user side if the parser step is less strict during compilation.

@dalexeev
Copy link
Member

Python probably replaces it with a placeholder or an error opcode.

Python is a purely dynamic language, type hints are completely optional. Therefore, any non-existent identifiers do not cause a runtime error until execution reaches them.

GDScript combines the qualities of both dynamic and static typing. GDScript uses type hints and implicitly inferred types for static analysis and optimizations, and reports non-existent identifiers (but this does not apply to attribute/index access). Changing this behavior may be undesirable because it helps users find errors faster.

You can use Engine.get_singleton(&"EditorInterface") instead of EditorInterface, as documented in the description of the new @export_tool_button annotation. Similar techniques can be used for other cases besides accessing editor singletons.

But I agree that this is a pretty important point that can hinder users who want to have different code for different platforms, operating systems, distribution stores and other types of engine builds and custom feature tags. I think there are the following options to solve this problem:

  1. Introduce a conditional compilation system in the form of preprocessor directives or another mechanism. This is quite complicated, but could potentially be useful for other scenarios.
  2. Change the analyzer and compiler behavior only on export and in the exported project. Instead of a compile-time error, generate a "get global" opcode.
  3. Replace the error about a non-existent identifier with a warning, which is treated as an error by default (for compatibility). I think this option is most consistent with the principles of GDScript.

@aekobear
Copy link
Contributor

aekobear commented Jan 3, 2025

This bug is hard to uncover with inherited classes. Take this example:

@tool
extends Node
class_name BaseTool

func editor_selection():
  EditorInterface.get_selection()
@tool
extends BaseTool
class_name CustomTool

func _ready():
  print("CustomTool is working!");

If you create a scene with a single CustomTool node, it works perfectly in the editor. But when you export, you get the error:

SCRIPT ERROR: Parse Error: Could not resolve class "BaseTool".
          at: GDScript::reload (res://custom-tool.gd:0)
ERROR: Failed to load script "res://custom-tool.gd" with error "Parse error".
   at: load (modules/gdscript/gdscript.cpp:2775)

There's no mention of EditorInterface at all. Combined with the fact that this functionality only happens on export and is entirely undocumented, basically the only way to uncover the problem is to comment out lines of code one at a time until the problem goes away.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Up for grabs
Development

No branches or pull requests

7 participants