glTF icon indicating copy to clipboard operation
glTF copied to clipboard

Proposal for vertex attribute based PBR materials

Open fhoenig opened this issue 7 years ago • 26 comments

A good chunk of this discussion was done via Twitter. Moving here for better record keeping.

fhoenig avatar Feb 07 '18 05:02 fhoenig

Probably needs one more change: Vertex attribute w.r.t. constant factors needs to be replace. Reasoning: when multiplied, it is set to 1.0 in most cases. This will result in fallback to fully metallic and no color. One can see this by opening the example in Windows Mixed Reality Viewer.

fhoenig avatar Feb 08 '18 09:02 fhoenig

Thanks for opening the discussion @fhoenig! Twitter context, for the curious.

I'm wondering why vertex colors would be stored in sRGB colorspace? That seems a bit out of place in this extension, to me.

EDIT: Alternatively, if sRGB vertex colors are a clear need, perhaps that should be addressed through a 2.X release of the core spec, and let this extension deal only with M/R attributes.

Probably needs one more change: [metal/rough] vertex attribute w.r.t. constant factors needs to be replace. Reasoning: when multiplied, it is set to 1.0 in most cases. This will result in fallback [in clients that don't support the extension] to fully metallic and no color.

Hm, unfortunate that it's inconsistent with vertex colors but this make sense. One alternative would be to specify new values in the extension:

    "materials": [
        {
            "name": "vertexPBRMat",
            "pbrMetallicRoughness": {
                "baseColorFactor": [1, 1, 1, 1],
                "metallicFactor" : 0.5,
                "roughnessFactor": 1
            },
            "extensions" : {
                "EXT_pbr_attributes" : {
                    "metallicFactorOverride" : 1
                }
            }
        }
    ],

But that does feel a little awkward, I'd be OK with the "replace instead of multiply" approach too.

donmccurdy avatar Feb 08 '18 21:02 donmccurdy

More Twitter context:

https://twitter.com/oceanquigley/status/960352377671856128

fhoenig avatar Feb 09 '18 19:02 fhoenig

I changed the multiply to replace in the recent commit. When a texture is present, they are still all multiplied, which should be fine. The constant factors can serve as a fallback now, which is described in best practice.

Regarding sRGB: I agree that it might be best to fix this in GLTF 2.x. Can more people weigh in on this please. I feel hesitant to remove it.

fhoenig avatar Feb 09 '18 23:02 fhoenig

Why adding operation if there is a texture in the material ? I feel it would be simpler to use vertex attribute if present otherwise metallicRoughnessTexture but not mixing them with a multiplication. For colorspace I feel it would be more consistent with pbr texture If we say: if the data is a color it's srgb if the data does not represent a color it's linear

cedricpinson avatar Feb 10 '18 13:02 cedricpinson

I feel it would be simpler to use vertex attribute if present otherwise metallicRoughnessTexture but not mixing them with a multiplication.

The thinking here was that if a person creates a gltf file with both, it should do something. The attribs would become a factor then. Nobody really needs to create such files but it would be a bit odd to have a ton of attributes and not used at all.

fhoenig avatar Feb 10 '18 20:02 fhoenig

FYI- https://github.com/UnboundTechnologies/glTF/pull/1 @hrydgard

Thoughts?

fhoenig avatar Feb 10 '18 20:02 fhoenig

Hi all, please review my pull request which modifies this proposal by adding an indirection from material properties -> mesh attributes. This provides for better flexibility in runtime environments, or in other words, brings functionalities of vertex attributes on the same level of flexibility as textures. This modification allows for classical game dev. tricks like blending between materials on state switches, better expressed future instancing GPU friendly workflows where instances can have different or random vertex based materials etc etc.

I think COLOR_0 approach in glTF 2.0 spec. where mesh data is directly affecting shading is not a good one, and the sooner we get rid of it the better.

Note that this material -> mesh approach also allows easier saving and loading for DCC apps and fits better with common game art workflows.

From the performance standpoint, one name lookup more is what's required from run-time engines, which should be of negligible performance impact.

Pull request: https://github.com/UnboundTechnologies/glTF/pull/1

Fully modified preview version: https://github.com/speedym/glTF/tree/master/extensions/2.0/Vendor/EXT_pbr_attributes

Looking forward to your feedback, Milan @ Foundry

speedym avatar Feb 13 '18 19:02 speedym

i think if we're going to be using vertex attributes for the same functionality as textures (defining metal/rough), it makes sense to give them the same level of indirection. If we want to have 1 mesh with a 'Fresh' and 'Beaten up' set of vertex attributes, it would make sense to allow one mesh primitive to be able to switch between them at runtime.

msfeldstein avatar Feb 13 '18 19:02 msfeldstein

I think the indirection is a fine proposal. But still unsure whether it belongs as part of this extension, another extension, or a future glTF release. With the EXT_ prefix, ideally there should be multiple vendors already planning to implement the extension in its entirety. I'm interested to learn more about the game workflows this would enable, and what other changes are required to make that happen. But IMO, a separate issue to discuss this material indirection would be best.

donmccurdy avatar Feb 13 '18 19:02 donmccurdy

What I would like to see in the example is a mesh referencing two materials. For example: left half of a sphere using textures (or a different attribute set) and right half using the default metal/rough attributes. This is what you are proposing Milan, right?

fhoenig avatar Feb 13 '18 23:02 fhoenig

Hey Don, Florian,

Any game art workflow where you'd want to blend between multiple materials in run-time would work smoothly with textures and would be a pain with vertex attributes (if they are not on feature parity with textures).

For example:

  1. A first person shooter gun which starts glowing red hot the more you use it and then cools down
  2. Any in-game item which transitions from new to worn out
  3. For future unlit workflows, transitioning the whole level environment from day to dusk to night and back

If we don't have the proposed indirection, for the above cases, DCC app would have to duplicate meshes for objects using attributes, and not duplicate meshes for objects using textures since textures have indirection. I hope you can probably imagine, at that point scene management becomes a big mess.

Also, with duplication of meshes, loading glTF files back into DCC app for a quick geometry edit, would produce multiple meshes inside DCC app, which then becomes a pain to edit due to double amount of meshes involved (at that point you can expect a lot of cursing artists when near their game deadline).

So to sum it up, that's why COLOR_0 special case was a bad idea, and I suggest that we don't go further down that road.

speedym avatar Feb 16 '18 09:02 speedym

Also, with duplication of meshes, loading glTF files back into DCC app for a quick geometry edit, would produce multiple meshes inside DCC app...

This may be considered an implementation bug in a DCC importer if it occurs. Where two meshes reference the same accessors, they should be considered instances of the same mesh. Probably the glTF spec should make this more explicit with a spec change or an implementation note, but I'd like to address that ecosystem-wide, not just through this extension. Opened a new issue to discuss.

If we don't have the proposed indirection, for the above cases, DCC app would have to duplicate meshes for objects using attributes, and not duplicate meshes for objects using textures since textures have indirection. ... So to sum it up, that's why COLOR_0 special case was a bad idea, and I suggest that we don't go further down that road.

Great examples and description of the problem, thanks — I'm still torn on how best to address it. The same issues will exist with KHR_materials_unlit and KHR_materials_pbrSpecularGlossiness, for example. All of the examples you shared would (in addition to the proposed indirection) require some mechanism for specifying the transition, which is another place the indirection could be introduced. Or perhaps this should be addressed in a glTF 2.X release, ecosystem-wide.

It is worthwhile to fix this, but may take some time to reach consensus, and meanwhile Unbound is already in early access using this extension (congrats @fhoenig!). With that in mind I'd suggest we do two things to move forward:

  1. Let's change the name of this extension to UNBOUND_pbr_attributes for now. The EXT_ prefix is intended for extensions where multiple vendors have fully implemented the spec, or are very close to doing so, which may take some time here. And in any case I think that a vendor-specific prefix is better for @fhoenig to iterate within the Unbound application while conversation continues here.
  2. Let's continue conversation in a new issue on material/attribute indirection and material transitions, and try to form consensus there before deciding how it should affect this PR.

donmccurdy avatar Feb 16 '18 17:02 donmccurdy

This may be considered an implementation bug in a DCC importer if it occurs. Where two meshes reference the same accessors, they should be considered instances of the same mesh

I think you're wrong here, and that's one of the main problems with this approach. Duplicated meshes will contain different attributes (say one mesh with day attribute and other with night), so DCC apps can't treat them as instances. Also, you can't easily blend between multiple meshes for transition between day and night in run-time. In other words, we're over-complicating run-time for attribute based workflows by going down COLOR_0 / this extension route.

All of the examples you shared would (in addition to the proposed indirection) require some mechanism for specifying the transition

I agree with the notion of transitions, and I think they would be a cool addition, but I don't think they are required. Note that gameplay scripters can easily do material blending from the code. My take is that transitions should be defined on a layer which is above materials.

So, meshes should define geometry, materials shading of geometry, and transitions on top blending between materials.

If Florian agrees, my proposal would be that we add indirection to this spec, and keep it as EXT_ since both Foundry and (I hope) Facebook would stand behind it. Then we can focus on defining transitions, and perhaps defining EXT_unlit_attributes and EXT_pbrSpecularGlossiness_attributes as well, or modifying KHR_materials_unlit and KHR_materials_pbrSpecularGlossiness to include material attribute properties.

speedym avatar Feb 16 '18 18:02 speedym

Duplicated meshes will contain different attributes (say one mesh with day attribute and other with night), so DCC apps can't treat them as instances. Also, you can't easily blend between multiple meshes for transition between day and night in run-time.

In all examples I've seen so far, the goal is blending between materials, and meshes are unrelated except the sense in which attributes are inputs to the material — to transition a scene from day to night, I would expect only a single mesh to be present. See example syntax here: https://github.com/KhronosGroup/glTF/issues/1250#issuecomment-366296745.

For that matter, morph targets may be used for this same purpose — morph targets could affect COLOR, METALNESS, or ROUGHNESS with very trivial loosening of the spec.

Again, multiple ways to address this problem, and I strongly recommend that we consider them holistically before putting something into this otherwise narrowly-focused extension.

donmccurdy avatar Feb 16 '18 18:02 donmccurdy

I agree with Don here. I don't think we should add something to this extension that raises the implementation complexity and doesn't actually enable any new functionality without additional application-specific understanding of the asset. Attribute indirection doesn't add a ton of extra complexity, but incremental complexity creep is a legitimate concern for the future of any extension, and needs to be kept to a minimum.

snagy avatar Feb 16 '18 22:02 snagy

@snagy @donmccurdy: I agree with simplicity.

Some clarification on the vendor specific path though: I'm against moving it away from EXT_ because that would make our original intent purposeless. Unbound does not load meshes. We have our own format which stores a list basis functions and we offer an iso-surface triangulation for exporting. We originally proposed the extension because FBX seems to be to rigid to cover simple scenarios for applications that have no UV mapping capabilities. This covers many new content generation tools as well as many WebGL based applications, galleries and demos. Before proposing, we asked around if anyone else would be interested in this and the list of contributors / acknowledgements have all expressed various degrees of interest / need for this particular scenario.

Regarding @speedym's proposed changes: Milan, I will take you up on connecting online and getting a demo from you. However, best way to think about what you're proposing is that it is a superset of this extension here. Why? Your tool is a superset in terms of functionality. Having EXT_pbr_attributes reserve the two extra vertex attribute names does not restrict you from creating MODO_pbr_attributes as a superset. I do believe that would even fall back gracefully.

So here is what I suggest moving forward. Let's address the "polish" on this extension and the open issues regarding color space and also @bghgary's comment on "How does this interact with DRACO".

For reference, the colorspace was put in to accommodate Oculus Medium's exporter and makes a lot of sense. I'd love to hear more feedback from David Farrell on this.

DRACO - I have to admit, even though I find it super interesting especially for this per-vertex material case, I have not had the time to dig into it. If you guys can give me any pointers or a quick kickstart, I'd be happy to add some interaction language.

fhoenig avatar Feb 17 '18 01:02 fhoenig

@fhoenig: Sounds good. I'm back from vacation and we can meet during the week.

My worry is that if we went ahead and implemented MODO_material_pbr_attributes (or EXT_material_pbr_attributes if Facebook agrees to support it), that extension and this one would be way too similar (we'd have two different ways of expressing the same thing) and I feel it would fragment the landscape of implementations too much, for the cost of only one name based lookup.

speedym avatar Feb 19 '18 15:02 speedym

All-

After a good discussion with @speedym today, here is a new (yet unfinished) version of the proposal.

Please for now only pay your attention to the example code, as the rest is not complete.

I've basically added the proposed indirection via material. This makes a lot of sense and doesn't complicate the implementation. Also additional attributes are recommended to be named with "_" prefix for maximum backward compatibility and fallback. We've just seen this issue today with Facebook's implementation not validating a file with unknown attribute names that a not prefixed. Which is valid.

Please let me know your thoughts.

fhoenig avatar Feb 23 '18 01:02 fhoenig

Yup, we had a very productive conversation, and I have to thank @fhoenig for his time.

I fully agree with the latest changes, since this extension does not trip up validators anymore, and we have added indirection for closer feature parity between attributes & textures. As an added bonus, this also enables relatively easy exporting / importing and shading of instances for DCC apps which require complete geometry data to be the same between all instances of a mesh. So, we've nailed three flies with one stroke.

The thing I found ugly is that we had to override factor parameters by attributes, which is different from what textures do, because of the bad fallback look, in case this extension is not supported. I think this whole problem space should be better addressed in future glTF 2.x specs. Also, if this ext. gets folded into main spec, I would expect factors to multiply attributes for feature parity with textures.

I think it would be important to mention that we discussed and in broad terms agreed on two main paths glTF files can be used after exporting, and their widely different requirements:

  1. for viewing - which means optimized data and easy viewer implementation
  2. editing / 3d data storage - which means we can export / import various complicated features of DCC apps such as Modo, as separate MODO_* extensions

I have also shown Florian basics of Modo's 3D / game art pipeline, so he's got a good glimpse into what a modern DCC app can do in that context. If anyone else would like to see this kind of presentation, please let me know / contact me in private. I think it's very useful as an insight for future glTF 2.0 / ext. development directions, and for striking a good balance between glTFs for viewing and editing (ie. 1. & 2.).

speedym avatar Feb 26 '18 17:02 speedym

Couple specific things first —

  • Might lean away from abbreviating "attribute" as "attrib" since we've spelled it out in the rest of the glTF schema.
  • Consider renaming to EXT_attributes_pbrMetallicRoughness to disambiguate from the spec/gloss workflow.
  • Why is baseColorSpace needed for this extension? Does it apply to baseColorFactor as well? I can't find anything in the current spec to say what colorspace is used, but would assume it's linear. Are we confident that multiple vendors are going to implement support for both colorspaces, or can we ask exporters to convert?
  • Should morph targets be described in the extension? All three use cases for attribute indirection mention transitions from one attribute-based material to another.

More broadly —

The thing I found ugly is that we had to override factor parameters by attributes, which is different from what textures do, because of the bad fallback look, in case this extension is not supported. I think this whole problem space should be better addressed in future glTF 2.x specs.

This is why I think attribute/semantic indirection should be omitted from this extension and addressed in a more complete way elsewhere. By including a partial solution here, we're creating wrinkles for any attempt to address it in the future. I also think we're conflating DCC tool needs (see below) with runtime material transitions, which could be addressed with morph targets (see https://github.com/KhronosGroup/glTF/issues/1250).

I think it would be important to mention that we discussed and in broad terms agreed on two main paths glTF files can be used after exporting, and their widely different requirements:

  1. for viewing - which means optimized data and easy viewer implementation
  2. editing / 3d data storage - which means we can export / import various complicated features of DCC apps such as Modo, as separate MODO_* extensions

Note that glTF is primarily a transmission format, and that is a crucial part of its appeal for viewing applications. Retaining authoring data has historically been at odds with that goal in other 3D formats. Anyone is welcome to define extensions for whatever purposes of course, but in terms of glTF future development, simplicity for viewers is important. I would hope that extensions intended for (1) and (2) could be distinct.

donmccurdy avatar Feb 26 '18 23:02 donmccurdy

Hi Don, thanks for the feedback!

Let me comment on a few points:

Why is baseColorSpace needed for this extension? Does it apply to baseColorFactor as well? I can't find anything in the current spec to say what colorspace is used, but would assume it's linear. Are we confident that multiple vendors are going to implement support for both colorspaces, or can we ask exporters to convert?

For 8bit attributes (ie. char), sRGB is the optimal color representation. For >8bit (16bit int, floating point) attributes and other values, optimal colorspace is linear. We wanted to cater for both options, since there are scenarios where artists would want to optimize for network bandwidth and use 8bit representation. It does not apply to baseColorFactor since it's a floating point number - actually baseColorFactor is ignored by this version of the spec.

Should morph targets be described in the extension? All three use cases for attribute indirection mention transitions from one attribute-based material to another.

I would prefer if animating between materials would be a separate extension, to avoid complicating this one further, and also to have clean layering.

By including a partial solution here, we're creating wrinkles for any attempt to address it in the future.

Can you elaborate on this please? I thought when folding an extension into the next version of glTF standard, we could tweak it further? We considered adding additional baseColorAttributeFactor, metallicAttributeFactor and other relevant factor parameters, so that attribute functionalities are exactly on par with textures. Would that resolve your concerns?

I would hope that extensions intended for (1) and (2) could be distinct.

Yup, I fully agree on that. On the other hand, it would be great if we could stick to a single way of doing things in similar areas - for example, if textures have indirection and a factor, I think attributes should have them too. There is a wide discrepancy in approach between COLOR_0 and textures, which is IMHO extremely ugly.

speedym avatar Feb 27 '18 14:02 speedym

Consider renaming to EXT_attributes_pbrMetallicRoughness to disambiguate from the spec/gloss workflow.

Agreed, we'll need one more similar extension for unlit attributes, as well. Note that we have already implemented support for KHR_materials_unlit in Modo. :)

speedym avatar Feb 27 '18 14:02 speedym

@fhoenig and authors - is this draft extension being used in any tools or engines? Does this need more work to get over the finish line? Or is this something that should be archived for future consideration?

Just tidying up open pull requests.

pjcozzi avatar Jan 25 '19 22:01 pjcozzi

Hi-

It’s implemented in Unbound. The plan is to implement it in a web based lib (probably the microsoft one) next but time is constraint. Probably in the next few month.

Thanks for the heads up!

Florian

On Fri, Jan 25, 2019 at 2:13 PM Patrick Cozzi [email protected] wrote:

@fhoenig https://github.com/fhoenig and authors - is this draft extension being used in any tools or engines? Does this need more work to get over the finish line? Or is this something that should be archived for future consideration?

Just tidying up open pull requests.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/KhronosGroup/glTF/pull/1235#issuecomment-457749702, or mute the thread https://github.com/notifications/unsubscribe-auth/AAWI6NBQcPyn2zgeRBVh7zVgzFmQmjnlks5vG4GfgaJpZM4R8JgO .

fhoenig avatar Jan 26 '19 20:01 fhoenig

Thanks for the quick response, @fhoenig!

Do you know if there are any other updates to the spec extension required beyond the TODOs? Is anyone available to make those changes?

Lastly, do you know if this should be an EXT_ or vendor extension? Has that been discussed?

pjcozzi avatar Jan 28 '19 02:01 pjcozzi