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

spherical volume rendering refactored #159

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

Conversation

chrishavlin
Copy link
Contributor

@chrishavlin chrishavlin commented Dec 3, 2024

So this is a verion of #64 that seems to work pretty well with a few caveats.

This version does 2 things differently:

  1. It calculates the cartesian bounding boxes of every block initially (using code from ENH: cartesian cutting planes through non-cartesian geometries  yt#4847), and passes that info down to the shaders as additional vertex attributes.
  2. And then those cartesian bounding boxes are used in the ray tracing shader for calculating ray intersections. At each sample point along the ray, the spherical coordinates are calculated and used to sample the texture if it falls in the actual spherical element.

The main caveat is that you need a fairly large number of sample points for the ray tracing since intersection with the cartesian bounding box does not guarantee intersection with the enclosed spherical element.

EDIT: cartsian bbox calculation now works with big elements (via a recursive division when its needed). Also, at least one thing that should be fixed before this goes in is that the cartesian bounding box calculation was designed for fairly small spherical elements and breaks down when blocks span whole quadrants of the spherical coordinate system. e.g., the new example script uses load_uniform_grid with nproc=128, if nproc is smaller (less than 8 or so), the blocks get too big and the bounding box calculation breaks down and results start to get wonky (or nonexistant). So, should fix that...

Finally -- I decided to go with a new PR rather than modifying #64 because I'm abandoning the complicated intersection checks with the underlying spherical element bounds. While I think that approach could work (and could be added on here...), I think it's worth getting the simpler version here merged first and wanted to preserve #64 in its current state to make that attempt easier in the future.

sochowski and others added 30 commits December 2, 2021 18:06
@chrishavlin
Copy link
Contributor Author

Latest push includes some new logic to limit the options that show up in the GUI so that unsupported shaders and scene components are not shown. I also bumped the maximum sample factor when the data is in spherical coordinates.

@chrishavlin
Copy link
Contributor Author

Working on docs now...

@chrishavlin
Copy link
Contributor Author

chrishavlin commented Feb 14, 2025

Just adding an example of a dataset with refined grids now that #171 is fixed (code over here)

yt_idv_sp_vr_amr_full.mp4

@chrishavlin
Copy link
Contributor Author

latest push adds block outlines via the CurveRendering data object
snap_0000
snap_0001

@chrishavlin
Copy link
Contributor Author

added grid outlines now

yt_idv_sp_vr_grid_display.mp4

@chrishavlin
Copy link
Contributor Author

Assuming tests still pass, I think this is ready!

I just updated the docs. and also added an additional pre-processor directive, NONCARTESIAN_GEOM that is used for directive checks in the rendering pipeline that would be common to any non-cartesian coordinate system. So the existing SPHERICAL_GEOM one is now only used where the coordiante conversion is defined in the fragment shader. This makes it easier to add support for other coordinate systems.

Copy link
Member

@matthewturk matthewturk left a comment

Choose a reason for hiding this comment

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

This is amazing! I've left a few comments, including one about possible artifacts. But I think it's generally good to go!

if f0 < f1: return f0
return f1

cdef (np.float64_t, np.float64_t, np.float64_t) _spherical_to_cartesian(np.float64_t r,
Copy link
Member

Choose a reason for hiding this comment

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

I did not realize you could return tuples with acceptable performance! This is great.

error code: 0 for success, 1 if something went wrong...

Note: this function is recursive! If either angular width is above a cutoff (of
0.2 radians or about 11 degrees), the element will be recursively divided.
Copy link
Member

Choose a reason for hiding this comment

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

Why 0.2 rad / 11 deg?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I... don't remember. Best guess looking at it now is that when playing with what to use, 10 degrees worked well without adding too much extra computation, but I wanted it to be a nice number in radians?

@@ -139,6 +163,18 @@ def draw(self, scene, program):
GL.glDrawArrays(GL.GL_POINTS, tex_ind * each, each)

def _set_uniforms(self, scene, shader_program):
if self.data._yt_geom_str == "spherical":
axis_id = self.data.data_source.ds.coordinates.axis_id
Copy link
Member

Choose a reason for hiding this comment

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

Great catch -- this is the kind of thing I usually miss doing.

vec3 tr = (right_edge - camera_pos)*idir;
vec3 tl, tr, dx_cart;
#ifdef NONCARTESIAN_GEOM
tl = (left_edge_cart - camera_pos)*idir;
Copy link
Member

Choose a reason for hiding this comment

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

I'm not 100% sure but I think that this may be the source of the (mild!) artifacts we see when we viz ("index", "ones") with projective integration. I attached just-off-axis and almost-on-axis to demonstrate.

To be clear, these artifacts are not deal breakers! But I'm wondering if this is a source of the artifacts, and if we could use more precise values to compute them.

spherical-vr-off-edge
spherical-vr-on-axis

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it could be! and i've banged my head on many things looking for the source of those artifacts but it didn't occur to me that it might a precision issue.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

so hopefully you're right! will look a bit more closely.

@chrishavlin
Copy link
Contributor Author

chrishavlin commented Mar 6, 2025

@matthewturk I've been looking at that artifact issue on and off. and I'm not positive but I think it might be a limitation of the current implementation that uses the intersection of the ray with the cartesian bounding box of each block as the ray start/end: if you imagine discretizing a ray, then the number of points along the ray that fall within a sphere will depend on the curvature of the sphere and the stepsize along the ray and there are situations that can lead to artifacts. I played around with the idea in 2D over in this notebook, here's a figure that plots the integrated distance along a 2D ray that falls within a circle for rays that are parallel to the y axis along with the derivative of that distance with respect to the x position of the ray:

Screenshot 2025-03-06 at 11 36 51 AM

I think those little bumps may be the artifact we're seeing: there are spots where the integrated distance increases depending on how the circle's radius lines up with the discretized ray. Not sure there's a fix for this beyond working out the full intersection calculations.

@matthewturk
Copy link
Member

Let's imagine we did do the full intersection calculations. Wouldn't we be able to do this only for the regions that actually intersect a ray -- i.e., only in the fragment shader, and so potentially with a reduced number of calculations? The bounding boxes are how we compute which fragments to compute, but potentially couldn't we inside the fragment shader also use that to discard fragments, or compute the actual intersections?

Also, if there are computations (potentially, for instance, inverse trig functions, square roots, etc) that we can bundle we could do them inside a compute shader step, or otherwise pre-compute and store them.

@chrishavlin
Copy link
Contributor Author

Let's imagine we did do the full intersection calculations. Wouldn't we be able to do this only for the regions that actually intersect a ray -- i.e., only in the fragment shader, and so potentially with a reduced number of calculations? The bounding boxes are how we compute which fragments to compute, but potentially couldn't we inside the fragment shader also use that to discard fragments, or compute the actual intersections?

yup! and that's what i've done in my attempts -- use the cartesian box check to initially discard fragments before doing the full intersection calculations. let me dig up my last attempt which I think was almost working and remind myself where things were going wrong...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants