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

Attenuation thickness not applied correctly? #10151

Closed
emackey opened this issue Apr 7, 2021 · 15 comments
Closed

Attenuation thickness not applied correctly? #10151

emackey opened this issue Apr 7, 2021 · 15 comments
Assignees
Milestone

Comments

@emackey
Copy link

emackey commented Apr 7, 2021

Although KHR_materials_volume is still in draft, I've been testing the implementation provided by @MiiBond in #8684.

My latest test model is DragonAttenuation. (Sample PR opened as KhronosGroup/glTF-Sample-Models#306).

In the Khronos Sample Viewer, the model looks like this:

screenshot_large

But, in Babylon Sandbox, I'm seeing this:

image

It's hard to see in the screenshot, but the deep orange color is somewhat visible in the thicker parts of the dragon. The thin parts are opaque white/gray, possibly the color of the thickness map itself.

Maybe there's a bug where the thickness map colors are leaking into BaseColor somehow?

This model also features a material variant, with a variant called "Surface Color" where the yellow itself comes from the baseColor and there is no attenuation. This variant appears to work correctly in the sandbox.

@sebavan
Copy link
Member

sebavan commented Apr 7, 2021

@Popov72 and @MiiBond do you think it is related to the latest in progress PR ? or could we check what is causing it ?

@MiiBond
Copy link
Contributor

MiiBond commented Apr 7, 2021

@emackey thanks for raising this. It's nice to have a point of comparison.

@Popov72 It's not related to the current PR.
There are 3 different things going on here, two of which are easy fixes.

  1. The thickness texture is improperly being used to mask transmission. That's where the white is coming from.
  2. The attenuation colour is being linearized at load but should already be linear in the glTF.
  3. The attenuation distance isn't being scaled based on the transform of the node.

I'll open a PR for the first two. For the node scale, maybe the Babylon team could come up with a way to set this dynamically so that the shader can scale the attenuation distance based on the node's world scale? I'm pretty swamped with other work at the moment.

@MiiBond
Copy link
Contributor

MiiBond commented Apr 7, 2021

Babylon PR for the first two issues is here:
#10153

The dragon is scaled to 0.24 so, in Babylon Sandbox, if you scale up the attenuation colour by 4, you get this:
Screen Shot 2021-04-07 at 4 12 14 PM

@emackey
Copy link
Author

emackey commented Apr 7, 2021

@MiiBond Awesome, thanks for taking a look at this!

Point of clarification on scaling:

  • attenuationDistance is a material property, specified in actual world coordinates. It should not scale with the node.

  • thicknessFactor is specified in the mesh's local coordinate system, and typically equals the distance at the thickest portion of the mesh. This allows a normalized thicknessTexture where "white" indicates the thickest part. But when rendering, thicknessFactor does need to inherit the rough uniform scaling of the node it's attached to, including all parent transforms (inside & outside the glTF). If the mesh has been scaled larger or smaller, then the thickest part has likewise been scaled, and so the scaling must be taken into account when processing the thickness. The final thickness is that scaling * thicknessFactor * thicknessTexture.

@emackey
Copy link
Author

emackey commented Apr 7, 2021

Just double-checking my math here, since this a new test model.

  • All world distance units default to meters, as per usual.

  • The thickness texture is normalized. The white part covers the thickest portion of the dragon's body, where the thickness is about 2.2 in unscaled mesh space (raw vertex space).

  • The thicknessFactor was manually set to 3.5 (although in a future iteration, perhaps I should lower this to 2.2, but let's leave that aside for now).

  • The node's world-to-local scale is 0.25, so the manually-assigned thickness of "white" in world-space is 3.5 * 0.25 = 0.875. The thickest part of the scaled dragon's body (no pun intended) should be about 0.875 in world space.

  • The attenuationDistance is set to 0.239015, and this is world space. The color is a faint yellow, and that color should be reached at the attenuation distance. Longer distances should result in darker colors.

So the thickest part of the dragon's body is 3.66 times the attenuation distance, making it dark orange. Thinner parts, such as the claws, are closer to 1x the attenuation distance, making them faint yellow.

If the node scaling were not accounted for here, yes, the dragon would be considered 4 times thicker and darker than it is. The claws would be orange and the body would be a very deep dark color.

So it seems like I could tweak the model to lower both the thicknessFactor and attenuationDistance by the same percentage, to get them to align to what a path tracer would calculate. But the bug here remains, the node's general world-to-local scaling should be multiplied with the thicknessFactor here.

@MiiBond
Copy link
Contributor

MiiBond commented Apr 8, 2021

@emackey yes, I wasn't being careful when I wrote that message. Yes, you're correct. The thickness scale is what needs to be set dynamically so that the attenuation can be applied. Right now, the shader assumes that the thickness is already in world units.

@emackey
Copy link
Author

emackey commented Apr 8, 2021

I wonder if this is something that can be adapted at the app level, instead of the shader. One could premultiply the thicknessFactor with the scaling. I haven't yet looked at the sample viewer implementation to see if that's what they're doing.

@MiiBond
Copy link
Contributor

MiiBond commented Apr 8, 2021

That's exactly what I'm doing in Adobe Dimension.

@Popov72
Copy link
Contributor

Popov72 commented Apr 8, 2021

There are 3 scaling values, one for each x/y/z axis: is glTF specifying how to derive the final scale value to use in the thicknessFactor * scale computation?

@MiiBond
Copy link
Contributor

MiiBond commented Apr 8, 2021

We don't yet specify this. In Adobe Dimension, I chose to take the average so as to try to better handle non-uniform scaling but I think taking the maximum is maybe the better choice. I may end up switching Dimension's logic to do that as well.

@emackey
Copy link
Author

emackey commented Apr 8, 2021

is glTF specifying how to derive the final scale value to use

glTF does not dictate a specific formula there, as "thickness" is merely an approximation for something that a path tracer would be able to calculate true values for.

I could imagine using the max of x, y, z, or using an average. But ultimately, non-uniform scales will never apply correctly to a mesh with a baked thickness map. A strongly nonuniform scale would cause the values of a thickness map to change substantially, if it were to be re-baked at that scale. There's no fixing that with a multiplier. Perhaps we should add a note to the spec that says you shouldn't nonuniformly scale a node with a thickness map.

@MiiBond
Copy link
Contributor

MiiBond commented Apr 8, 2021

There's nothing really stopping an implementation from handling non-uniform scaling correctly based on view direction. So, specifying that the thicknessScale in glTF applies to the max dimension of the un-scaled mesh seems reasonable. It's up to an implementation if they go to the trouble of supporting non-uniform scaling in their rendering.

@Popov72
Copy link
Contributor

Popov72 commented Apr 14, 2021

On our side, we have merged the PR from @MiiBond + we pre-multiply the thicknessFactor with the max(abs(scaleX), abs(scaleY), abs(scaleZ)).

@emackey
Copy link
Author

emackey commented Apr 14, 2021

@Popov72 Sounds great!

@sebavan
Copy link
Member

sebavan commented Apr 15, 2021

Thanks @Popov72 !!!

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

No branches or pull requests

6 participants