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

Add support for exporting script classes without a name #82528

Merged
merged 1 commit into from
Dec 8, 2023

Conversation

SaracenOne
Copy link
Member

@SaracenOne SaracenOne commented Sep 29, 2023

In the current editor implementation, you can export classes with custom script types and have the correct type validation be performed for both the inspector drag and drop behaviour and the scene tree selection popup. However, you can also export classes which are not defined by custom classes, but by script path constants, but these will not be validated by either of these editor options. This PR will check the script path of nodes to determine if they match. It also checks the inheritence of scripts.

It's debatable whether this is technically a bug or not, but it deviates from standard expecting operating behaviour given what is permissible in scripting, and is generally quite annoying if you don't like defining custom class names for every minor script.

godot windows editor dev x86_64_K3615bWo5I

@KoBeWi
Copy link
Member

KoBeWi commented Sep 30, 2023

It should show the script's icon (its own icon if set or base node type). Also instead of full path, which can be very long, I think it should be filename only.

@SaracenOne
Copy link
Member Author

Okay, I'll see if I can edit this to make the result a bit more presentable

@SaracenOne SaracenOne force-pushed the path_types branch 2 times, most recently from 8927682 to edd8668 Compare September 30, 2023 18:10
@SaracenOne
Copy link
Member Author

godot windows editor dev x86_64_GfdSHtjtmY
There we go, it will now show the filename instead of the full path. It will also attempt to find the most appropiate icon, first checking the inheritence chain, and if it can't find anything, it will fall back to the icon of the base type.

@KoBeWi
Copy link
Member

KoBeWi commented Sep 30, 2023

You could've used EditorData's get_script_icon(). Sorry for not mentioning it earlier .-.

@SaracenOne
Copy link
Member Author

Okay, I'll update it again to use this function

@SaracenOne
Copy link
Member Author

Hmm, that function appears to be bugged, doesn't return an icon from scripts that don't have any custom defined icon. I guess I'll open up another PR for that once I've tested things.

@SaracenOne
Copy link
Member Author

Actually, 'fixing' this might or might not be desirable. Allowing that fuction to return the base script type changes how scripts are loaded in the inspector
godot windows editor dev x86_64_RU0DIROJsk
godot windows editor dev x86_64_i2q0vaTtf1

@SaracenOne
Copy link
Member Author

SaracenOne commented Sep 30, 2023

We could either:
A. Keep it as is
B. Change the script icon return function to return the base types
C. Change it so that when the type is a script type without an explicit icon, use the same cog icon as in the inspector uses currently.

@KoBeWi
Copy link
Member

KoBeWi commented Sep 30, 2023

Maybe try _get_class_or_script_icon()?
EDIT:
No it's private. But there is get_class_icon(), so you could use it as fallback if script has no icon.

@SaracenOne
Copy link
Member Author

SaracenOne commented Nov 4, 2023

@KoBeWi Okay, I've gone back to have a look at this. The solution I've landed on is basically deriving the icon during the set_valid_types method and storing it in the metadata.

		// Attempt to get the correct name and icon for script path types
		String type_name = type;
		Ref<Texture2D> type_icon = EditorNode::get_singleton()->get_class_icon(type);

		// If we can't find a global class icon, try to find one for the script.
		if (type_icon.is_null()) {
			if (ResourceLoader::exists(type, "Script")) {
				Ref<Script> node_script = ResourceLoader::load(type);
				if (node_script.is_valid()) {
					type_name = type_name.get_file();
					type_icon = EditorNode::get_editor_data().get_script_icon(node_script);
					if (type_icon.is_null()) {
						type_icon = EditorNode::get_singleton()->get_class_icon(node_script->get_class_name());
					}
				}
			}
		}

This is a bit more simplified and also solves an issue with the earlier implementation that it wouldn't use custom icons unless we defined a global class. This time, it simply attempts to get a class icon, and if it fails, it will check if a script exists for that path. If it does, it will change the type name to just the name of the script and will use the get_script_icon method. Since this doesn't work for scripts which don't have explicitly defined custom icons, I decided instead of trying to determine the base type, to just fall back on the script's class's own icon if a custom icon cannot be found. This is consistent with how script icons are displayed in the inspector and seems pretty consistent.

Basically, you now get a script icon (a cog for GDScript specifically), or you get whatever you define with icon() now displaying correctly without the need give your script an explicit class_name.

// If we can't find a global class icon, try to find one for the script.
if (type_icon.is_null()) {
if (ResourceLoader::exists(type, "Script")) {
Ref<Script> node_script = ResourceLoader::load(type);
Copy link
Member

Choose a reason for hiding this comment

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

I think there might be a better way, but at least it works.

Comment on lines +502 to +509
Ref<Script> node_script = p_node->get_script();
while (node_script.is_valid()) {
if (node_script->get_path() == E) {
valid = true;
break;
}
node_script = node_script->get_base_script();
}
Copy link
Member

Choose a reason for hiding this comment

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

This could be changed to method probably, but I wonder how it could be reused in editor properties too (since it's the same).

@SaracenOne
Copy link
Member Author

Updated some of the things pointed out.

@YuriSizov YuriSizov requested a review from KoBeWi November 6, 2023 12:46
@YuriSizov YuriSizov removed this from the 4.2 milestone Nov 6, 2023
@YuriSizov YuriSizov added this to the 4.3 milestone Nov 6, 2023
@YuriSizov YuriSizov added the cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release label Nov 6, 2023
Allow script path type hints to be used in drag and drop
and scene tree popup.
@SaracenOne
Copy link
Member Author

Updated now.

@KoBeWi
Copy link
Member

KoBeWi commented Nov 8, 2023

The base node class icon is not displayed, generic Script icon is used:
image
Although that's probably a problem with get_object_icon() (though normally it's expected, because this icon shows in Script field).

@YuriSizov YuriSizov changed the title Script path type support in editor Add support for exporting script classes without a name Dec 4, 2023
@YuriSizov YuriSizov added enhancement and removed bug labels Dec 4, 2023
@YuriSizov YuriSizov merged commit 0481a0b into godotengine:master Dec 8, 2023
15 checks passed
@YuriSizov
Copy link
Contributor

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release enhancement topic:editor usability
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants