-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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
Compute normals for voxel shape intersections #11847
Conversation
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.
Thanks @jjhembd! I have a few comments and questions.
@@ -182,7 +185,7 @@ void main() | |||
// This is similar to traverseOctreeFromBeginning but is faster when the ray is in the same tile as the previous step. | |||
positionUvShapeSpace = convertUvToShapeUvSpace(positionUv); | |||
traverseOctreeFromExisting(positionUvShapeSpace, traversalData, sampleDatas); | |||
step = getStepSize(sampleDatas[0], viewRayUv, shapeIntersection); | |||
step = getStepSize(sampleDatas[0], viewRayUv, shapeIntersection, currT); |
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 know it's internal, but we try to avoid abbreviations where possible. Can this become currentT
?
@@ -20,6 +20,36 @@ struct RayShapeIntersection { | |||
vec4 exit; | |||
}; | |||
|
|||
vec4 intersectionMin(in vec4 intersect0, in vec4 intersect1) | |||
{ | |||
if (intersect0.w == NO_HIT) { |
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.
Do we need the same conditional for intersect1
?
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.
The other conditional wasn't happening in current usage, but I agree, handling both cases would be more future-proof.
return vec2(NO_HIT); | ||
// Scale the ray by the ellipsoid axes to make it a unit sphere | ||
// Note: approximating ellipsoid + height as an ellipsoid | ||
// Note: may be better to approximate radiiCorrection for small relative heights |
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.
Does this comment indicate a future TODO, or a recommendation to the function user? If the former, it's better to put that in an issue than the code itself, where it may get overlooked.
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 removed the comment--it's irrelevant now that we aren't using this function for individual voxel intersections.
|
||
float a = dot(direction, direction); // ~ 1.0 (or maybe 4.0 if ray is scaled) | ||
float b = dot(direction, position); // roughly inside [-1.0, 1.0] when zoomed in | ||
float c = dot(position, position) - 1.0; // ~ 0.0 when zoomed in. Note cancellation problem! |
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.
Cancellation problem?
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.
This one is also OBE, so I removed it
|
||
if (det < 0.0) { | ||
float sinSqrHalfAngle = 1.0 - cosSqrHalfAngle; | ||
// a = d.z * d.z - dot(d, d) * cosSqrHalfAngle; |
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.
Remove commented lines?
The normals for both the ellipsoid and cylinder are looking good in the example! |
Looks good to me! Thanks @jjhembd! |
Description
This PR adds voxel surface normals for the Cylinder and Ellipsoid shapes, following what was done in #11076 for Box-shaped voxels.
There are 3 places where surface normals are relevant:
This PR only addresses the first 2 of these. Normals for individual voxels are not computed. If the current sample position is within a voxel cell, the value of
fsInput.voxel.surfaceNormal
is simply the current ray direction.Testing plan
Load this local Sandcastle and use the "Voxel Inspector" sliders to check the lighting on different shape bound or clipping surfaces.
Author checklist
CONTRIBUTORS.md
CHANGES.md
with a short summary of my change