-
Notifications
You must be signed in to change notification settings - Fork 5
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
base: main
Are you sure you want to change the base?
Conversation
…der functionality based on dataset geometry
for more information, see https://pre-commit.ci
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. |
Working on docs now... |
added grid outlines now yt_idv_sp_vr_grid_display.mp4 |
Assuming tests still pass, I think this is ready! I just updated the docs. and also added an additional pre-processor directive, |
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 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, |
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 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. |
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.
Why 0.2 rad / 11 deg?
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... 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 |
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.
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; |
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'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.
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.
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.
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 hopefully you're right! will look a bit more closely.
for more information, see https://pre-commit.ci
@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: ![]() 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. |
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 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. |
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... |
So this is a verion of #64 that seems to work pretty well with a few caveats.
This version does 2 things differently:
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 usesload_uniform_grid
withnproc=128
, ifnproc
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.