You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
Diffuse reflection on DAGMC surfaces should be working in principle, but there's an optimization that's present in DAGMCSurface::reflect due to it's specialized implementation that isn't present for Surface::diffuse_reflect currently.
Both of these methods make a call to DAGMCSurface::normal which can optimize this call if the GeometryState::history (the record of previous surface triangles intersected) is present by using the most-recently intersected triangle. Right now this only occurs for ::normal. It would also be best if these methods were consistent.
As @gridley noted in #2655, this does make the signature kind of odd upon first sight as there's no immediate reason why the entire particle state should be required for computing the normal of a surface at a given location.
There are a few approaches to solving this:
add a specialized implementation of DAGMCSurface::diffuse_normal and include a GeometryState argument for that method (least invasive).
In addition to 1, update signatures of the Surface::normal method to allow a GeometryState pointer to be passed optionally. All surfaces can then rely on the same Surface::reflect/Surface::diffuse_reflect implementation.
Create normal signatures that take only a GeometryState pointer (an addition to the ones that exist currently) that is used in updated versions of the reflect/diffuse_reflect methods.
Compatibility
Any signature changes suggested here would include optional arguments or additional signatures.
The text was updated successfully, but these errors were encountered:
It makes perfect sense when you word it this way! My vote is for number 2. Just having an argument that's const GeometryState * = nullptr would be great. There wasn't any documentation in the code about why that was there before, so it'd be good to add that this is used for optimizations on mesh-based surfaces.
Yeah I'm leaning toward #2 as well. I'll need to review the code a bit to make sure we can consistently choose the right position from the particle state. And agreed, there should be more documentation around the GeometryState::history in general.
Description
Diffuse reflection on DAGMC surfaces should be working in principle, but there's an optimization that's present in
DAGMCSurface::reflect
due to it's specialized implementation that isn't present forSurface::diffuse_reflect
currently.Both of these methods make a call to
DAGMCSurface::normal
which can optimize this call if theGeometryState::history
(the record of previous surface triangles intersected) is present by using the most-recently intersected triangle. Right now this only occurs for::normal
. It would also be best if these methods were consistent.As @gridley noted in #2655, this does make the signature kind of odd upon first sight as there's no immediate reason why the entire particle state should be required for computing the normal of a surface at a given location.
There are a few approaches to solving this:
add a specialized implementation of
DAGMCSurface::diffuse_normal
and include aGeometryState
argument for that method (least invasive).In addition to 1, update signatures of the
Surface::normal
method to allow aGeometryState
pointer to be passed optionally. All surfaces can then rely on the sameSurface::reflect
/Surface::diffuse_reflect
implementation.Create
normal
signatures that take only aGeometryState
pointer (an addition to the ones that exist currently) that is used in updated versions of thereflect
/diffuse_reflect
methods.Compatibility
Any signature changes suggested here would include optional arguments or additional signatures.
The text was updated successfully, but these errors were encountered: