Fix parallax mapping correctness and promote its example - #9207
Merged
Conversation
Build size reportThis PR changes the size of the minified bundles.
|
Contributor
|
If parallax mapping was broken, why not just upgrade it to something more modern like displacement mapping? |
Contributor
Author
Working on that in a follow up PR, as an optional (and more expensive) option. But this is a valid option for lower end devices. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
StandardMaterial#heightMapwas a single-tap offset mapping implementation with aviewDirT.z += 0.42fudge added in 2016 and never revisited. Several parts of it were broken, and the parts that worked were about 4x too weak to see.Changes:
getParallax()ran aftergetOpacity(), so the opacity map sampled un-offset uvs while every other map of the material was offset (GLSL and WGSL).hasTBNexcludeduseHeights, so no tangent frame was generated and the offset was silently zero.LIT_HEIGHTSnow also selects the derivative-based frame, and the frame's uv falls back to the height map's uv channel._getUvSourceExpressiontakes a newallowParallaxOffsetargument for this.dUvOffsetis no longer referenced by passes which do not compute it. It is declared underFORWARD_PASSonly, so any other pass with a height map enabled would emit a shader that fails to compile. The renderer does not reach that combination today, so this is an invariant rather than a live bug.height * normalize(viewDirT).xy, replacing theviewDirT.z += 0.42fudge which both understated the effect and skewed it as the view direction changed.test/scene/shader-lib/standard-parallax.test.mjscovering the four fixes above, each checked to fail against the unfixed source.API Changes:
heightMapFactorscales the offset by0.1rather than0.025, so the default factor of 1 displaces the texture by up to 5% of a uv tile instead of ~1.25%. Existing content using a height map will show a noticeably stronger effect - divide the previous value by 4 to keep the old look. Useful values are now roughly 0 to 2.StandardMaterial#heightMapand#heightMapFactordocs now state that mid-grey is the level of the original geometry, that the offset applies to every other map of the material (so the height map needs the same tiling and offset as those maps), and what the factor means in uv units.Examples:
materials/parallax-mapping- promoted out oftest/and un-hidden. Four shapes in a 2x2 grid compare diffuse only, normal map, height map alone, and normal map + height map, with a shape selector (box or sphere) so the offset is exercised on differently-oriented tangent frames, and a slider drivingheightMapFactor. Thumbnails regenerated.#7821 could not be reproduced - the shader compile failure theorised in that report is not reachable through the renderer, as noted above - but the parallax path it exercises is fixed here, so it is closed by this change too.
Parallax occlusion mapping (#155) is not part of this change.
Fixes #8689
Fixes #7821
Closes #5209