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

Volume-mesh intersections with view position texture #259

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

frasercl
Copy link
Collaborator

@frasercl frasercl commented Nov 12, 2024

review time: small (~5min)

#237 added proper intersections between volumes and meshes. This works in the fragment shader by:

  1. sampling the depth buffer
  2. converting the depth value and uv to a position in view space
  3. converting that position back to object space
  4. terminating a ray early if it would run past that object space position

Getting volume-mesh intersections right was motivated in part by plans to incorporate volume rendering into Simularium. But Simularium's initial render pass generates a buffer of view space positions, which lets us skip step two above. This change adds support for using that view space buffer for intersections.

@frasercl frasercl requested a review from a team as a code owner November 12, 2024 00:19
@frasercl frasercl requested review from toloudis and ascibisz and removed request for a team November 12, 2024 00:19
Copy link
Contributor

@toloudis toloudis left a comment

Choose a reason for hiding this comment

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

Good stuff! Looks like you didn't even have to manipulate the value. I recommend a clarifying comment and maybe a variable rename.

float depth = texture2D(textureDepth, vUv).r;
// Sample the depth/position texture
// This is EITHER a view space position or a depth value we'll convert to a view space position
vec4 meshViewPos = texture2D(textureDepth, vUv);
Copy link
Contributor

@toloudis toloudis Nov 12, 2024

Choose a reason for hiding this comment

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

Nit/optional: naming is a little weird here as this value might not be in view space yet

vec4 meshProj = vec4(vUv * 2.0 - 1.0, depth * 2.0 - 1.0, 1.0);
vec4 meshView = inverseProjMatrix * meshProj;
vec4 meshObj = inverseModelViewMatrix * vec4(meshView.xyz / meshView.w, 1.0);
if (hasDepthValue || (usingPositionTexture == 1 && meshViewPos.a > 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.

I think this could use an extra comment up above to say what r,g,b,a of the position texture are expected to represent. You might even mention that this is a data format contract with Simularium viewer.

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