-
Notifications
You must be signed in to change notification settings - Fork 313
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
Added normal mapping example #305
Conversation
Test Merge
…aders, texture loading, or maps need work
…nctions, and added additional comments to code
Ready for review. Contains some of the same utils as used in the bitonicSort shader, which can either be inlined, kept, or moved to a separate folder depending on the maintainers' preferences. |
Adding video to demonstrate intended functionality. Normal.Mapping.-.WebGPU.Samples.-.Google.Chrome.2023-10-25.17-03-15.mp4 |
@greggman could you help review this PR? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So I don't know how opinionated to be on this review. Please excuse me if I'm being too strict. I'm just going with my own thoughts. I'm also unfamiliar with the other samples. They may have similar issues.
I ran it locally and ran into the following issues
- The cube is too far away to see any details
To fix I set the camera to this
cameraPosX: 0.0,
cameraPosY: 0.8, // was 0
cameraPosZ: -1.4, // was -2.4
I removed the x rotation from the cube since just moving the camera above the cube seemed less surprising
// mat4.rotateX(modelMatrix, 10, modelMatrix);
and updated the perspective to 0.1 instead of 1 for near
const projectionMatrix = mat4.perspective(
(2 * Math.PI) / 5,
aspect,
0.1, // was 1
10.0 // was 100
) as Float32Array;
- The view matrix seemed wrong. I changed to this
function getViewMatrix() {
return mat4.lookAt(
[settings.cameraPosX, settings.cameraPosY, settings.cameraPosZ],
[0, 0, 0],
[0, 1, 0]
);
}
- The light seemed wrong.
If the cube is at 0,0,0 and the camera is -Z then a light at
lightPosX: 1.7,
lightPosY: -0.7,
lightPosZ: 1.9,
Would be behind the cube (camera is -Z, light is +Z), to right (X is positive), and under the cube (Y is negative).
Changed to
lightPosX: 1.7,
lightPosY: 0.7, // above the cube
lightPosZ: -1.9, // same side as camera
-
Then, I couldn't see any difference in rendering between these "Bump Mode"s
| 'Normal Texture'
| 'Depth Texture'
| 'Normal Map'
| 'Parallax Scale'
| 'Steep Parallax';
To make sure I wasn't crazy I added an 'animate' GUI setting that stops the rotation. To do that I added a clock
const settings: GUISettings = {
...
animate: true,
...
};
...
gui.add(settings, 'animate');
...
let then = 0;
let time = 0;
function frame(now) {
if (!pageState.active) return;
const elapsedTime = settings.animate ? (then - now) * 0.001 : 0;
time += elapsedTime;
then = now;
// Write to normal map shader
const viewMatrix = getViewMatrix();
const modelMatrix = getModelMatrix(time);
and updated getModelMatrix
to
function getModelMatrix(time: number) {
const modelMatrix = mat4.create();
mat4.identitåy(modelMatrix);
mat4.rotateY(modelMatrix, time * -0.5, modelMatrix);
return modelMatrix;
}
Once I had that in I tried stopping the animation and switching the bump mode. There is no difference in rendering between any of them except for "Diffuse Texture"
Screen.Recording.2023-10-26.at.18.11.40.mov
- I'm not sure why all the casts like this
// Write to normal map shader
const viewMatrixTemp = getViewMatrix();
const viewMatrix = viewMatrixTemp as Float32Array;
const modelMatrixTemp = getModelMatrix();
const modelMatrix = modelMatrixTemp as Float32Array;
I changed to
// Write to normal map shader
const viewMatrix = getViewMatrix();
const modelMatrix = getModelMatrix(time);
And I don't get any TypeScript warnings 🤷♂️
Here's the changes I made mentioned above
depthSample: f32, | ||
depthScale: f32, | ||
) -> vec2f { | ||
if (mapInfo.mappingType == 4) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are uber shaders generally considered bad practice? Would this be better with a separate shader for each mode? I'd think we don't want people copying this example if it's not considered a good thing to do. But I might wrong.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's more understandable and performant than having to manage multiple pipelines, especially when different types of depth mapping are still using the same code, but I defer to your expertise.
I implemented most of the suggestions (although the perspective of my camera doesn't seem to match yours). However, I left in the model rotation, since I think it more effectively demonstrates how the perceptual depth changes as the surface moves in and out of light, and I wasn't sure what was meant by surprising (too visually distracting, unexpected?). |
Also, I was able to fix the issue with the map mode not changing by remembering to write that value as a u32 rather than a f32. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
An example of normal mapping that allows the user to adjust the lighting of the scene, the depth layers, and the currently displayed combination of diffuse, normal, and height maps (Spiral, Toybox, Bark).
Current issues to be addressed:
Parallax Occlusion Mapping (Possibly in a later PR)
Weird Bark Height Map Behavior
Creating Generalized Way of Loading Images in Assets Folder from Helper Functions Rather Than Public
Removing unused types, interfaces, and helper functions per usual.