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

Update PBR clear coat and sheen to latest spec #9004

Merged
merged 21 commits into from Oct 1, 2020
Merged

Update PBR clear coat and sheen to latest spec #9004

merged 21 commits into from Oct 1, 2020

Conversation

Popov72
Copy link
Contributor

@Popov72 Popov72 commented Sep 19, 2020

Fix #9001.

@sebavan
Copy link
Member

sebavan commented Sep 21, 2020

It looks like there might be a compilation issue in Webgl1

@Popov72
Copy link
Contributor Author

Popov72 commented Sep 21, 2020

Fixed

@bghgary
Copy link
Contributor

bghgary commented Sep 21, 2020

I guess we don't have any validation tests for clearcoat or sheen?

@bghgary
Copy link
Contributor

bghgary commented Sep 21, 2020

I'll work on updating the sample model.

@bghgary
Copy link
Contributor

bghgary commented Sep 21, 2020

It's updated here: KhronosGroup/glTF-Sample-Models#266

@Popov72
Copy link
Contributor Author

Popov72 commented Sep 21, 2020

I guess we don't have any validation tests for clearcoat or sheen?

We have:

Those PGs are activating clearcoat and sheen.

Notably, the last 3 are testing different settings of the clearcoat and sheen modules.

Do you want me to add a validation test with the sheen asset?

@bghgary
Copy link
Contributor

bghgary commented Sep 21, 2020

Do you want me to add a validation test with the sheen asset?

Are the PGs you mentioned not already in the validation tests? If not, then yes, I think we should add one. Specifically a glTF sample model.

@Popov72
Copy link
Contributor Author

Popov72 commented Sep 21, 2020

Yes, the PGs I have linked are already in the validation test list.

@bghgary
Copy link
Contributor

bghgary commented Sep 21, 2020

Yes, the PGs I have linked are already in the validation test list.

I mean glTF models with clearcoat and sheen, not just clearcoat and sheen directly.

@Popov72
Copy link
Contributor Author

Popov72 commented Sep 21, 2020

Yes, the PGs I have linked are already in the validation test list.

I mean glTF models with clearcoat and sheen, not just clearcoat and sheen directly.

Ok, then no there isn't. Have you a model for the clearcoat validation test?

@bghgary
Copy link
Contributor

bghgary commented Sep 21, 2020

@Popov72
Copy link
Contributor Author

Popov72 commented Sep 22, 2020

Code is updated, waiting for BabylonJS/Assets#16 to be merged before adding validation tests.

@deltakosh
Copy link
Contributor

Ok Gary has a feedback on the assets so waiting to merge it

@deltakosh
Copy link
Contributor

we do have some conflicts to fix (I merged the assets)

@Popov72
Copy link
Contributor Author

Popov72 commented Sep 22, 2020

Fixed and validation tests added.

@deltakosh
Copy link
Contributor

it fails :(

@Popov72
Copy link
Contributor Author

Popov72 commented Sep 23, 2020

There is a 3mn timeout on the clear coat test, but it's exactly the same than for sheen which works... Only the asset is different, but it does work here: https://playground.babylonjs.com/#YG3BBF#1

@deltakosh
Copy link
Contributor

@sebavan can we increase the timeout?

@Popov72
Copy link
Contributor Author

Popov72 commented Sep 23, 2020

In fact the asset is quite small for clear coat: only 233 ko...

And it is bigger for sheen: 4.86Mo in all.

clear coat asset is glb whereas it is gltf for sheen...

@Popov72
Copy link
Contributor Author

Popov72 commented Sep 23, 2020

There was a conflict with the clear coat ts file, let's see how it goes now.

@Popov72
Copy link
Contributor Author

Popov72 commented Sep 23, 2020

Still does not work, trying another PG...

@Popov72
Copy link
Contributor Author

Popov72 commented Sep 24, 2020

Well, it seems the dev server does not like the ClearCoatTest.glb asset, I can't make it to work...

@deltakosh
Copy link
Contributor

Make sure there is no upper caps / lower caps issues ?

@Popov72
Copy link
Contributor Author

Popov72 commented Sep 24, 2020

It is working when using it in the PG: https://playground.babylonjs.com/#YG3BBF#1

Or https://playground.babylonjs.com/#YG3BBF#3, which is the one I tried in the latest commit.

@deltakosh
Copy link
Contributor

can we try with the gltf version?

@Popov72
Copy link
Contributor Author

Popov72 commented Sep 24, 2020

Waiting for BabylonJS/Assets#17 to be in before changing the test.

@Popov72
Copy link
Contributor Author

Popov72 commented Sep 24, 2020

Same problem, timeout...

@deltakosh
Copy link
Contributor

deltakosh commented Sep 25, 2020

There is something wrong here adding @sebavan to have his thougths

@deltakosh
Copy link
Contributor

up?

@sebavan
Copy link
Member

sebavan commented Oct 1, 2020

Still need to look into the tests should be able to do it tomorrow night.

@Popov72
Copy link
Contributor Author

Popov72 commented Oct 1, 2020

I have excluded the clearcoat unit test from automatic testing so that the PR can be merged: @sebavan is currently looking at the problem in another PR.

@deltakosh deltakosh merged commit 6c22914 into BabylonJS:master Oct 1, 2020
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.

Update PBR sheen and clearcoat to the latest Khronos Spec
4 participants