Conversation
|
We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google. ℹ️ Googlers: Go here for more info. |
|
@googlebot I fixed it. |
|
CLAs look good, thanks! ℹ️ Googlers: Go here for more info. |
cdata
left a comment
There was a problem hiding this comment.
This is very exciting 🙏 I hope we get to keep it
| // via onBeforeCompile.toString(), so these two functions do the same thing | ||
| // but look different in order to force a proper recompile. | ||
| const oldOnBeforeCompile = material.onBeforeCompile; | ||
| clone.onBeforeCompile = (material as any).isGLTFSpecularGlossinessMaterial ? |
There was a problem hiding this comment.
Is it possible for both material types to use the the SpecularGlossiness path here?
There was a problem hiding this comment.
Alas no; I tried to describe it in the comment. If the toString() version of onBeforeCompile is the same for the two materials, the second one will not be compiled and will instead try to use the first, which does not work. Took me all morning to diagnose this bug...
There was a problem hiding this comment.
Yah, that's a really funky way to test for shader equivalence.
It would be nice if there was a more hygienic / extensible API for patching Three.js materials. Oh well.
There was a problem hiding this comment.
Indeed, though I'm just glad there's a way to patch Three.js materials at all. The onBeforeCompile method hasn't been around that long.
|
|
||
| const widthDPR = width * dpr; | ||
| const heightDPR = height * dpr; | ||
| context.clearRect(0, 0, widthDPR, heightDPR); |
| // This little hack ignores alpha for opaque materials, in order to comply | ||
| // with the glTF spec. | ||
| if (!clone.alphaTest && !clone.transparent) { | ||
| clone.alphaTest = -0.5; |
There was a problem hiding this comment.
Does -0.5 have meaningful significance?
There was a problem hiding this comment.
No, any value less than zero has the same behavior.
|
@elalish @nicolas-daures |
Transparent rendering (google#975) * updated three to r113 * fixes after pulling latest three * fixed extensions * removing special treatment of specularGlossiness * further cleanup * fixed transparent rendering * pulling example * removed background-color attribute * updated tests * removed background-color from docs * updated package-lock * updating package-lock * addressing feedback * merging r113 * updated package-lock * fixed specGloss patching * fixed specGloss more * fixing transparency test
Fixes #874
Fixes #492
Fixes #182
This a possible alternative approach to #949, to avoid a second renderer, as I'm concerned about how that will complicate some upcoming performance improvement work. This pulls ideas from #949 (thank you @nicolas-daures!), but makes the singleton renderer transparent and instead fixes #381 directly.
This also does away with our background-color attribute, since with a properly transparent rendering context, the standard CSS background-color property works as expected. Also, is it just me or does this render faster?