I am very undecided here. The importer currently only adds an unconnected node to the Node setup which is meant to be inspected by the user for deciding how to apply the data to the material.
Also in general the mapping of the Phong, Blinn, Lambert to PBSDF is not really defined.

As it stands by now this patch would either keep the Specularity=0.5 (when it is defined in the collada import) or set it 0 when it is not defined in the collada import. This behavior seems at least questionable to me, but maybe it is "common sense" and thus could be tolerated (i do not dare to decide on this)...

@Brecht Van Lommel (brecht) : I recall we talked about this some time ago and as far as i remember we decided to keep away from mapping collada shaders to PBSDF... ?

I don't understand what's wrong with it. This is a strict improvement when the <specular> is black; in other cases, there is no change.

The Principled node basically does (using the Eevee approximation, and ignoring some terms)

output = emission + diffuse term + specular term

Consider the simple <effect>

<lambert>
  <emission><color>...</color></emission>
  <diffuse><color>0 0 0 1</color></diffuse>
</lambert>

Collada says it should be a constant color

output = emission

In this specific case, the mapping from Collada to Principled is clear: the emission terms are equal, the Principled's diffuse term is zero (Base Color is black), so the Principled's specular term just needs to be made zero.

With current Specular=0.5, this is how it looks

This is what it looks like on a model from the "real world" (model from here).

On the left you can see the Specular=0.5 makes everything look shiny. On the right is with this patch.

With your patch the value of the Specular socket can be either Blender's default value (apparently 0.5) or 0.0 depending on the existence of the Specular entry in the collada file.

I guess the best thing we can do is to always set the socket value to 0 and keep with the disconnected node as it is created today.

This way we make no odd assumptions about the Specular value at all. What do you think about this?

So maybe simply skip the is_zero check and always set the default value to 0.0

I honestly have not thought about the non-zero specular case at all. Like I said, this was just to fix the zero specular case, and I didn't change other cases at all.

If you want to always set Specular=0 it would fix the zero case and I suppose not greatly harm the non-zero case since the Specular=0.5 there is presumably just by default, not by intent.

If you want to make a patch for that instead, that's fine with me.

Hi, Bastien;

I do not want to decide this without some advise from the FBX team :)

Quick reminder
Collada supports Constant, Blinn, Phong and Lambert Shaders while Blender uses the principled BSDF shader (by default). About a year ago we (@Brecht Van Lommel (brecht) and if i remember correct you where involved too) decided to not try to map the collada shaders to BSDF. But in order to not lose information the related data (ambient, specular, reflective) is still imported into separate unconnected nodes, such that the user can then decide what to do with the information.

This patch would reset the Specular value of the PBSDF to 0 if Specular is absent or black in the collada file, otherwise let Specular at its default of 0.5. While i see how this can be of benefit, i am unsure if that is the right way to solve the issue.

As a side note: I wonder if there is a general mapping for "old" shaders to PBSDF. If that is the case then i rather would add that mapping to the Collada importer and not fix corner cases one by one.

What if we replaced the principled BSDF Shader by a Specular Shader when we import a material based on a Phong/Blinn Shader ?

• collada :: diffuse -> Specular Shader :: Base Color
• collada :: emission -> Specular Shader :: Emissive Color
• collada :: specular -> Specular Shader :: Specular Color
• collada :: shininess -> Specular Shader :: Roughness (need verification)
• collada :: transparency -> Specular shader :: transparency

then we "only" need to figure out how we can map the remaining collada attributes:

• cot transparent
• cot reflective
• double reflectivity
• double ior
• cot ambient

Could this become a major improvement after all ?

Can you keep this focused on the zero-specularity issue? <constant>/<lambert> would be the most common case for that.

I know what you mean and i would wish that i just could say, lets approve the patch and move on, it works for most cases, so what?

But there are a couple of reasons why i hesitate to be lax here. What disturbs me most is that the principled BSDF shader actually seems not to be very "compatible" with the Shader types supported by Collada. Especially the "Specular" Input of the PBSDF shader (a value) is very different from the Collada Specular (color or texture).

So we would compare apples with pears here and then we might make bad decisions without noticing. For example: from my investigation so far the Specular Input of the PBSDF shader seems to be not the right place for fixing this issue even when it seems to work on the surface.

This is my motivation to look into replacing the currently created PBSDF shader by something better suited for the purpose. But this might include some more investigations into the imported Shader types (constant, Lambert, Phong and Blinn) and how to map the Collada-Shader values to the Blender shaders.

I hope that this issue and maybe a few others can be fixed in an easy and robust way. Your patches are helping a lot here, but we must take the time and at the very least check what the FBX people are thinking about this. They even might have already come up with their own solutions for this.

The Specular shader node is an Eevee only feature at the moment. I don't mind it being added to Cycles, but it's not there right now, so not ideal for importers to rely on it.

FBX uses a Python script that converts parameters to the Principled BSDF.

As far as I can tell this patch looks like a bugfix for the COLLADA importer, if there is no specular component it makes sense to set that to zero rather than the default in the Principled BSDF. The default values of the Principled BSDF are not intended to be specifically meaningful for importers.

@Scurest (scurest) Many thanks for submitting the patch!

Btw there's a mismatched ) in the comment in set_specular now: /* no specular term) */.