What about doing this in the case where we have an emission but no diffuse color set:

if diffuse is missing but has emission:
    set diffuse to emission color (or black)

Otherwise i fear the entire scene could become filled with black items...

It has to be black. Quoting from the 1.4 spec, Ch 8, common_color_or_texture_type (8-26)

The schema does not specify default colors for <ambient>, <diffuse> and other child elements of the
shaders <blinn>, <constant>, <lambert>, and <phong>. If any child element is unspecified, apply
the specified shader equation without that portion. This provides equivalent results to explicitly specifying
black for that child element.

If you have

<lambert>
  <emission><color>0.5 0.5 0.5 1</color></emission>
</lambert>

this is what the difference looks like. The cube on the right is correct.

Ok, that makes sense. Then do we agree to do it like this?:

if diffuse is missing but has emission:
    set diffuse to black

The logic behind this is: If there is a material defined without diffuse color but other parameters are included, then this is either a mistake on the exporter's side (not to be handled by us) or it is on purpose. In the latter case we must assume the exporter refers to the collada rules and thus the diffuse color must be set to black.

If you agree on this then you might want to modify the patch accordingly and add a comment that refers to the collada rule to explain why this is done.

The spec says an omitted <diffuse> is the same as black. It doesn't depend on emission or anything.

@Scurest (scurest) I am sorry to tell you that the arc land did not land the patch in your name although it told me it would do so. I apologize for that. But at least the comment refers to this patch. If you wish i can try to get that fixed. Anyways thank you for the patch!

@Gaia Clary (gaiaclary) It's fine, you changed it anyway. Thanks for merging.

With this, I think the the diffuse/emission/specular are all correct for any <constant> or <lambert> shader. So that's nice I guess.

@Gaia Clary (gaiaclary) I also just noticed when you reduced duplication, you also changed it to set the material's Viewport Display color to black when there is no diffuse term. Was that intentional? I think the old behavior of leaving it the default color was better. The new behavior makes eg. any <constant> shader appear black in the Solid view.

This is exactly what i meant when in my comment on Sat, May 15, 9:41 AM (see old comments in this Differential) It also sounds very wrong to me to have the basic color set to Default while the Shader Diffuse color is set to Black.
The whole Materials handling needs a bit of a rework anyways, so i will create a Task for this later.

Oh, well I wasn't suggesting changing the viewport color.

But I think a good way to set viewport color now would be taking the both the emission and diffuse into account

• If either diffuse or emission is a texture, leave the viewport color as the default (grayish)
• Otherwise, set the viewport color RGB to (emission + diffuse). This matches the apparent color of the Collada surface in full light (when N dot L = 1).

This would be a generalization of the current code that takes only diffuse into account.
(Also the viewport never actually matches the viewport color terribly closely anyway.)

edit: actually the dae/gltf/obj/fbx all set a pure emissive material to a black viewport color, so I guess the dae importer is consistent with all the others.

@Gaia Clary (gaiaclary) Will you review the rest of my Collada patches too?