-
Notifications
You must be signed in to change notification settings - Fork 279
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 rendering for octrees #2610
Volume rendering for octrees #2610
Conversation
After quite a long time working on this, I think this should be ready for review! There's probably ways to make all this cleaner, so don't hesitate! It is also probably still missing some tests, @matthewturk any suggestion regarding the way to go? Duplicate all 3d rendering tests already happening with grid-based datasets? |
There seems to be a compilation issue caused with an older version of gcc used on jenkins. Any guess how to solve this? |
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.
Didn't have time for a full review, I stopped at 8/40 files. I only have a handful of minor suggestions for those, this seems very well written as usual :)
I made some high-res images of a high-resolution galaxy (dx ~ 30pc). It turns out that I had some Fortran/C ordering issues (now fixed in be79cb0) which significantly improves the quality of the image (see images before (top) and after (bottom) the fix). We can still some grid effects, but they seem "normal" to me. PerformancesParallelized over 40 cores, each image takes about ~3s to generate (for a resolution of 1024x1024). Eye-candyHere's some eye-candy. This shows a galaxy at z=2, with resolution 30pc. Took 10min to render (200frames, 3,168,583 cells). |
Converted this to draft because we first need to merge #2425! |
7a39833
to
1817b1b
Compare
47b367a
to
06b8faf
Compare
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 can not honestly pretend I read and reviewed the core cpp part, but for the parts I did read, here are my suggestions. Great stuff as always :)
RE = np.array([1, 1, 1], dtype=np.float64) | ||
depth = data_source.ds.parameters["levelmax"] | ||
|
||
self.octree = CythonOctreeRayTracing(LE, RE, depth) |
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.
tbh I find the naming of those classes a bit odd. CythonOctreeRayTracing
sounds like the Cython version of OctreeRayTracing
, not of a component of the class. But maybe I just don't understand enough of this, it could be absolutely fine.
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 is very odd. We should discuss better options. @matthewturk @munkm what's your opinion? The issue is that my implementation is multi-layered, there's a C++ implementation that's wrapped in a cython file that's wrapped in a python file... So we could probably get rid of most of these layers, but which ones?
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 changed the naming so that the python file is named octree_raytracing.py
, the cython wrapper is _octree_raytracing.p{xd,yx}
and the C++ file is _octree_raytracing.hpp
.
ds = data_source.ds | ||
|
||
xyz = np.stack( | ||
[data_source[_].to("unitary").value for _ in "x y z".split()], axis=-1 |
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.
[data_source[_].to("unitary").value for _ in "x y z".split()], axis=-1 | |
[data_source[key].to("unitary").value for key in "xyz"], axis=-1 |
yt/utilities/lib/image_samplers.pyx
Outdated
@@ -67,6 +71,10 @@ cdef class ImageSampler: | |||
*args, **kwargs): | |||
cdef int i | |||
|
|||
self.volume_method = kwargs.pop('volume_method', None) | |||
if self.volume_method and self.volume_method not in ('KDTree', 'Octree'): |
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.
if self.volume_method and self.volume_method not in ('KDTree', 'Octree'): | |
if self.volume_method not in ('KDTree', 'Octree'): |
ds = data.ds | ||
|
||
xyz = np.stack( | ||
[data[_].to("unitary").value for _ in "x y z".split()], axis=-1 |
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.
[data[_].to("unitary").value for _ in "x y z".split()], axis=-1 | |
[data[key].to("unitary").value for key in "xyz"], axis=-1 |
return image | ||
self.zbuffer = zbuffer | ||
self.set_sampler(camera) | ||
assert self.sampler is not None |
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.
assert self.sampler is not None | |
if self.sampler is None: | |
raise RuntimeError(message...) |
The failing tests are real; I'll have to dig into it. |
/rebase |
/rebase
|
This reverts commit c768d9d for off_axis_projection.py
Call function Only support Ndim=3 Drop usage of "using" and rely on typedef instead Use uint8_t instead of u_char Fix platform in appveyor Using uint64_t instead of uint
f8dc942
to
748e7ef
Compare
This is now ready to go! |
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 think we should do it. One slight reservation is that we're changing the appveyor image,b ut that's not necessarily a big deal.
I am actually unsure this is still necessary. Let me try using the old one. |
I reverted the change of the appveyor image, as it was not necessary anymore! |
@yt-fido test this please |
This PR adds support for doing volume rendering for octree-based datasets (e.g. RAMSES). It is built on top of #2425 to provide vertex-centred data (not yet wired though).
It follows the same idea as #2442, but relying instead with an independent octree ray tracer. This has the advantage of making the code much simpler, but generates a bit of code repetition and requires rebuilding an octree.
Currently, it supports the yt API for volume rendering.
Features
Demo
Notes on performance
The entire script above takes about 20s to run from end to end. Casting the rays and integrating the transfer function actually only takes 4s on 8 cores (with hyperthreading), 5s on 4 cores and 16s on a single core. The dataset consists of 1,749,455 cells.