Three.js: Material / MultiMaterial not properly implemented

Created on 19 May 2016 · 20Comments · Source: mrdoob/three.js

Logically speaking, the current implementation does not make much sense.
object.material for single material
object.material.materials[n] for multi material

The correct implementation (used in almost any 3D engine) is the following:
object.material[0] for single material
and
object.material[0]
object.material[1]
...
object.material[n] for multi material

Source

ghost

👍3

All 20 comments

object.material[0] for single material

Yes, I think, it is a good idea.
So for material sorting, you won't need extra condition:
(Now)

if(object.material instanceof THREE.MultiMaterial){
    object.material.materials.forEach(function(material){ 
        foo(material); 
    });
}else{ 
    foo(object.material)
}

(Can be)

object.material.forEach(function(material){ 
    foo(material); 
});
//or
for(var i = 0; i < object.material.length; i++){
    foo(object.material[i];
}

MrMaksymov on 19 May 2016

👍1

The correct implementation (used in almost any 3D engine) is the following:

Can you share links to the relevant page for those other engines?

mrdoob on 20 May 2016

I don't have a ton of experience with different engines, but I can speak specifically for Unreal Engine and Unity - they both utilize a simple array of materials on a mesh - with index 0 being "single", and more indices show up the more material face ids are located on the mesh.

schubboy on 7 Jun 2016

Unreal Engine and Unity - they both utilize a simple array of materials on a mesh - with index 0 being "single"

That's right.
All the 3D engines i know use a similar implementation.

material = shader
object.shader = object.shader[0] - for single material
object.shader[0], object.shader[1], ..., object.shader[n] - for multi material

Again, in Three.js object.material.materials[n] does not make any sense.

ghost on 10 Jun 2016

I can't remember why we decided to use MultiMaterial. But, @mrdumb... This would break backwards compatibility.

I can't think of a way of displaying a warning/error if the user wrote something as basic as this:

mesh.material.color.setHex( 0xff0000 );

That won't work, because now material is an Array.

mrdoob on 14 Jun 2016

How about this pattern?

THREE.Mesh = function ( geometry, material ) {

    THREE.Object3D.call( this );

    this.type = 'Mesh';

    this.geometry = geometry !== undefined ? geometry : new THREE.BufferGeometry();

    if ( material instanceof THREE.Material ) {

        this.materials = [ material ];

    } else if ( material instanceof Array ) {

        this.materials = material;

    } else {

        this.materials = [ new THREE.MeshBasicMaterial( { color: Math.random() * 0xffffff } ) ];

    }

    this.drawMode = THREE.TrianglesDrawMode;

    this.updateMorphTargets();

    Object.defineProperties( this, {

        material: {

            enumerable: true,

            get: function() {

                return this.materials[ 0 ];

            },

            set: function( value ) {

                this.materials[ 0 ] = value;

            }

        }

    } );

};

WestLangley on 14 Jun 2016

That could be an option. Having mesh.material and mesh.materials sounds like it can bring problems though...

We also lose MultiMaterial's .visible but I guess that's ok.

mrdoob on 14 Jun 2016

But, @mrdumb... This would break backwards compatibility.

Glad to hear that is important to you.

I can't think of a way of displaying a warning/error if the user wrote something as basic as this:
mesh.material.color.setHex( 0xff0000 );
That won't work, because now material is an Array.

As I said before, for many 3D engines:
mesh.material = mesh.material[0]
And @WestLangley 's idea is a good one.

ghost on 16 Jun 2016

I guess the check should then be...

if ( material instanceof THREE.MultiMaterial ) {

    // Deprecated

    this.materials = material.materials;

} else if ( material instanceof THREE.Material ) {

    this.materials = [ material ];

} else if ( material instanceof Array ) {

    this.materials = material;

} else {

    this.materials = [ new THREE.MeshBasicMaterial( { color: Math.random() * 0xffffff } ) ];

}

And then we should also add a deprecated message to THREE.MultiMaterial's constructor...

mrdoob on 21 Jun 2016

I guess the check should then be...

It looks good to me.

ghost on 23 Jun 2016

I guess we can close this...

WestLangley on 1 Mar 2017

@WestLangley I think I'll implement your pattern. Otherwise, as seen in #10960, we get Array.isArray() all over the place.

Maybe that's ok though?

mrdoob on 10 Mar 2017

@gero3 what do you about "moving" to geometry.materials and avoid doing the Array.isArray()?

mrdoob on 11 Mar 2017

@mrdoob I can be happy with either my proposal above or your current implementation.

WestLangley on 11 Mar 2017

👍1

@WestLangley I think I'll try your approach tonight and see how it feels.

mrdoob on 11 Mar 2017

Ok, so I gave a it a go... Unfortunately, this is the code that we have for rendering multi materials:

https://github.com/mrdoob/three.js/blob/dev/src/renderers/WebGLRenderer.js#L1365-L1378

With that code, geometry groups mandates how materials are used. Which means that, if we switch to materials only, BoxGeometry could no longer be rendered with a single material because we generate groups for each side by default.

This is how webgl_animation_cloth ends up:

screen shot 2017-03-10 at 22 31 05