The problem with this solution is that VisualShader nodes have to count the embeds manually. If someone adds a new node with a resource, but does not mark embed, it will miss the error.

Came back to this. I wonder how we feel about adding a hint_default("res://blabla.png") ?

@KoBeWi

If someone adds a new node with a resource, but does not mark embed,

Assuming you are talking about VisualShaderNodeCustom - VisualShaderNodeCustom cannot have Resource Embeds, nothing in the current API provides that functionality unless I am misunderstanding.

@QbieShay

Came back to this. I wonder how we feel about adding a hint_default("res://blabla.png") ?

We talked about this in the Rendering Meeting today (Aug.20) (You were there :) ) and determined hint_default is not a workable feature.

Conclusion we reached in the Rendering Meeting for the issue is to throw a warning popup when the conversion plugin is called via Editor, and fall back to throwing an Error (like the current solution) when the conversion is initiated via code. I will adjust my fix to throw the popup when I have some spare time

Assuming you are talking about VisualShaderNodeCustom

No, adding a new visual shader node type in the engine. This PR modifies many classes, and in the future there may be more classes that will need similar checks.

The problem with this solution is that VisualShader nodes have to count the embeds manually. If someone adds a new node with a resource, but does not mark embed, it will miss the error.
No, adding a new visual shader node type in the engine. This PR modifies many classes, and in the future there may be more classes that will need similar checks.

Tracking embeds is necessary to catch and communicate this bad conversion behavior. I think the responsibility of tracking embeds is best assigned to individual node classes. Currently only 6 VisualShaderNode classes can have embedded resources, and I would say safe handling of resources on future node classes is important, and low cost.

I have a hard time envisioning an alternative architecture that defers the responsibility of tracking embeds somewhere else? Managing this information from the Visual Shader Editor Plugin seems like it would require more work than having each node track itself, and I don't know if the Visual Shader resource Class has enough info to do any of this tracking.

@KoBeWi Talked about it in the rendering meeting today, and have updated the PR with a different architecture for the fix.

// Returns 0 if no embeds, 1 if external embeds, 2 if builtin embeds
int VisualShader::has_node_embeds() const {
	bool external_embeds = false;
	for (int i = 0; i < TYPE_MAX; i++) {
		for (const KeyValue<int, Node> &E : graph[i].nodes) {
			List<PropertyInfo> props;
			E.value.node->get_property_list(&props);
			// For classes that inherit from VisualShaderNode, the class properties start at the 12th, and the last value is always 'script'
			for (int i = 12; i < props.size() - 1; i++) {
				// VisualShaderNodeCustom cannot have embeds
				if (props.get(i).name == "VisualShaderNodeCustom") {
					break;
				}
				// Ref<Resource> properties get classed as type Variant::Object
				if (props.get(i).type == Variant::OBJECT) {
					Ref<Resource> res = E.value.node->get(props.get(i).name);
					if (res.is_valid()) {
						if (res->is_built_in()) {
							return 2;
						} else {
							external_embeds = true;
						}
					}
				}
			}
		}
	}

	return external_embeds;
}

This single function in the VisualShader class should be able to catch which child nodes have embedded resource properties without requiring any overhead on the VisualShaderNode classes

I tested the MRP from #101375 with the newest version of this PR and it doesn't seem to do anything. Conversion is still broken and there is no error/warning. Maybe I'm doing something wrong, idk.

@KoBeWi My bad! Silly mistake, I've pushed a fix.

I changed an inner loop variable name 'i'->'j' to satisfy the code check that inner loop should not use same name as outer loop, but forget to replace in a few relevant locations. Fixed, tested, works.

Looks fine now, but I'd change the warning to a toaster message, as it's easy to miss.

Also converting the shader from the inspector results in clearing the property if there are sub-resources.

This could be avoided by returning the same shader instead of null.

@KoBeWi Updated warning language, and changed to throw a Toaster Message (when it can - I think there are cases where a conversion plugin can be invoked outside of editor)

Also converting the shader from the inspector results in clearing the property if there are sub-resources.
This could be avoided by returning the same shader instead of null.

Made this update too, although I think it'd be preferable if the Inspector Dock handled a null return and recognized the value as unchanged. Looking for the file that manages that ;)

Looks like easy fix:

Added a catch in Editor_Resource_Picker conversion to handle being given null by a failed conversion. Returned the Conversion plugin to always return null in a failed conversion.

Also added emit_signal(SNAME("file_removed") to the FileSystemDock conversion action - without this, if a VisualShader was converted to Shader, the ShaderEditor would keep the VisualShader version open as a hanging [unsaved]

Pushed a build addressing @KoBeWi and @AThousandShips comments.

Thanks!

Cherry-picked for 4.5.2.