Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

3D Tiles: Make KHR_Materials_Common work with batch tables and CESIUM_RTC extensions #4684

Merged

Conversation

JDepooter
Copy link
Contributor

Addresses the problem described in #4669

I've updated the code which generates shaders for the KHR_Materials_Common extension. The update addresses two related issues:

  • When a batch table is used, a batch id attribute and parameter are needed in the generated vertex shader.
  • When the CESIUM_RTC extension is used, the generated shader should use the CESIUM_RTC_MODELVIEW semantic for the model view matrix. This change also fixes the shader generation for glTF models loaded via the Model::fromGltf() function.

One thing I'm not certain is correct is the type of the BATCHID shader parameter. I've set it to WebGLConstants.UNSIGNED_SHORT, based on this link. Could anybody comment whether this needs to be adjusted based on the actual data type of the batch id values in the glTF buffer?

Copy link
Contributor

@lilleyse lilleyse left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These changes look great! I'll throw together a sample tileset soon for testing.

@@ -349,6 +349,16 @@ define([
vertexShader += 'attribute vec4 a_joint;\n';
vertexShader += 'attribute vec4 a_weight;\n';
}

if (options.addBatchIdToGeneratedShaders)
{
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Format as:
if (options.addBatchIdToGeneratedShaders) {

techniqueAttributes.a_batchId = 'batchId';
techniqueParameters.batchId = {
semantic: 'BATCHID',
type: WebGLConstants.UNSIGNED_SHORT
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could anybody comment whether this needs to be adjusted based on the actual data type of the batch id values in the glTF buffer?

Yes this will need to be adjusted based on the actual type in the buffer. The spec may need to be clearer in saying that the batch id can be any type.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Quick question @lexaknyazev @pjcozzi

Does a parameter's type equal the data type stored in the buffer or the type declared in the shader? If batchId is stored as unsigned shorts, should type be 5123 or 5126.

"technique": {
    "attributes": {
        "a_batchId": "batchId"
    },
    "parameters": {
        "batchId": {
            "semantic": "BATCHID",
            "type": 5123
        }
    }
}

Shader

attribute float a_batchId;

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @lexaknyazev for the update.

@JDepooter the type can be WebGLConstants.FLOAT. I also updated the 3D Tiles spec: CesiumGS/3d-tiles#154

@JDepooter JDepooter force-pushed the khr_materials_common_with_batch_id branch from 4fe0b98 to 809ec54 Compare December 2, 2016 20:11
@JDepooter
Copy link
Contributor Author

@lilleyse I've updated the formatting as requested.

With respect to the type of the BATCHID parameter, if the parameter type must match the data type stored in the buffer, there could be situations where multiple shaders would need to be generated per material. If two primitives use the same material, but have different batch id types, they would need different shaders.

@lilleyse
Copy link
Contributor

lilleyse commented Dec 2, 2016

Yeah, I'm hoping the answer is that the type should actually just be 5126 for float, but am waiting on a response to that.

@lilleyse
Copy link
Contributor

lilleyse commented Dec 2, 2016

Not quite final, but the following tileset works on this branch:
BatchedWithKHRMaterialsCommon.zip

@JDepooter JDepooter force-pushed the khr_materials_common_with_batch_id branch from 809ec54 to dff7931 Compare December 5, 2016 19:19
@JDepooter
Copy link
Contributor Author

@lilleyse I've updated the type to WebGLConstants.FLOAT

@pjcozzi
Copy link
Contributor

pjcozzi commented Dec 5, 2016

Thanks again for the contribution, @JDepooter!

@lilleyse we have a CLA so please merge when ready.

@lilleyse
Copy link
Contributor

lilleyse commented Dec 6, 2016

I added a simple test with the sample tileset I linked.

Thanks @JDepooter.

@lilleyse lilleyse merged commit b40394b into CesiumGS:3d-tiles Dec 6, 2016
@JDepooter JDepooter deleted the khr_materials_common_with_batch_id branch December 7, 2016 18:56
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