-
-
Notifications
You must be signed in to change notification settings - Fork 35.6k
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
GLTFLoader: Implement KHR_materials_clearcoat. #18682
GLTFLoader: Implement KHR_materials_clearcoat. #18682
Conversation
src/renderers/shaders/ShaderChunk/lights_physical_fragment.glsl.js
Outdated
Show resolved
Hide resolved
Successfully rendering @emackey's excellent test asset now: |
Moved the core changes to a new PR, #18769. Will keep the GLTFLoader portion here, to check in afterward. |
Do you plan to implement GLTFLoader clearcoat extension on top of the new plugin system #18484? Or do you first go without it? |
I'm willing to do either. The new plugin API is a big change, though, and I'm having trouble deciding how quickly we should move forward on that. |
ad10ad6
to
ae64d23
Compare
I see, thanks. Regarding the new plugin API, let's keep discussing in #18484 |
@takahirox if I polish up this PR this week, would you be OK with merging it before the plugin API? |
That's fine because I'm thinking to create a new PR so I don't care if this change can make the conflicts with the existing plugin PR. Then I'll make a branch after this PR gets in. |
ae64d23
to
a7c8e06
Compare
Um, it seems the URL of the new asset (Clearcoat Test) is not correct since I get a 404. |
@donmccurdy Looks like a case issue. In your branch, rename the model and its folder from |
Fixed ✅ |
@donmccurdy feel free to merge this when you think it's ready 👌 |
Clearcoat: without (left) and with (right).
To do:
Extension may need to be read from the material def, not the pbrSpecularGlossiness sub-object.Test against more cases, e.g. Clearcoat feature-test model KhronosGroup/glTF-Sample-Models#251.