Add example model for KHR_materials_transmission #265
Conversation
|
To be consistent with other samples, it is better to name the sample model file the same as the folder name is a good idea.
|
@cx20 Good point. Done. |
|
I tried adding this model to gltf-test as a trial.
So far, only Babylon.js seems to support this extension. BTW, it may be a matter of taste, but I prefer the model to be in the center of the Scene, as in the ClearCoatTest model. |
…e requirement of texture transform extension
|
@lexaknyazev It looks like CI didn't run here. Any ideas? |
|
@lexaknyazev Correction, CI is running, just not showing up as green checkmarks or red X's on this PR. It's not warning me not to merge the PR with an error in it. https://travis-ci.org/github/KhronosGroup/glTF-Sample-Models/builds |
|
GitHub isn't showing it, but validation has passed: https://travis-ci.org/github/KhronosGroup/glTF-Sample-Models/builds/702172431 |
There was a problem hiding this comment.
This sample model looks good to me. We'll hold off merging until KhronosGroup/glTF#1698 is merged.
|
I tried displaying this model in Babylon.js Sandbox again. However, if you apply the glTF's Previous model result: Current model result: I would suggest removing the built-in camera or adjusting it to fit in the frame. |
|
Great model! After taking a close look, I've got a few questions regarding the sample. I noticed that in cases where a transmission texture is used (e.g., RedTransTexture), we specify a transmission texture but transmission factor is not specified. Does it need to be? The spec says that transmissionTexture and factor are to be combined, but transmissionFactor defaults to 0, resulting in no transmission. "GreenMask" material is using alphaMode="Blend". Should alphaMode be set to Mask? The texture is indeed a masking texture. There are possibly other materials that could be set to Mask rather than blend. The more of these spheres that render in the opaque pass, the better. A final minor comment is that all meshes are named "Sphere". For debugging purposes it would be helpful if these had unique names. |
|
Thanks for catching that. I've updated |
|
In several renderers I'm seeing some issue that looks like imprecision in the alpha (UVs too close to a texture boundary?).
The screenshot above is from three.js, which doesn't support the transmission extension yet, but the same thing appears in BabylonJS, which does. |
|
@donmccurdy I should double-check, but that error looks very, very familiar to me. It's what happens when you have a double-sided sphere, with BLEND enabled and the z-buffer writes disabled, with no depth sorting. I even have a hacky workaround for such models: Make two co-located single-sided spheres, as separate primitives. Either the inside or the outside will be consistently "on top", which looks better than the bands even if the order comes out wrong. But I don't know if such hacky workarounds belong in official sample models, probably they don't. |
|
Yes, in fact I implemented something very similar in <model-viewer> to
handle transparency better:
https://github.com/google/model-viewer/blob/ec2057513beb40a3c6ea9c2be0ea66d3065f4990/packages/model-viewer/src/three-components/gltf-instance/ModelViewerGLTFInstance.ts#L90,
which is based on a three.js example. My point here is I don't think this
adjustment needs to be part of the glTF; it can be applied in the
rendering engine as appropriate.
…-Emmett
On Mon, Aug 3, 2020 at 10:29 AM Ed Mackey ***@***.***> wrote:
@donmccurdy <https://github.com/donmccurdy> I should double-check, but
that error looks very, very familiar to me. It's what happens when you have
a double-sided sphere, with BLEND enabled and the z-buffer writes disabled,
with no depth sorting.
I even have a hacky workaround for such models: Make two co-located
single-sided spheres, as separate primitives. Either the inside or the
outside will be consistently "on top", which looks better than the bands
even if the order comes out wrong. But I don't know if such hacky
workarounds belong in official sample models, probably they don't.
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
<#265 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAMS2LF5CFVMYBKN7TU62PDR63XWJANCNFSM4OACQS3A>
.
|
|
@emackey Nice guess, you're correct that the model is using
^table does not include |
|
Yes, I can confirm this is an issue with BLEND mode, and the "easiest" way to fix this is to render doublesided transmission objects in two passes, backfaces then front faces. I raised the BLEND vs MASK point earlier in this PR, but the choice of BLEND mode is intentional and likely helped us uncover a bug in another engine as a result anyway. I think many of these spheres look better as doublesided anyway, as these patterns on the backside should be visible through the spheres. |
|
Yes, double-sided is very intensional as these are thin-shells where you can see the inside surface. |
|
From the proposed spec:
We don't typically expect transmissive or non-opaque materials to appear behind transmissive materials, do we? I understand that rendering doublesided transmission in two passes is useful here, but I worry that this model is testing many things not required in the spec:
(1) and (3) are fine, I'm wondering about (2) and (4). I'd rather not mix an alpha map with the sample, no. But should some of this be spelled out in the spec? Do we expect all clients to split doubleSided materials into two passes, or is that case-by-case? |
I might still prefer that this use MASK, rather than BLEND. It seems better for this example to demonstrate best practice than to exercise all the edge cases, and if we're deviating from best practice in order to show an edge case we should be very clear about that in the README. But I think it would be easier to exercise those combinations in a separate example more like https://github.com/KhronosGroup/glTF-Sample-Models/tree/master/2.0/AlphaBlendModeTest, later. |
…mission_test_model # Conflicts: # 2.0/README.md
|
@donmccurdy I'm not sure BLEND vs MASK makes much difference to this model, although I certainly agree we should have a separate test model to break out specific edge cases. Off-topic, but not everyone on the Filament team is on board with the way we did MASK, and there are separate open issues on that. Can this sample go in as-is? |
That's almost the point — in a world where all engines support BLEND triangle sorting perfectly, you'd practically never need MASK, BLEND would always work fine. But implementation is not that simple, and so if you could use either, you should choose MASK. I can't see any reason to use BLEND here, except to make implementation more difficult. I don't think that is the right goal for our sample models, which should aim to show best practice and be very clear about the reason when they do not. We deal with three.js users (very regularly) having trouble with models using BLEND that could be using OPAQUE or MASK, and I don't want to encourage that as the default for exporters with the new transmission materials. 😕 More generally: if alpha-as-coverage does not need semitransparency, the blend mode should be "MASK" and not "BLEND", as if the transmission property were not involved at all. |
|
@donmccurdy Good points here. @MiiBond You did say in an earlier comment here that the use of BLEND is intentional, but BLEND is known to be problematic when multiple polygons overlap, such as on a double-sided sphere, even in the absence of transmission. I think we need to test transmission with BLEND on geometry that doesn't overlap, which would mean a different test model than this. In light of that, would it be OK to switch this model to use MASK? |
|
Hi. Sorry, I've been neglecting this PR. Yeah, MASK is definitely more appropriate. I usually export with BLEND because it gives nicer edges and Dimension's web viewer does OIT. |
|
The following is the comparison result between the old and new models. The screenshot is a link to a sample. I can't determine if this is an expected result, but Babylon.js does show an improvement.
|
|
Thanks @MiiBond! Here's a summary of the materials in the latest draft:
The only thing I might consider changing would be to replace the emissive "LabelMaterial" with an unlit material — it will behave better in path tracers or in rasterizers that use bloom effects. I'm not as concerned about that, so it's fine with me to merge this! |
|
Re: unlit material. Are we okay including another extension in this sample model? I thought we were trying to avoid that. |
|
Yeah, I think it's cleaner if we leave unlit out, so we're only testing the one extension here. |
|
Ok, works for me. 👍 |
No description provided.