AcademySoftwareFoundation / OpenPBR

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

Normal maps #74

Closed portsmouth closed 3 months ago

portsmouth commented 1 year ago

@brechtvl wrote:

It's not clear to me if these parameters expect tangent space or world/rendering space normals. I would guess it's world/rendering space if bump maps are to be supported as well, and because also for normal maps there might be various additional controls that feel out of scope for OpenPBR.

portsmouth commented 1 year ago

I generally feel also that there are too many additional parameters needed for this to be fully specified within the model. There may be a need for an industry standard normal mapping specification covering all the possible parametrizations, but it seems out of scope.

In the existing document, we just say the shader will be packaged with some metadata that makes sense of the normal map inputs in some reasonably unambiguous fashion, producing a well-defined shading frame. In practice maybe that works for asset exchange, I'm not sure.

anderslanglands commented 1 year ago

I don't think that's enough. What other information would be needed to support tangent-space normal maps other than specifying whether it's "DX-" or "OGL-style"?

brechtvl commented 1 year ago

In MaterialX, there is a normalmap node with these inputs, and my understanding of their purpose:

brechtvl commented 1 year ago

So far the reference implementations of Standard Surface and OpenPBR have not dealt with normal maps, they just take world space normals as input. The Standard Surface specification also does not mention normal maps. To me that seems fine in the context of OSL and MaterialX implementations, additional nodes can be used for normal maps or bump maps.

The OpenPBR specification does mention normal maps, but it's unclear to me what the intent was exactly. Are node based implementations like OSL and MaterialX intended to interpret the normal input different than before? Or is it specifying a convention for other implementations that only handle fixed values and (baked) texture maps for inputs?

portsmouth commented 1 year ago

I think the intention of the spec was to try to make it clear how the shading frame of the model is specified, i.e.:

This just makes it clear that only these 3 vector fields are under control. For definiteness, we say these are given in world space. Each of these, if unspecified, will revert to the unperturbed world-space shading frame vectors given by the geometry.

The actual input though (for normals) is presumably usually a normal or bump map, with some convention for mapping from the texels to the perturbation of the normal in the local space (except for special cases with procedural normals/tangents). It seems somewhat out of scope though for the shading model itself to provide this mapping, as there are lots of different conventions out there (for the data/file format, meaning of the channels, how the scaling is done etc.).

In Arnold there are separate nodes that handle normal/bump maps, with 20+ parameters controlling just that aspect. I don't think we can roll those into OpenPBR, and if we did it still wouldn't cover the needs of other renderers.

Agreed that ideally there needs to be some way to make it unambiguous how a given asset is normal/tangent mapped. We did hint at this in the Metadata section, saying that the shader should be packaged with metadata that provides

The assumed convention for converting the input geometry_normal, geometry_tangent and geometry_coat_normal maps, if present, to the final shading normal of the local surface at each point.

Though we don't currently make it concrete how that is specified.

meshula commented 1 year ago

We left things perhaps unintentionally subject to interpretation in MatX and USD PreviewSurface; iteration is ongoing to straighten things up. I think the conventions around tangent space for normal maps should be well defined up front since history shows it can take a while to sort out after the fact. I don't know how many bugs I had to look at in Hydra around "why are the normals from the normal map wrong in this one weird corner case".

meshula commented 1 year ago

MaterialX settled on the Mikkelsen, as has the games industry, pretty much. https://github.com/AcademySoftwareFoundation/MaterialX/blob/6d6c5aa72670c38de95726190a51ccf9bd935485/source/MaterialXRender/Mesh.cpp#L89

https://github.com/mmikk/MikkTSpace/blob/master/mikktspace.h

portsmouth commented 6 months ago

Just to note, I think that unambiguous specification of the normal map conventions is something that would be very valuable to have in the model, eventually, to eliminate the pain associated with the convention being ill-defined, mentioned by @anderslanglands and @meshula.

But i'd defer this for a later point in time once (perhaps) the model is more established, as to do it now would add quite a bit of complexity and it's not necessarily clear what parametrization to choose (or how the lack of it impacts the integration of the model into pipelines and workflows).