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

[WIP] PBR Materials extension #643

Closed
wants to merge 44 commits into
base: master
from

Conversation

Projects
None yet
@mlimper
Contributor

mlimper commented Jul 8, 2016

This is our (currently WIP) PBR material extension - this PR is currently mainly intended to serve as a place for discussion.

Direct Preview:
https://github.com/tsturm/glTF/tree/master/extensions/Vendor/FRAUNHOFER_materials_pbr

@pjcozzi

This comment has been minimized.

Show comment
Hide comment
@pjcozzi

pjcozzi Jul 9, 2016

Member

@erich666 would you or any of your colleagues be interested in reviewing this proposed PBR materials extension to glTF?

Member

pjcozzi commented Jul 9, 2016

@erich666 would you or any of your colleagues be interested in reviewing this proposed PBR materials extension to glTF?

### Specular - Glossiness
<img src="figures/specular_glossiness_2.png" align="middle" height="220" style="display: block; margin: 0 auto; padding: 20px 0 10px 0;">

This comment has been minimized.

@pjcozzi

pjcozzi Jul 9, 2016

Member

Nice figure. Did you create it? If not, do we need permission to use it here?

@pjcozzi

pjcozzi Jul 9, 2016

Member

Nice figure. Did you create it? If not, do we need permission to use it here?

This comment has been minimized.

@pjcozzi

pjcozzi Jul 9, 2016

Member

Same comment below, of course.

@pjcozzi

pjcozzi Jul 9, 2016

Member

Same comment below, of course.

This comment has been minimized.

@pjcozzi

pjcozzi Jul 9, 2016

Member

Same comments for the equation figures in Appendix.md.

@pjcozzi

pjcozzi Jul 9, 2016

Member

Same comments for the equation figures in Appendix.md.

This comment has been minimized.

@mlimper

mlimper Sep 1, 2016

Contributor

Yes, we created those figures (also for the equations). We used them in the respective ACM Web3D paper, so we should double-check that it is OK to use them for non-commercial purposes. I would guess so, but I am not 100% sure, so: good point, we need to check (or simply create replacements specifically for this extension).

@mlimper

mlimper Sep 1, 2016

Contributor

Yes, we created those figures (also for the equations). We used them in the respective ACM Web3D paper, so we should double-check that it is OK to use them for non-commercial purposes. I would guess so, but I am not 100% sure, so: good point, we need to check (or simply create replacements specifically for this extension).

@pjcozzi pjcozzi referenced this pull request Jul 9, 2016

Closed

PBR, Streaming and LOD #596

<strong>Usage Example:</strong>
```javascript

This comment has been minimized.

@pjcozzi

pjcozzi Jul 9, 2016

Member

Same comments as above for this example.

@pjcozzi

pjcozzi Jul 9, 2016

Member

Same comments as above for this example.

An introduction to PBR concepts is provided in the [appendix](Appendix.md).
## Known Implementations

This comment has been minimized.

@pjcozzi

pjcozzi Jul 9, 2016

Member

Would you consider contributing a stage to gltf-pipeline to convert a glTF model with PBR materials using this extension to a glTF model with generated materials/techniques/shaders?

This would:

  • Create a low barrier-to-entry for users to get started with this extension
  • Serve as a reference implementation for those implementing the extension (JavaScript developers could probably use parts of it as is)
  • Help ensure consistent implementations of this extension across engines
@pjcozzi

pjcozzi Jul 9, 2016

Member

Would you consider contributing a stage to gltf-pipeline to convert a glTF model with PBR materials using this extension to a glTF model with generated materials/techniques/shaders?

This would:

  • Create a low barrier-to-entry for users to get started with this extension
  • Serve as a reference implementation for those implementing the extension (JavaScript developers could probably use parts of it as is)
  • Help ensure consistent implementations of this extension across engines
@pjcozzi

This comment has been minimized.

Show comment
Hide comment
@pjcozzi

pjcozzi Jan 11, 2017

Member

One of my students, @jian-ru, added support for the current version of this extension to his Vulkan engine: https://github.com/jian-ru/laugh_engine#gltf-support

Member

pjcozzi commented Jan 11, 2017

One of my students, @jian-ru, added support for the current version of this extension to his Vulkan engine: https://github.com/jian-ru/laugh_engine#gltf-support

@pjcozzi

This comment has been minimized.

Show comment
Hide comment
@pjcozzi

pjcozzi Jan 11, 2017

Member

Also WIP WebGL glTF PBR reference implementation (raw WebGL, no engine) by @moneimne is here: http://www.seas.upenn.edu/~moneimne/WebGL-PBR/

Repo: https://github.com/moneimne/WebGL-PBR

Member

pjcozzi commented Jan 11, 2017

Also WIP WebGL glTF PBR reference implementation (raw WebGL, no engine) by @moneimne is here: http://www.seas.upenn.edu/~moneimne/WebGL-PBR/

Repo: https://github.com/moneimne/WebGL-PBR

@pjcozzi pjcozzi added the 2.0 label Jan 13, 2017

@pjcozzi pjcozzi referenced this pull request Jan 17, 2017

Closed

glTF 1.1 will be 2.0! #817

4 of 5 tasks complete
@AurL

This comment has been minimized.

Show comment
Hide comment
@AurL

AurL Jan 18, 2017

From #817

We have also decided that the metallic-roughness PBR material model will be in the core glTF spec (specular-glossiness will remain in the extension)

Is there any material schema example that shows the diff between a metal-roughness material vs a specular-glossiness one ?

It seems to be a small change regarding the schema but as I am working on an exporter using PBR, I would like to be sure that it's done well. From what I understand, only the specular-glossiness material description would be under an EXT_materials_PBR extension ?

AurL commented Jan 18, 2017

From #817

We have also decided that the metallic-roughness PBR material model will be in the core glTF spec (specular-glossiness will remain in the extension)

Is there any material schema example that shows the diff between a metal-roughness material vs a specular-glossiness one ?

It seems to be a small change regarding the schema but as I am working on an exporter using PBR, I would like to be sure that it's done well. From what I understand, only the specular-glossiness material description would be under an EXT_materials_PBR extension ?

@pjcozzi

This comment has been minimized.

Show comment
Hide comment
@pjcozzi

pjcozzi Jan 18, 2017

Member

@AurL the JSON schemas are here:

@mlimper other than moving metallic-roughness into core spec, are these fully up-to-date?

Member

pjcozzi commented Jan 18, 2017

@AurL the JSON schemas are here:

@mlimper other than moving metallic-roughness into core spec, are these fully up-to-date?

@mlimper

This comment has been minimized.

Show comment
Hide comment
@mlimper

mlimper Jan 18, 2017

Contributor

other than moving metallic-roughness into core spec, are these fully up-to-date?

Three points coming to my mind right now:

  • @sbtron, @bghgary and colleagues suggested to make some additional comments, such as specifying priorities for the different maps, which should help a client that does not support the required number of channels.

  • There should be the possibility to use an F0 texture, to specify F0 values for non-metallic parts. That case is a bit special, but we should cover it.

  • I'll provide a separate document for additional maps (normals, occlusion), and I would like to link to that one in both specs, materials_common and materials_pbr.

The last point will take some more days, but the first two points can be done relatively quickly.

@bghgary Would it be possible for you and your colleague (Sebastien, I believe) to specify the prioritization of the different maps as we discussed it and open a PR for that?

Contributor

mlimper commented Jan 18, 2017

other than moving metallic-roughness into core spec, are these fully up-to-date?

Three points coming to my mind right now:

  • @sbtron, @bghgary and colleagues suggested to make some additional comments, such as specifying priorities for the different maps, which should help a client that does not support the required number of channels.

  • There should be the possibility to use an F0 texture, to specify F0 values for non-metallic parts. That case is a bit special, but we should cover it.

  • I'll provide a separate document for additional maps (normals, occlusion), and I would like to link to that one in both specs, materials_common and materials_pbr.

The last point will take some more days, but the first two points can be done relatively quickly.

@bghgary Would it be possible for you and your colleague (Sebastien, I believe) to specify the prioritization of the different maps as we discussed it and open a PR for that?

@pjcozzi

This comment has been minimized.

Show comment
Hide comment
@pjcozzi

pjcozzi Jan 20, 2017

Member

If there is anything @moneimne can do to help finish the spec/extension, please let us know.

Member

pjcozzi commented Jan 20, 2017

If there is anything @moneimne can do to help finish the spec/extension, please let us know.

@bghgary

This comment has been minimized.

Show comment
Hide comment
@bghgary

bghgary Jan 20, 2017

Contributor

Yes, though for the PR, I would expect this information to go with the separate document for additional maps that you mentioned. I can wait until you are done or you can just incorporate this into your document directly.

List of additional maps in priority order:

Map What happens if it’s not there
Tangent Space Normal Geometry will appear less detailed than authored
Ambient Occlusion Model will appear brighter in areas that should be darker
Emission Model with lights will not be lit (e.g. the headlights of a car model will be off instead of on)
F0 (for dielectrics) Some non-metallic materials, such as ruby and diamond, will be less shiny than it’s supposed to be

Resource-bound implementations should drop maps from the bottom to the top.

Contributor

bghgary commented Jan 20, 2017

Yes, though for the PR, I would expect this information to go with the separate document for additional maps that you mentioned. I can wait until you are done or you can just incorporate this into your document directly.

List of additional maps in priority order:

Map What happens if it’s not there
Tangent Space Normal Geometry will appear less detailed than authored
Ambient Occlusion Model will appear brighter in areas that should be darker
Emission Model with lights will not be lit (e.g. the headlights of a car model will be off instead of on)
F0 (for dielectrics) Some non-metallic materials, such as ruby and diamond, will be less shiny than it’s supposed to be

Resource-bound implementations should drop maps from the bottom to the top.

@mlimper

This comment has been minimized.

Show comment
Hide comment
@mlimper

mlimper Jan 20, 2017

Contributor

Yes, though for the PR, I would expect this information to go with the separate document for additional maps that you mentioned. I can wait until you are done or you can just incorporate this into your document directly.

Yes, I'll provide that one. Please feel free to open the PR and already incorporate the table into the spec right now.

Contributor

mlimper commented Jan 20, 2017

Yes, though for the PR, I would expect this information to go with the separate document for additional maps that you mentioned. I can wait until you are done or you can just incorporate this into your document directly.

Yes, I'll provide that one. Please feel free to open the PR and already incorporate the table into the spec right now.

@javagl

This comment has been minimized.

Show comment
Hide comment
@javagl

javagl Jan 24, 2017

Contributor

@selim-bekkar-sb mentioned in #643 (comment)

Why do you force to merge the Specular and Glossiness and or the Metallic and Roughness textures ?

The answer by @pjcozzi in #643 (comment) was:

The spirit of glTF is that it is easy and fast for an engine to load and render; it does not need to match how a modeling tool wants to store data since it is a "final stage" format that is intended for engines, not modeling tools.

Is it safe to assume that combining these textures will be enforced?

I'm asking because, for example, the "Helmet" test model and the work in https://github.com/moneimne/WebGL-PBR in general seem to operate on separate textures. I'd like to create a "baseline" version (not an "official reference" - only for personal use - but to be published at https://github.com/javagl/gltfTestModels/tree/master/DamagedHelmetModified ), and now assume that I'd create a combined version of the respective textures.

Contributor

javagl commented Jan 24, 2017

@selim-bekkar-sb mentioned in #643 (comment)

Why do you force to merge the Specular and Glossiness and or the Metallic and Roughness textures ?

The answer by @pjcozzi in #643 (comment) was:

The spirit of glTF is that it is easy and fast for an engine to load and render; it does not need to match how a modeling tool wants to store data since it is a "final stage" format that is intended for engines, not modeling tools.

Is it safe to assume that combining these textures will be enforced?

I'm asking because, for example, the "Helmet" test model and the work in https://github.com/moneimne/WebGL-PBR in general seem to operate on separate textures. I'd like to create a "baseline" version (not an "official reference" - only for personal use - but to be published at https://github.com/javagl/gltfTestModels/tree/master/DamagedHelmetModified ), and now assume that I'd create a combined version of the respective textures.

@mlimper

This comment has been minimized.

Show comment
Hide comment
@mlimper

mlimper Jan 24, 2017

Contributor

Is it safe to assume that combining these textures will be enforced?

Yes, and also agreed with the comment of @pjcozzi

Imagine we would allow both, we would have the risk that each combination "exporter/renderer" implements only one of the possible ways, which would lead to assets not being fully interchangeable across engines

@moneimne
you probably used the separate textures as an intermediate step towards the final version of the demo, I guess?

Contributor

mlimper commented Jan 24, 2017

Is it safe to assume that combining these textures will be enforced?

Yes, and also agreed with the comment of @pjcozzi

Imagine we would allow both, we would have the risk that each combination "exporter/renderer" implements only one of the possible ways, which would lead to assets not being fully interchangeable across engines

@moneimne
you probably used the separate textures as an intermediate step towards the final version of the demo, I guess?

@javagl

This comment has been minimized.

Show comment
Hide comment
@javagl
Contributor

javagl commented Jan 24, 2017

@moneimne

This comment has been minimized.

Show comment
Hide comment
@moneimne

moneimne Jan 24, 2017

you probably used the separate textures as an intermediate step towards the final version of the demo, I guess?

Yes, I have been using the Helmet test model as is for now, but I can definitely use the combined version of the model once the changes have been made.

Thank you for the combined texture, @javagl! Do you also plan on updating the glTF file to reflect this change?

moneimne commented Jan 24, 2017

you probably used the separate textures as an intermediate step towards the final version of the demo, I guess?

Yes, I have been using the Helmet test model as is for now, but I can definitely use the combined version of the model once the changes have been made.

Thank you for the combined texture, @javagl! Do you also plan on updating the glTF file to reflect this change?

@javagl

This comment has been minimized.

Show comment
Hide comment
@javagl

javagl Jan 24, 2017

Contributor

Sure. I just started working on all that, and intended to create a shader implementation that implements the spec as closely as possible. I started reading the Frostbite PBR document, but it's difficult to flesh out the relevant parts here. E.g. the spec does not cover normals/bump maps or ambient occulsion, and I still have to figure out how they are integrated eventually (there are some open issues for the discussion about these points).

In the meantime, I have updated the glTF and did some minor updates to the shaders, to use the combined metallic+roughness texture, and to take the factors (base color, metallic, roughness) into account: javagl/gltfTestModels@6682793

Contributor

javagl commented Jan 24, 2017

Sure. I just started working on all that, and intended to create a shader implementation that implements the spec as closely as possible. I started reading the Frostbite PBR document, but it's difficult to flesh out the relevant parts here. E.g. the spec does not cover normals/bump maps or ambient occulsion, and I still have to figure out how they are integrated eventually (there are some open issues for the discussion about these points).

In the meantime, I have updated the glTF and did some minor updates to the shaders, to use the combined metallic+roughness texture, and to take the factors (base color, metallic, roughness) into account: javagl/gltfTestModels@6682793

@pjcozzi

This comment has been minimized.

Show comment
Hide comment
@pjcozzi

pjcozzi Feb 4, 2017

Member

Metal Roughness is being added to the core glTF 2.0 spec in #830.

Member

pjcozzi commented Feb 4, 2017

Metal Roughness is being added to the core glTF 2.0 spec in #830.

@tparisi

This comment has been minimized.

Show comment
Hide comment
@tparisi

tparisi Feb 22, 2017

Contributor

@pjcozzi can we have a succinct recap of the logic behind putting specular-glossiness into an extensions? @cedricpinson how does the decision relate to Sketchfab's PBR model out of curiosity?

Contributor

tparisi commented Feb 22, 2017

@pjcozzi can we have a succinct recap of the logic behind putting specular-glossiness into an extensions? @cedricpinson how does the decision relate to Sketchfab's PBR model out of curiosity?

@pjcozzi

This comment has been minimized.

Show comment
Hide comment
@pjcozzi

pjcozzi Feb 22, 2017

Member

@tparisi this was discussed at length with broad community input, see #696 (comment)

Member

pjcozzi commented Feb 22, 2017

@tparisi this was discussed at length with broad community input, see #696 (comment)

@cedricpinson

This comment has been minimized.

Show comment
Hide comment
@cedricpinson

cedricpinson Feb 26, 2017

@tparisi well nothing special we adapted our implementation to fit the extension. I would have been happy to have this in the core, but it does not really change a lot. I understand that simplicity with metalness,roughness is good for everybody.

cedricpinson commented Feb 26, 2017

@tparisi well nothing special we adapted our implementation to fit the extension. I would have been happy to have this in the core, but it does not really change a lot. I understand that simplicity with metalness,roughness is good for everybody.

@lexaknyazev

This comment has been minimized.

Show comment
Hide comment
@lexaknyazev

lexaknyazev Feb 27, 2017

Member

Should we close this PR, or it will be published as glTF 1.0 extension?

Member

lexaknyazev commented Feb 27, 2017

Should we close this PR, or it will be published as glTF 1.0 extension?

@mlimper

This comment has been minimized.

Show comment
Hide comment
@mlimper

mlimper Feb 27, 2017

Contributor

I think we could close it.

@pjcozzi What is your opinion?

Contributor

mlimper commented Feb 27, 2017

I think we could close it.

@pjcozzi What is your opinion?

@pjcozzi

This comment has been minimized.

Show comment
Hide comment
@pjcozzi

pjcozzi Feb 27, 2017

Member

OK with me. If there are any TODOs in comments, please capture them in a separate issue.

Member

pjcozzi commented Feb 27, 2017

OK with me. If there are any TODOs in comments, please capture them in a separate issue.

@mlimper

This comment has been minimized.

Show comment
Hide comment
@mlimper

mlimper Feb 27, 2017

Contributor

I believe everything that was at some point identified as a separate TODO has already been captured in separate issues, which are all mentioned in this thread.
The ones that are still open are:
#697
#699

Contributor

mlimper commented Feb 27, 2017

I believe everything that was at some point identified as a separate TODO has already been captured in separate issues, which are all mentioned in this thread.
The ones that are still open are:
#697
#699

@mlimper mlimper closed this Feb 27, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment