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

Simplify voxel shaders #11838

Merged
merged 7 commits into from
Feb 22, 2024
Merged

Simplify voxel shaders #11838

merged 7 commits into from
Feb 22, 2024

Conversation

jjhembd
Copy link
Contributor

@jjhembd jjhembd commented Feb 16, 2024

Description

This PR simplifies the voxel shaders with two main changes:

  1. Isolate longitude intersection functions in the new IntersectLongitude.glsl, so they can be re-used for both Ellipsoid and Cylinder shapes
  2. Consolidate the shader defines used to handle different ranges of shape bounds

The existing code handled different ranges of shape and render bounds by re-compiling with different defines. For example,

    #if defined(CYLINDER_HAS_SHAPE_BOUNDS_RADIUS_FLAT)
        float radius = 1.0;
    #else
        float radius = length(positionLocal.xy); // [0,1]
        #if defined(CYLINDER_HAS_SHAPE_BOUNDS_RADIUS)
            radius = radius * u_cylinderUvToShapeUvRadius.x + u_cylinderUvToShapeUvRadius.y; // x = scale, y = offset
        #endif
    #endif

The case with RADIUS_FLAT is where the user is not viewing a volume, but a thin shell with constant radius.

With this PR, when the user selects a single radius, the value of the scale and offset in u_cylinderUvToShapeUvRadius is adjusted on the CPU side to return 1.0, so that the same shader path can be used. This approach helps by:

  1. Simplifying both the JS and shader code
  2. Reducing jank caused by shader recompilation when the user adjusts render bounds

While this approach may add some render cost, I believe the added cost is justifiable because:

  • The added cost is minimal (one multiply + add).
  • The added costs appear in the simpler view. The more expensive volumetric view is not affected.
  • The new approach avoids shader recompilation, which is the most significant cost when making bounds adjustments.

Testing plan

  1. Verify that the Voxel???Shape specs run successfully
  2. Load the development/Voxels Sandcastle and test different bounds and clipping in the Voxel Inspector

Author checklist

  • I have submitted a Contributor License Agreement
  • I have added my name to CONTRIBUTORS.md
  • [ ] I have updated CHANGES.md with a short summary of my change Code cleanup, does not affect user-facing behavior
  • [ ] I have added or updated unit tests to ensure consistent code coverage Code cleanup, does not affect test results or coverage
  • I have update the inline documentation, and included code examples where relevant
  • I have performed a self-review of my code

RayShapeIntersection intersectRegularWedge(in Ray ray, in vec2 minMaxAngle, in bool convex)
{
// Compute intersections with the two planes
// TODO: the sign of the normals looks backwards?? But it works for convex == false
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this TODO be addressed?

vec4 entry = inOrder ? intersect1 : intersect2;
vec4 exit = inOrder ? intersect2 : intersect1;

bool enterFromOutside = (entry.w >= 0.0) == (dot(ray.pos.xy, entry.xy) < 0.0);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nitpick: Maybe assign the boolean values to intermediate variables to avoid any confusion or accidental assignments.

Copy link
Contributor

@ggetz ggetz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @jjhembd! Tests are passing and the Sandcastle examples are working well.

@jjhembd
Copy link
Contributor Author

jjhembd commented Feb 21, 2024

Thanks for the feedback @ggetz! I looked through that TODO, and realized I had been confused about the meaning of a "regular wedge" -- it is actually a negative volume in this case, where the wedge is "outside" the shape bounds.

I updated the naming and comments in that function to avoid confusion.

@ggetz
Copy link
Contributor

ggetz commented Feb 22, 2024

Looks good to me! Thanks @jjhembd!

@ggetz ggetz merged commit 8ed89db into main Feb 22, 2024
9 checks passed
@ggetz ggetz deleted the voxel-cleanup branch February 22, 2024 14:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants