Skip to content

Conversation

Mugen87
Copy link
Collaborator

@Mugen87 Mugen87 commented Feb 22, 2016

This PR introduces BoxBufferGeometry, the BufferGeometry port of BoxGeometry. I've changed three examples so they directly use BoxBufferGeometry.

BoxGeometry's BufferGeometry port
@WestLangley
Copy link
Collaborator

Nice work!

Unfortunately, materialIndex has been lost from BoxGeometry with this PR, so this pattern will not work:

mesh = new THREE.Mesh( new THREE.BoxGeometry(), new THREE.MultiMaterial( materials ) );

@Mugen87
Copy link
Collaborator Author

Mugen87 commented Feb 22, 2016

@WestLangley Any ideas, how we can add materialIndex to BoxBufferGeometry? To be honest, i've found no obvious/simple solution for this. Maybe..... via groups?

@WestLangley
Copy link
Collaborator

This is a lot of history on that issue. I do not recall what the current status is.

@Mugen87
Copy link
Collaborator Author

Mugen87 commented Feb 22, 2016

Thanks for the link 👍 . I'll try to get an overview...

@Mugen87
Copy link
Collaborator Author

Mugen87 commented Feb 22, 2016

I can manage to add a group for each side of the box, so you can use a MultiMaterial with BoxBufferGeometry. The problem is that the amount of materials in MultiMaterial must exactly match the amount of indices in the geometry, otherwise you get runtime errors in projectObject.

for ( var i = 0, l = groups.length; i < l; i ++ ) {

    var group = groups[ i ];
    var groupMaterial = materials[ group.materialIndex ];

    if ( groupMaterial.visible === true ) { // ERROR: Cannot read property 'visible' of undefined

        pushRenderItem( object, geometry, groupMaterial, _vector3.z, group );

    }

}

Because a box has six sides, BoxBufferGeometry should have six groups and therefore six material indices (0-5). But how can the renderer handle this geometry when the users applies a MultiMaterial with e.g. two materials? Or would this be an invalid setup?

@WestLangley
Copy link
Collaborator

Geometry.fromBufferGeometry() does not set materialIndex properly. Consequently the BoxGeometry constructor in this PR will not work correctly.

I think this can be easily fixed by passing materialIndex as an arg to addFace( a, b, c ) in Geometry.fromBufferGeometry().

/ping @mrdoob

But how can the renderer handle this geometry when the users applies a MultiMaterial with e.g. two materials? Or would this be an invalid setup?

If I recall correctly, that used to work: the materials would repeat. Perhaps not with the current design, though.

@Mugen87
Copy link
Collaborator Author

Mugen87 commented Feb 23, 2016

I've provided a simple jsfiddle with the new code. Removing one of the MeshBasicMaterial from the materials array will cause the mentioned runtime error.

I think this can be easily fixed by passing materialIndex as an arg to addFace( a, b, c ) in Geometry.fromBufferGeometry().

👍 That works great! (change in the jsfiddle the BoxBufferGeometry to BoxGeometry to check)

@Mugen87 Mugen87 closed this Feb 23, 2016
@Mugen87 Mugen87 reopened this Feb 23, 2016
@WestLangley
Copy link
Collaborator

This looks good to me. Thanks!

@mrdoob
Copy link
Owner

mrdoob commented Feb 24, 2016

But how can the renderer handle this geometry when the users applies a MultiMaterial with e.g. two materials? Or would this be an invalid setup?

If I recall correctly, that used to work: the materials would repeat. Perhaps not with the current design, though.

Did they? I think it was always the expectation that the MultiMaterial will have the same amount of materials. But if the user misses one we should be able to handle that.

mrdoob added a commit that referenced this pull request Feb 24, 2016
@mrdoob mrdoob merged commit 7c5b995 into mrdoob:dev Feb 24, 2016
@mrdoob
Copy link
Owner

mrdoob commented Feb 24, 2016

Many thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants