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

Raytrace plots #2655

Open
wants to merge 57 commits into
base: develop
Choose a base branch
from
Open

Raytrace plots #2655

wants to merge 57 commits into from

Conversation

gridley
Copy link
Contributor

@gridley gridley commented Aug 18, 2023

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 new PhongPlot (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:

orthographic_example1

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.

phong

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.

phong_diffuse

You can move the light source location, which can make a nice dramatic effect:

phong_move_light

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 of Particle into a base class called Geometron. This comes in handy for openmc_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 the Geometron 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 the Geometron is that all the geometric operations take Geometron references or pointers, not Particle. Of course, simply passing a Particle 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. Geometrons fatal_error on erroneous leakage from the model, but Particles 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 in Particle now will resort to mark_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 called mark_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_ or on_cross_surface_ to avoid repeating it in PhongPlot and ProjectionPlot. 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 of Universe.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:

example1

With this PR, it correctly maps pixels onto ray directions, making it look a lot better:

example1

Checklist

  • I have performed a self-review of my own code
  • I have run clang-format on any C++ source files (if applicable)
  • I have followed the style guidelines for Python source files (if applicable)
  • I have made corresponding changes to the documentation (if applicable)
  • I have added tests that prove my fix is effective or that my feature works (if applicable)
  • Add PhongPlot to python API
  • Factor out ray tracing logic into RayTracePlot (it is duplicated right now)
  • Add Universe plot3d method for easily obtained 3D visualization in ipython notebooks
  • Plot a slice of the BEAVRS core with pins and core structure shown as solids.
  • Make it work with rotated/translated subuniverses

@gridley gridley added the Plots Changes related to plotting of models and results label Aug 18, 2023
@gridley
Copy link
Contributor Author

gridley commented Aug 26, 2023

Just sharing some new stuff. Here's a plot of some TRISO:

image

And here's an example of shadows working correctly with the classic CSG test object:

csg_background

Copy link
Contributor

@pshriwise pshriwise left a 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.

include/openmc/geometry.h Outdated Show resolved Hide resolved
include/openmc/geometry.h Outdated Show resolved Hide resolved
include/openmc/particle_data.h Outdated Show resolved Hide resolved
include/openmc/plot.h Show resolved Hide resolved
include/openmc/geometry.h Outdated Show resolved Hide resolved
src/plot.cpp Outdated Show resolved Hide resolved
src/plot.cpp Outdated Show resolved Hide resolved
src/plot.cpp Outdated Show resolved Hide resolved
src/plot.cpp Outdated Show resolved Hide resolved
src/plot.cpp Outdated Show resolved Hide resolved
include/openmc/dagmc.h Outdated Show resolved Hide resolved
@shimwell
Copy link
Member

Just made one of these plots, very nice interface and easy to use.
p

@gridley
Copy link
Contributor Author

gridley commented Sep 9, 2023

""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...

@pshriwise
Copy link
Contributor

""Some files failed the formatting check! See job summary and file annotations for more info" && exit 1"

You can see them in the run summary by clicking on the 🏠 icon in the left sidebar
https://github.com/openmc-dev/openmc/actions/runs/6133354411?pr=2655

How can I look at these annotations? Huh. Thought I was running clang-format...

Are you running the same version as CI (15)? We've seen a few others get hit by that recently.

@gridley
Copy link
Contributor Author

gridley commented Oct 5, 2023

We should be good for another review here, now! I removed the exception crap to just use mark_as_lost and fatal_error instead. However, we need to use a virtual here in order to retain the correct behavior: die with an error for Geometron, but save the particle info and continue for Particle.

@pshriwise
Copy link
Contributor

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.

@gridley
Copy link
Contributor Author

gridley commented Oct 5, 2023

Oh sweet! OK I just realized that the mark_as_lost does actually have to be virtual to get the right behavior, and I have to move the id_ data to Geometron, but that's not really a big deal. (so gonna push that ina sec)

@gridley
Copy link
Contributor Author

gridley commented Oct 5, 2023

Alright, now it should be good for review!

Copy link
Contributor

@pshriwise pshriwise left a 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....

src/plot.cpp Outdated Show resolved Hide resolved
src/plot.cpp Show resolved Hide resolved
src/plot.cpp Outdated Show resolved Hide resolved
@gridley
Copy link
Contributor Author

gridley commented Oct 7, 2023

Just sharing this nice image I made for the docs. Imagine how many voxels you'd need to create this previously!
phong_triso

@gridley
Copy link
Contributor Author

gridley commented Oct 7, 2023

@cfichtlscherer has some super nice stuff too. Are you able to give any sneak peeks Chris?

@gridley
Copy link
Contributor Author

gridley commented Oct 10, 2023

test_evaluate_legendre is failing for me in CI. No way it's related to the recent commits... Any ideas?

@pshriwise
Copy link
Contributor

pshriwise commented Oct 10, 2023

test_evaluate_legendre is failing for me in CI. No way it's related to the recent commits... Any ideas?

I've been seeing that too here: https://github.com/openmc-dev/openmc/actions/runs/6471480840/job/17575480291

I'll take a look soon.

@cfichtlscherer
Copy link
Contributor

@cfichtlscherer has some super nice stuff too. Are you able to give any sneak peeks Chris?

Yeah I used it to plot some weapon models. I think it would be so nice to have it in the main code.

warheads_models_01102023

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
Copy link
Contributor

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?

@shimwell
Copy link
Member

wondering if metallic or surface texture or reflective surfaces would be possible

src/plot.cpp Outdated
// 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) {
Copy link
Contributor

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.

@pshriwise
Copy link
Contributor

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 Geometron is a great contribution in and of itself. Would you be willing to break that out into a separate PR so we can get that merged more quickly? I think it would speed up review iteration and allow us to focus more on the plotting aspects of the work only.

@pshriwise
Copy link
Contributor

test_evaluate_legendre is failing for me in CI. No way it's related to the recent commits... Any ideas?

This has been resolved in #2729. We'll need to update this branch to get CI passing again.

@gridley gridley mentioned this pull request Oct 22, 2023
5 tasks
@gridley gridley force-pushed the raytrace_plots branch 2 times, most recently from f58d45e to d8fc057 Compare February 25, 2024 17:24
@gridley
Copy link
Contributor Author

gridley commented Feb 25, 2024

OK @pshriwise, just resolved any conflicts, could you give this one more review?

@pshriwise
Copy link
Contributor

OK @pshriwise, just resolved any conflicts, could you give this one more review?

Can do! Thanks for updating it

@pshriwise
Copy link
Contributor

pshriwise commented Jan 11, 2025

@gridley I made some adjustments to the RayTracePlot class to remove some of the attributes that are already present through inheritance of the GeometryState class (i_surface_ --> GeometryState::surface() and dist_ --> GeometryState::boundary(). As a result, there were a couple of attributes that became local variables and after a little more tweaking they could be removed altogether.

It's important to note that i_surface_, an index into the model::surfaces vector, was replaced with GeometryState::surface_, which is a surface token (expected to be signed and non-zero when valid). Some of the return values reflect this now by using 0 as an invalid value (works for tokens) instead of an invalid value of -1 (works for an index).

Once this was complete I realized that the advance_to_boundary_from_void method may come in really handy for delta tracking, so I moved it onto the GeometryState object. I think there's the possibility that we could refactor the Particle and GeometryState classes to the point where we could rely on the same set of calls to move particles and transfer rays, but it's definitely not the time to dive into that here. Just an exciting prospect for the future 😄

There is one significant behavioral change to note: the condition for flipping the surface normal. Previously, the normal was flipped based on the sign of the surface token and whether or not the surface was of DAGMC make, but this isn't entirely robust as it can depend on the relationship of the surface token and ray orientation. When flipping the camera and light location over the xy-plane in the phong_move_light test I found that the shading on the bottom of the model didn't look like what I'd expect.

phong_move_light_orig

With the light source below the model I'd expect a brighter bottom surface. I think a more general approach is to flip the normal if the dot product of the surface normal and the ray direction is greater than one. This ensures that the normal is opposed to the ray direction, which is what we want in these kind of plots to determine if the reflection/diffuse reflection will potentially reach the light source. After implementing this, the image seemed to look better to me. It is also independent of the geometry type, so we don't have to check for DAGMC geometry in this case anymore. After that update, the image looked more like what I was thinking it would

phong_move_light_new

It maybe goes without saying that the test results needed to be updated due to small differences in pixel color, but upon inspection none of them changed significantly. Let me know if I'm off-base with any of this @gridley and I'd be happy to make further adjustments!

@pshriwise
Copy link
Contributor

@gridley because my submission of #3260 was responsible for the conflicts here I went ahead and addressed them. I also updated the data members here to use the new SURFACE_NONE constant where appropriate.

Because the TrackSegment was relying on an absolute value of the surface token I went ahead and changed that to a surface index instead to remove an additional std::abs on a surface token that doesn't result in an index. It doesn't need to in this case, but I thought it would improve clarity. Let me know if you feel otherwise.

@gridley
Copy link
Contributor Author

gridley commented Jan 17, 2025

Makes sense to me!!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Plots Changes related to plotting of models and results
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants