Support vertex colors for materialsCommon and pbrMetallicRoughness #6089
Conversation
|
@lilleyse, thanks for the pull request! Maintainers, we have a signed CLA from @lilleyse, so you can review this at any time.
I am a bot who helps you make Cesium awesome! Contributions to my configuration are welcome. 🌍 🌎 🌏 |
|
@emackey any interest in reviewing this? |
| ForEach.meshPrimitive(mesh, function(primitive) { | ||
| ForEach.meshPrimitiveAttribute(primitive, function(attribute, semantic) { | ||
| if (semantic.indexOf('COLOR') === 0) { | ||
| hasVertexColors = true; |
There was a problem hiding this comment.
Individual materials are allowed to have, or not have, vertex colors. This branch appears to apply hasVertexColors globally to a model, so if one material is using them, the others go dark (by multiplying by zero apparently, in my testing).
Haven't tried this, but could you make the default color white, so if hasVertexColors is true but a particular material doesn't have COLOR_0, it acts as if it had white vertices instead of black?
|
Updated:
|
|
I merged |
| @@ -52,12 +52,15 @@ define([ | |||
| }; | |||
| }; | |||
|
|
|||
| ModelUtility.getAttributeOrUniformBySemantic = function(gltf, semantic) { | |||
| ModelUtility.getAttributeOrUniformBySemantic = function(gltf, semantic, programId) { | |||
There was a problem hiding this comment.
Am I missing something here? I don't see parameter semantic being used in this function.
There was a problem hiding this comment.
It's being used: defined(parameter) && parameter.semantic === semantic
|
@bagnell can you review the 3D Tiles aspects of this? |
|
This looks good to me. @emackey, is this good to merge? |
|
Tests pass on my local box, but Travis has a failure. Still OK to merge? |
|
Yep, the test failure is an unrelated timeout from an imagery provider. Merging. |
Adds support for rendering vertex colors on models using the
COLOR_0vertex attribute. Includes gltf-pipeline changes from CesiumGS/gltf-pipeline#345.Besides just rounding out our gltf implemenation this was added so that we can introduce a proper test case for #5874.
TODO: