-
Notifications
You must be signed in to change notification settings - Fork 484
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
Raytrace plots #2655
base: develop
Are you sure you want to change the base?
Raytrace plots #2655
Conversation
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.
Very excited about this. Between the projection plots and this new capability we're building up to a fairly advanced visualization capability! Thanks for driving this effort @gridley!
This is a first-round review. I think we're at a place where we want to make sure we're really happy with the design so we can extend it further. I'm going to try this out with some DAGMC geometries too just to see how far I can get.
8a6da9b
to
d1ddaf7
Compare
""Some files failed the formatting check! See job summary and file annotations for more info" && exit 1" How can I look at these annotations? Huh. Thought I was running clang-format... |
You can see them in the run summary by clicking on the 🏠 icon in the left sidebar
Are you running the same version as CI (15)? We've seen a few others get hit by that recently. |
We should be good for another review here, now! I removed the exception crap to just use |
I think I still owe you a mini-PR to handle DAGMC geometry. I'll try to take care of that today. Hopefully I can review it soon. |
Oh sweet! OK I just realized that the |
Alright, now it should be good for review! |
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.
A few more comments from a mini-review. DAGMC PR imminent once I rebase my test branch onto this....
@cfichtlscherer has some super nice stuff too. Are you able to give any sneak peeks Chris? |
|
I've been seeing that too here: https://github.com/openmc-dev/openmc/actions/runs/6471480840/job/17575480291 I'll take a look soon. |
Yeah I used it to plot some weapon models. I think it would be so nice to have it in the main code. |
src/plot.cpp
Outdated
void Ray::trace() | ||
{ | ||
int first_surface_ = -1; // surface first passed when entering the model | ||
first_inside_model_ = true; // false after entering the model |
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.
Would performing a exhaustive_find_cell
check to start and calling RayTracePlot::advance_to_boundary_from_void
if the result is false
allow us to simplify this loop a bit?
Speaking of RayTracePlot::advance_to_boundary_from_void
, is there any reason it isn't a method of the Ray
class itself?
wondering if metallic or surface texture or reflective surfaces would be possible |
// We cannot detect it in the outer loop, and it only matters here, so | ||
// that's why the error handling is a little different than for a lost | ||
// ray. | ||
if (i_surface() == -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.
This might be another issue that could be solved by using our particle transport mechanisms for surface intersection and crossing.
Hey @gridley. Sorry it's taken me a while to get back to you. I talked with @jtramm about this PR and what overlap there might be with his random ray implementation. For the time being, we settled on separate implementations b/c, from what we could tell, there wouldn't be a lot of overlap aside from the definition of the ray itself (position and direction). We also identified a couple of updates here that could be of benefit. I made note of them in file comments. One overarching note was that the introduction of the |
This has been resolved in #2729. We'll need to update this branch to get CI passing again. |
f58d45e
to
d8fc057
Compare
OK @pshriwise, just resolved any conflicts, could you give this one more review? |
Can do! Thanks for updating it |
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.
Just as awesome as the first time I looked at it! I had a good time playing around with it today.
Making note of some future work for now:
- focal plane setting
- pixel position generator classes for perspective and orthographic projections
- a camera object
- create a
GeometryState::advance
method - add a method for line segment entries based on
plot_.color_by
to reduce code duplication
I wouldn't say any of these are blocking, aside perhaps from the inverse_rotation
line comment, but I wanted to write them down while they're in my head.
plots.append(Plot.from_xml_element(e)) | ||
elif plot_type == 'slice': |
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.
While I think this is an improvement, does it break backward compatibility? I can't recall how long the Plot.type
attribute has been around...
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.
Pretty sure it does not, no.
OK, pretty sure I got all your comments addressed @pshriwise! |
This automatic clang format is driving me nuts. I am running clang format, but they're different versions and have some tiny whitespace differences. I've dropped some pretty unnecessary comments in the last commit where I can't figure out how to make the two versions of clang-format happy. |
Yeah the C++ formatter check can be pretty frustrating sometimes, sorry. It's plagued others as well. It would be great if we could configure a bot to provide the updates as suggestions or something. I'll give that some thought. |
f72a699
to
fb352d0
Compare
As an update, I am back on getting this merge-able. The previous implementation was MUCH slower than it should have been due to not leveraging neighbor lists while in the model-internal cell-to-cell tracking phase. I have refactored the code to use two separate ray tracing loops: one which handles the approach up to the model from outside and another that uses neighbor_list_find_cell while traversing the actual model. I'm hoping that this will not only give a performance benefit, but in addition fix the weird DAGMC results this has been giving. I suspect it's related to ray history or something, why I had found it not working previously. |
I know @pshriwise you had implemented some anti-aliasing, but to get that same effect we could generate the image at a higher resolution then interpolate down. I think the computational cost would probably be about the same either way. |
Description
This PR implements 3D plotting with Phong shading. I factored out the basic ray creation from a camera into an abstract base class called
RayTracePlot
. Then,ProjectionPlot
implements the logic for moving rays all the way through a geometry in order to wireframe cells or materials of interest and color them in an x-ray-like view. The newPhongPlot
(happy to rename) lets the user specify a few materials or cells which they would like to have be opaque, with everything else in the model being transparent. By default it assumes that a light is placed at the camera's location, but the light can optionally be moved.As an example, here is a reminder of what a
ProjectionPlot
looks like:A Phong plot on a similar geometry looks kinda neat, and could certainly be a helpful tool for visualization on intricately structured stuff--especially for communicating what a model looks like to other people.
You can adjust the fraction of diffuse light to give a slightly different look. By default, it uses 10% diffuse lighting, 90% lighting from the light source with single scattered isotropic scattering off surfaces.
You can move the light source location, which can make a nice dramatic effect:
I was frustrated that we have to create a full
Particle
object just for geometric operations. As a result, I factored out the geometry state ofParticle
into a base class calledGeometron
. This comes in handy foropenmc_find_cell
as we no longer have to initialize the cross section caches and all that other physics cache data just to do a cell search. Similarly theGeometron
is instead used for ray tracing in the plotter, now. It could also be used for the implementation of the random ray method which I hear is coming along. My last note on theGeometron
is that all the geometric operations takeGeometron
references or pointers, notParticle
. Of course, simply passing aParticle
pointer works as usual.This does not invoke any polymorphism, so there will be zero performance overhead as a result of this, or perhaps even performance gains since the compiler will know that only a smaller subset of the data can potentially be accessed.Polymorphism is used for error handling in geometric routines.Geometron
sfatal_error
on erroneous leakage from the model, butParticle
s save the leaked ID and then the simulation continues.In this direction, there was one larger change I had to make for error handling in geometry. It seems that raising an exception is the right thing to do here, with calling code defining the exception handling. Code inParticle
now will resort tomark_as_lost
for lost stuff, but in the plotter, you just get a plain old exception that stops the program. Previously it would just say "particle with ID=-1 leaked" or something like that. For MOC or other things like that, of course you would just want the program to exit if you leak a ray, so I hope other people like this error handling approach in the geometry now. Also, the compiler knows that exceptions happen with low frequency, so there might be a slight performance gain from the improved branch prediction in those few places where we previously had just calledmark_as_lost
.Lastly, in the
ParticleData
class, I moved the comments to the setters/getters rather than on the private variables, since this is the public-facing API that people would actually want to read.There are a few other things I want to do before merging this. It is a WIP. Of course I need to add the new plot and its options to the python API. Really not looking forward to the XML boilerplate. Also I want to factor the plot ray tracer into its own RayKernel-object. I'm thinking it'd be nice to create a ray that just has some lambdas attached to it like
on_intersection_
oron_cross_surface_
to avoid repeating it inPhongPlot
andProjectionPlot
. Of course this could help with parallelization and could be used in a random ray implementation. Lastly I'd like to add a 3D version ofUniverse.plot
that automatically picks a camera position based on the bounding box size and some simple position argument like "top left" or "front left".Also, I need to figure out how to rotate surface normals in transformed nested universes in order to get the direction to the light in the root universe. That's going to be a fun one.Oh wait... there's one more thing. The ProjectionPlot camera rays were not uniform in pixel space, which did not create distortion effects for things that are far away, but are distorted for things up close. The old plotter would make straight surfaces appear curved, e.g:
With this PR, it correctly maps pixels onto ray directions, making it look a lot better:
Checklist
Add Universeplot3d
method for easily obtained 3D visualization in ipython notebooksPlot a slice of the BEAVRS core with pins and core structure shown as solids.