Raytrace plots #2655

gridley · 2023-08-18T18:38:11Z

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:

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 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:

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

Checklist

gridley · 2023-08-26T17:32:24Z

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

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

pshriwise

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

include/openmc/particle_data.h

include/openmc/plot.h

include/openmc/geometry.h

src/plot.cpp

include/openmc/dagmc.h

shimwell · 2023-08-31T15:48:15Z

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

gridley · 2023-09-09T22:06:01Z

""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 · 2023-09-12T09:35:00Z

""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 · 2023-10-05T15:00:57Z

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 · 2023-10-05T15:06:17Z

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 · 2023-10-05T15:07:02Z

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 · 2023-10-05T15:17:25Z

Alright, now it should be good for review!

pshriwise

A few more comments from a mini-review. DAGMC PR imminent once I rebase my test branch onto this....

src/plot.cpp

gridley · 2023-10-07T01:36:52Z

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

gridley · 2023-10-07T01:37:30Z

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

gridley · 2023-10-10T19:24:22Z

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

pshriwise · 2023-10-10T20:06:17Z

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 · 2023-10-10T21:04:57Z

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

pshriwise · 2023-10-20T21:52:33Z

src/plot.cpp

+void Ray::trace()
+{
+  int first_surface_ = -1;    // surface first passed when entering the model
+  first_inside_model_ = true; // false after entering the model


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 · 2023-10-20T21:52:42Z

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

pshriwise · 2023-10-20T21:53:35Z

src/plot.cpp

+    // 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) {


This might be another issue that could be solved by using our particle transport mechanisms for surface intersection and crossing.

pshriwise · 2023-10-20T21:54:55Z

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 · 2023-10-20T21:56:28Z

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 · 2024-02-25T17:25:54Z

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

pshriwise · 2024-02-26T21:43:52Z

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

Can do! Thanks for updating it

gridley · 2024-10-14T18:16:08Z

what do you think can be improved on the images Patrick?

…le visual changes

gridley · 2024-12-07T05:19:03Z

On that last commit: I too have had some issues where the hash test fails on the images but they otherwise look indistinguishable. I believe this is because some rounding comes into play when multiplying the RGB values by a double then converting back to unsigned chars.

pshriwise · 2024-12-07T18:13:33Z

On that last commit: I too have had some issues where the hash test fails on the images but they otherwise look indistinguishable. I believe this is because some rounding comes into play when multiplying the RGB values by a double then converting back to unsigned chars.

Yeah with these plotter hashes I'm not too worried about it, especially in this case where the selection of pixel color values aren't "discrete" (if you'll allow a little abuse of that term).

pshriwise

A few final line comments, and one on DAGMC arguments from me but then I think this is good! Thanks for your patience @gridley!

src/dagmc.cpp

pshriwise · 2024-12-08T05:31:51Z

openmc/plots.py

@@ -204,14 +204,14 @@ def voxel_to_vtk(voxel_file: PathLike, output: PathLike = 'plot.vti'):
        Path of the .vti file produced
    """

-    # imported vtk only if used as vtk is an option dependency
+#imported vtk only if used as vtk is an option dependency


Something funky happened to the comment indentation in this section

openmc/plots.py

pshriwise · 2024-12-08T05:35:05Z

openmc/plots.py

+
+    @opaque_domains.setter
+    def opaque_domains(self, x):
+        cv.check_type('opaque domains', x, Iterable)


Probably worth using check_iterable_type here to ensure that items in the iterable are valid domains.

I don't think that makes sense. They can be ints, materials, or cells as long as all are consistent and match the current coloring scheme. The _check_domains_consistent_with_color_by handles that.

What do you think?

I added a comment to explain this.

True, but the user won't know that until the object's XML is generated. The checks in the property setters are generally for more immediate feedback. Here, I'm thinking that a more coarse can be provided for early indication of a problem -- a simple check for any of the valid domain identifiers accepted in _check_domains_consistent_with_color_by

Suggested change

cv.check_type('opaque domains', x, Iterable)

cv.check_type('opaque domains', x, Iterable)

cv.check_iterable_type('opaque domains', x, (Integral, openmc.Material, openmc.Cell))

include/openmc/dagmc.h

pshriwise · 2024-12-13T18:48:06Z

src/plot.cpp

 {
-  constexpr double scoot = 1e-5;
-  double min_dist = {INFINITY};
+  double min_dist {INFINITY};
  auto coord = p.coord(0);


Let's use a different variable name here so it doesn't shadow the GeometryState method.

What method are you talking about?

gavin@Gavins-MBP openmc % grep -r "min_dist" include gavin@Gavins-MBP openmc %

Whoops! Comment was off by a line.

The variable coord on the line below is what I was referring to. It doesn't shadow really (we'd know if it did), but I think a different name like root_coord would be more clear and reduce confusion with the Particle::coord method.

pshriwise · 2024-12-17T15:33:17Z

@gridley I made some changes in my fork to resolve the compiler errors here, but I'm recalling now that changing the Surface::normal signature was pretty pervasive and that overriding the Surface::reflect definition in the DAGSurface class mitigated this issue. In short, the proposal here is to return this to status quo. Thoughts?

pshriwise · 2025-01-02T19:36:37Z

@gridley I made some changes in my fork to resolve the compiler errors here, but I'm recalling now that changing the Surface::normal signature was pretty pervasive and that overriding the Surface::reflect definition in the DAGSurface class mitigated this issue. In short, the proposal here is to return this to status quo. Thoughts?

I went ahead and reverted those signatures for now @gridley. I've made #3239 so we can return to this topic separately.

DAGMC models seem to be working quite well now!

I went through a few old models and created some more DAGMC-based images for a CSG-clipped tokamak model and a human phantom model (kinda creepy lookin).

Just in case this PR didn't have enough pictures in it, here they are:

gridley · 2025-01-02T19:37:38Z

Hey Patrick, thanks for adding those changes! I meant to get around to those updates but have been attending to some other things. Beautiful pics 🤩

pshriwise · 2025-01-02T19:59:41Z

@gridley no worries! I'm glad to be getting to this finally. I've just added some XML roundtrip testing for the PhongPlot class. There were some attributes that weren't getting picked up on read. Could this be true for other derivatives of RayTracePlot?

gridley · 2025-01-03T17:49:06Z

Oh wow, I'm surprised to hear that. I could have sworn I double-checked on the roundtripping for exports and imports. It's certainly possible. I'll give it another look.

pshriwise · 2025-01-06T21:31:28Z

src/boundary_condition.cpp

@@ -37,7 +37,7 @@ void VacuumBC::handle_particle(Particle& p, const Surface& surf) const

 void ReflectiveBC::handle_particle(Particle& p, const Surface& surf) const
 {
-  Direction u = surf.reflect(p.r(), p.u(), &p);
+  Direction u = surf.reflect(p.r(), p.u());


Just caught this. I think the last parameter still needs to be here for DAGMC surfaces.

…the position rotation methods so we they can be used with other containers.

…GSurface for the GeometryState pointer

gridley added the Plots Changes related to plotting of models and results label Aug 18, 2023

pshriwise requested changes Aug 29, 2023

View reviewed changes

pshriwise reviewed Aug 29, 2023

View reviewed changes

include/openmc/dagmc.h Outdated Show resolved Hide resolved

pshriwise mentioned this pull request Sep 7, 2023

Refactor of translation from mesh handle to index for DAGMC geometry. #2687

Merged

3 tasks

gridley force-pushed the raytrace_plots branch from 8a6da9b to d1ddaf7 Compare September 9, 2023 21:53

gridley marked this pull request as ready for review September 9, 2023 22:01

pshriwise requested changes Oct 5, 2023

View reviewed changes

src/plot.cpp Outdated Show resolved Hide resolved

src/plot.cpp Show resolved Hide resolved

src/plot.cpp Outdated Show resolved Hide resolved

pshriwise reviewed Oct 20, 2023

View reviewed changes

gridley mentioned this pull request Oct 22, 2023

Geometron #2744

Merged

5 tasks

gridley force-pushed the raytrace_plots branch 2 times, most recently from f58d45e to d8fc057 Compare February 25, 2024 17:24

pshriwise added 3 commits December 6, 2024 10:38

Merge remote-tracking branch 'upstream/develop' into raytrace_plots

fa8bed6

Providing rng. Correcting comment indentation

ab5480d

Updating plot_projection results. Verified that there are no noticeab…

d5d04fb

…le visual changes

pshriwise requested changes Dec 13, 2024

View reviewed changes

gridley added 2 commits December 13, 2024 14:25

fix comment indentation in plots.py

b6bad23

address review comments

c536898

pshriwise added 2 commits January 2, 2025 12:35

Reverting to old Surface::reflect signatures

79614ed

C++ style

c24e37a

pshriwise mentioned this pull request Jan 2, 2025

DAGMC diffuse reflection implementation #3239

Open

Adding XML roundtrip test for PhongPlot

61fdaed

pshriwise reviewed Jan 6, 2025

View reviewed changes

pshriwise added 11 commits January 6, 2025 15:34

Adding methods operating on the camera class for clarity. Templating …

0ae7612

…the position rotation methods so we they can be used with other containers.

Correct test name

9fbf479

Simplifying logic in Plots.from_xml_element

8989eb7

Passing particle pointer to Surface::reflect, re-adding Ensures in DA…

027deee

…GSurface for the GeometryState pointer

Correcting comment indentation

7c8f988

Test PhongPlot repr

d76ffe5

Style update

b202ee5

Adding description of the camera_to_model matrix axes

90012e5

Adding tests for ProjectionPlot roundtrip

58cce6d

Style update

d4ed3de

rm whitespace

6280602

	cv.check_type('opaque domains', x, Iterable)
	cv.check_type('opaque domains', x, Iterable)
	cv.check_iterable_type('opaque domains', x, (Integral, openmc.Material, openmc.Cell))

Raytrace plots #2655

Are you sure you want to change the base?

Raytrace plots #2655

Conversation

gridley commented Aug 18, 2023 • edited Loading

Description

Checklist

gridley commented Aug 26, 2023

pshriwise left a comment

Choose a reason for hiding this comment

shimwell commented Aug 31, 2023

gridley commented Sep 9, 2023

pshriwise commented Sep 12, 2023

gridley commented Oct 5, 2023

pshriwise commented Oct 5, 2023

gridley commented Oct 5, 2023 • edited Loading

gridley commented Oct 5, 2023

pshriwise left a comment

Choose a reason for hiding this comment

gridley commented Oct 7, 2023

gridley commented Oct 7, 2023

gridley commented Oct 10, 2023

pshriwise commented Oct 10, 2023 • edited Loading

cfichtlscherer commented Oct 10, 2023

Choose a reason for hiding this comment

shimwell commented Oct 20, 2023

Choose a reason for hiding this comment

pshriwise commented Oct 20, 2023

pshriwise commented Oct 20, 2023

gridley commented Feb 25, 2024

pshriwise commented Feb 26, 2024

gridley commented Oct 14, 2024

gridley commented Dec 7, 2024

pshriwise commented Dec 7, 2024

pshriwise left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pshriwise commented Dec 17, 2024

pshriwise commented Jan 2, 2025

gridley commented Jan 2, 2025

pshriwise commented Jan 2, 2025

gridley commented Jan 3, 2025

Choose a reason for hiding this comment

gridley commented Aug 18, 2023 •

edited

Loading

gridley commented Oct 5, 2023 •

edited

Loading

pshriwise commented Oct 10, 2023 •

edited

Loading