AcademySoftwareFoundation / OpenPBR

Specification and reference implementation for the OpenPBR Surface shading model
Apache License 2.0
425 stars 18 forks source link

Spec version metadata #83

Open anderslanglands opened 1 year ago

anderslanglands commented 1 year ago

In section 2.5, the spec lists a series of metadata to help compatibility. Ought the spec version be part of this? Or is there some other way to inspect which version of the spec a particular instance of an OpenPBR shader is?

In addition, from reading it's not clear what the different metadata being mentioned are, or where they are applied? Would be great to specify all this explicitly.

portsmouth commented 1 year ago

I think it would make sense for the metadata to include the version.

The section about metadata is a bit tentative. It doesn't seem appropriate to make such metadata (e.g. the assumed color space of the parameters) part of the material model itself, as it would require introducing string or enum parameters into the model.

The assumption seemed to be that way the shader parameters are packaged/contained for exchange is outside of the scope of the spec, and so perhaps it suffices to verbally/loosely describe the extra metadata/information needed that should be packaged with the parameters. I assume this has been done to some extent for the Standard Surface model.

For example how the N/T/B of the shading frame are actually defined (e.g. a normal map and all its parameters in some specific convention we don't dictate), can presumably be made reasonably unambiguous, but it's beyond the scope of the current spec to do that apart from pointing out that the asset management should set up the metadata to make it unambiguous.

Perhaps though we need to try to be more specific and actually have named pieces of metadata, with specified datatypes etc. I'm not sure.

anderslanglands commented 1 year ago

I’m in two minds over whether you even want to specify the colour space on the material itself for anything other than texture parameters. You’re assuming that the material description will always be transmitted in MtlX and/or UsdShade right?

I’m not too familiar with how MtlX handles this: does it just provide colour space information as a hint or will it also do the conversion to your desired rendering space for you? USD is supposed to do that for you in some future version using Hydra 2.0, at which point colour space metadata on material parameters is redundant at best, and a point of conflict/failure at worst.

I feel pretty strongly that normal map conventions need to be specified. The OGL/DX thing for normal maps is a constant issue in real assets and not providing an explicit means to specify it (eg a “flip green” parameter) will just lead to stuff breaking.

On Mon, 28 Aug 2023 at 06:05, Jamie Portsmouth @.***> wrote:

I think it would make sense for the metadata to include the version.

The section about metadata is a bit tentative. It doesn't seem appropriate to make such metadata (e.g. the assumed color space of the parameters of the material model), as it would require introducing string or enum parameters into the model.

The assumption seemed to be that way the shader parameters are packaged/contained for exchange is outside of the scope of the spec, and so perhaps it suffices to verbally describe the information needed that should be packaged with the parameters.

For example how the N/T/B of the shading frame are actually defined (e.g. a normal map and all its parameters in some specific convention we don't dictate), can presumably be made reasonably unambiguous, but it's beyond the scope of the current spec to do that apart from pointing out that the asset management should set up the metadata to make it unambiguous.

Perhaps though we need to try to be more specific and actually have named pieces of metadata, with specified datatypes etc. I'm not sure.

— Reply to this email directly, view it on GitHub https://github.com/AcademySoftwareFoundation/OpenPBR/issues/83#issuecomment-1694727661, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAOYQXMDF6XZBRRNMCBOLVTXXOD7PANCNFSM6AAAAAA3ZESZHQ . You are receiving this because you authored the thread.Message ID: @.***>

anderslanglands commented 1 year ago

The other option of course is as Brecht suggested and you just make the normal input explicitly and only in world space, and leave it to MtlX to specify the mapping from NTB.

On Mon, 28 Aug 2023 at 06:59, Anders Langlands @.***> wrote:

I’m in two minds over whether you even want to specify the colour space on the material itself for anything other than texture parameters. You’re assuming that the material description will always be transmitted in MtlX and/or UsdShade right?

I’m not too familiar with how MtlX handles this: does it just provide colour space information as a hint or will it also do the conversion to your desired rendering space for you? USD is supposed to do that for you in some future version using Hydra 2.0, at which point colour space metadata on material parameters is redundant at best, and a point of conflict/failure at worst.

I feel pretty strongly that normal map conventions need to be specified. The OGL/DX thing for normal maps is a constant issue in real assets and not providing an explicit means to specify it (eg a “flip green” parameter) will just lead to stuff breaking.

On Mon, 28 Aug 2023 at 06:05, Jamie Portsmouth @.***> wrote:

I think it would make sense for the metadata to include the version.

The section about metadata is a bit tentative. It doesn't seem appropriate to make such metadata (e.g. the assumed color space of the parameters of the material model), as it would require introducing string or enum parameters into the model.

The assumption seemed to be that way the shader parameters are packaged/contained for exchange is outside of the scope of the spec, and so perhaps it suffices to verbally describe the information needed that should be packaged with the parameters.

For example how the N/T/B of the shading frame are actually defined (e.g. a normal map and all its parameters in some specific convention we don't dictate), can presumably be made reasonably unambiguous, but it's beyond the scope of the current spec to do that apart from pointing out that the asset management should set up the metadata to make it unambiguous.

Perhaps though we need to try to be more specific and actually have named pieces of metadata, with specified datatypes etc. I'm not sure.

— Reply to this email directly, view it on GitHub https://github.com/AcademySoftwareFoundation/OpenPBR/issues/83#issuecomment-1694727661, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAOYQXMDF6XZBRRNMCBOLVTXXOD7PANCNFSM6AAAAAA3ZESZHQ . You are receiving this because you authored the thread.Message ID: @.***>

portsmouth commented 1 year ago

See https://github.com/AcademySoftwareFoundation/OpenPBR/issues/74#issuecomment-1706566753 for my take on the normal map specification issue.

portsmouth commented 6 months ago

We did add "The version of the specification implemented" to the suggested metadata.

The normal map conventions would be quite involved to properly specify (I think). So it would be for a later release, if we do it at all (it could be that this is outside of the scope of the spec permanently though).