Fix external Material for invalid Material

[antonWetzel]

Closes #64853

"Use External" works for Imported Assets (Blender, GLTF) even without a Material to replace.
The Implementation requires the importer to assign a unique "filler_id" as meta data to a Material if the Material does not have a valid import id or name.

I could not find a solution without edits in Importers because the Materials have no identification to find the right external Material to replace.

Bugsquad edit: fixes: #64970

[antonWetzel]

@godotengine/import

I looked at the different importers

• GLTF and Blender: Creates for every surface without material a new material
• Collada: Creates no material for surfaces without one
• Filmbox: Creates one default material for every surface without material
• (Wavefront: Creates a immutable mesh)

The current implementation works for all consistent, because every importer (except GLTF and derived) names all the materials.

One unified behavior for missing Materials would be nice, but not my focus of this pull request.

[antonWetzel]

Change iteration to unique name based on mesh and surface id to keep reference if other meshes are added.

[clayjohn]

Looks fine to me. I tested the PR and it worked. This is a huge QoL improvement for people using the new advanced importer.

I don't do much work in this area so it would be nice to get another set of eyes on this. Perhaps @fire or @SaracenOne since they have done some work in this area in the past.

[lyuma]

I have concerns about this:

mat3d->set_meta("filler_id", vformat("@FILLER:%s:%s", import_mesh->get_name(), itos(j)));

This looks to me like it's going to cause a new filler material for each mesh.
If I make a GLTF scene with no materials but a lot of meshes, does this prevent godot from batching them?

[antonWetzel]

@lyuma The current implementation (master) creates a new Material for every surface of every mesh. The filler-id is only used to replace the material if use external is set; if use external is not used the result before and after commit should be equal.

[clayjohn]

If I make a GLTF scene with no materials but a lot of meshes, does this prevent godot from batching them?

No. Godot caches StandardMaterials based on the generated shader not on anything related to the Resource or its metadata

[fire]

The rest looks good. I want to focus attention on the exact wording of this filler_id because we may have to live with it for a while.

"@FILLER:%s:%s"

Edited:

Is this the first time we use this concept?

[fire]

1. Do we use @FILLER style anywhere else? Like at and upper case filler.
2. Do we want to synchronize with the gltf material placeholder codes?
3. The double colons looks weird.
4. We've been avoid using meta for anything in editor. Don't have any ideas other than creating a property.

[antonWetzel]

1.

I used import_id as orientation.

godot/editor/import/scene_import_settings.cpp

Lines 146 to 154 in 6f5704d

	if (p_material->has_meta("import_id")) {
	import_id = p_material->get_meta("import_id");
	has_import_id = true;
	} else if (!p_material->get_name().is_empty()) {
	import_id = p_material->get_name();
	has_import_id = true;
	} else {
	import_id = "@MATERIAL:" + itos(material_set.size() - 1);
	}

@Filler or FILLER is never used anywhere else

2.

Don't know how the placeholder codes work. (Every Material after the import need an identifier, even if the GLTF-file has zero materials)

3.

_ and - are alerady used in other parts of the content and spaces feel wrong

4.

Don't know the import system enough to know what to change

Ideas

Change meta to "identifier" (maybe "unique_identifier" or "unique_id")
Change content to "%s@%s", import_mesh->get_name(), itos(j)

The content is only visible in the <file>.import.

Current

_subresources={
"materials": {
"@FILLER:Assets-f1737e9e675e596b2cfccf62dc0e906e_Cube002:0": {
"use_external/enabled": true,
"use_external/path": "res://Assets/Materials/Bullet.tres"
},

Idea

_subresources={
"materials": {
"Assets-f1737e9e675e596b2cfccf62dc0e906e_Cube002@0": {
"use_external/enabled": true,
"use_external/path": "res://Assets/Materials/Bullet.tres"
},

[lyuma]

I'll still voice my preference that there should be exactly one material for default (I know it's not currently implemented this way, but it should be), and therefore the name of the filler material could be something generic like "" or "@default" or "@FILLER" doesn't really matter, but it avoids the whole problem of needing one per material slot.

Note that all this whole issue is addressing the off-chance that the user somehow didn't assign any materials in the DCC and wants to assign materials one-by-one in Godot rather than in original content creation tool where they should be doing this in the first place

If we want per-mesh or per-material slot overrides, that should be done by selecting the mesh or mesh instance in the import screen, and that should have an option to do a per-mesh or per-material slot override for just that mesh... if we do add that, it should be a separate bespoke importer feature, not special behavior only used for filler materials.

[TokisanGames]

Doesn't this also fix #64970 according to Op? #64970 (comment)

@lyuma Note that all this whole issue is addressing the off-chance that the user somehow didn't assign any materials in the DCC and wants to assign materials one-by-one in Godot rather than in original content creation tool where they should be doing this in the first place

This isn't an off chance. This is more likely the default case for most experienced developers. Blender and Godot materials are not the same, no matter how smooth the conversion process is, and they need tweaking in Godot. All of my materials are saved to disk as text files, with all textures pre-converted to DDS. Many of the materials are ShaderMaterials, some are standard. I wouldn't use any materials or textures embedded in GLBs as I don't want materials I can't change, nor redundant data in my git repo. If I were importing a premade GLB with textures, I'd import it twice, once to extract the material and textures, then I'd rexport from blender w/ mesh data only. I'd prefer not using the GLB at all, but I cannot until Godot fully supports the native ArrayMesh format.

[Zireael07]

I agree, even if I model something in Blender I apply materials in Godot itself.

[akien-mga]

We discussed this in a PR review meeting; without being very familiar with the import workflow here it's a bit difficult to assess if this is the right/best solution for the proposed workflow.

We could give it a try and see if it fits the needs of all users or not. It seems this unique ID is only set for glTF, so I wonder what's the workflow like for other formats? (Though .blend and .fbx both go through glTF now, so it's pretty much just OBJ and COLLADA aside from it.)

WDYT @fire @lyuma @SaracenOne?

[antonWetzel]

It seems this unique ID is only set for glTF, so I wonder what's the workflow like for other formats?

Only materials can be replaced with other materials. The other converters create Meshes without Materials and the missing materials can't be replaced.

I would prefer if the "Replace Material"-option is per surface, not per existing material, but this requires a major change in the import implementation.

[jss2a98aj]

After trying a build of 4.1.1 with the changes here added, I can't find any issues with it on the editor side. It works exactly as I had expected things to by default.
Edit: Works fine when added to 4.1.2 as well. I have been using Godot with the changes here extensively since making this post with no problems.
Edit 2: Still does the job in 4.2
Edit 3: 4.3 as well.

[YuriSizov]

Poking @fire @lyuma @SaracenOne again :) What would be your decision on this change?

[Jamsers]

Originally, setting Use External: Enabled on a <Unnamed Material> and assigning a material would crash Godot. This got fixed, but now we have this behavior where we assign a material to <Unnamed Material> in the importer, and it has no effect.

We have no intention of merging fixes of this nature because we're waiting on a GLTF spec update to fundamentally fix the issue. Correct?

If we're gonna wait all the way till the GLTF spec update, in the meantime we have this gaping UX issue. Users will expect this functionality to work - it doesn't, so it's considered a bug. If it's not a bug because it's not supposed to work until the GLTF spec gets updated, shouldn't we just straight up disable setting Use External: Enabled on a <Unnamed Material> then?

For me for example, I'm assigning materials for thousands of meshes in both the importer panel and the inherited scenes. I'm doing so because the inherited scenes are the only way to use GLTFs with <Unnamed Material>s and put materials on them, for now. But I assign them in the importer panel as well in the hopes that this issue will get fixed in the future, and I can stop relying on inherited scenes in the future.

Should I just not bother and stick exclusively to inherited scenes then?

[DeeJayLSP]

Needs rebase

[lyuma]

Superseded by #107339

Thanks for creating the original PR and starting the discussion, @antonWetzel