-
Notifications
You must be signed in to change notification settings - Fork 136
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
First draft of KHR_materials_diffuse_transmission. #179
base: master
Are you sure you want to change the base?
Conversation
Looks like a model with this extension got added to glTF-Sample-Assets although I'm not sure what this means for the status of the extension (I'm used to the sample repositories only getting extensions after they get merged to glTF repository but the extension PR is still open): https://github.com/KhronosGroup/glTF-Sample-Assets/blob/main/Models/MandarinOrange/glTF/MandarinOrange.gltf |
|
||
// Defaults | ||
cgltf_fill_float_array(out_diff_transmission->diffuse_transmission_color_factor, 3, 1.0f); | ||
out_diff_transmission->diffuse_transmission_factor = 0.f; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit, 0.f will be assigned automatically as we zero out all fields by default
cgltf.h
Outdated
@@ -1911,6 +1921,11 @@ void cgltf_free(cgltf_data* data) | |||
cgltf_free_extensions(data, data->materials[i].iridescence.iridescence_texture.extensions, data->materials[i].iridescence.iridescence_texture.extensions_count); | |||
cgltf_free_extensions(data, data->materials[i].iridescence.iridescence_thickness_texture.extensions, data->materials[i].iridescence.iridescence_thickness_texture.extensions_count); | |||
} | |||
if (data->materials[i].has_diffuse_transmission) | |||
{ | |||
cgltf_free_extensions(data, data->materials[i].diffuse_transmission.diffuse_transmission_texture.extensions, data->materials[i].diffuse_transmission.diffuse_transmission_texture.extensions_count); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These two calls should use cgltf_free_texture_view for now (and the PR needs to be rebased to latest master) to take care of extras. This will keep the code correct for now, although once I get to #118 this will become unnecessary as the code will just get deleted.
Looks like the extension has landed! @abwood fyi as the change needs to be rebased (extensions are gone from texture view) |
# Conflicts: # README.md # cgltf.h # cgltf_write.h
README.md
Outdated
@@ -117,6 +117,8 @@ cgltf also supports some glTF extensions: | |||
- KHR_materials_unlit | |||
- KHR_materials_variants | |||
- KHR_materials_volume | |||
- KHR_materials_iridescence |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: this list was sorted by name I believe.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
whoops, and looks like I duplicated the iridescence listing. Thanks for catching this.
FWIW I've tested this PR in gltfpack (only cgltf.h portion, as I don't use cgltf_write) and it works correctly on two models from glTF-Sample-Assets that already use this extension. |
Adds support for KHR_materials_diffuse_transmission.
Not ready for merge just yet
The specification for this extension is currently marked as "Draft" and is not yet ratified, though it appears to be getting close. Staging this change in case anyone else is looking to take a peak and plug into their own renderers. I've gone so far as to just make sure this compiles and verified a basic model's properties are read from this extension. More testing is needed.