Add Toroid surface by osanstrong · Pull Request #2158 · celeritas-project/celeritas

osanstrong · 2025-12-15T00:43:23Z

Add z aligned, elliptical toroid surface.

Adds the surface class Toroid, representing an elliptical toroid with ellipse radii a and b, revolved at major radius r around a z-aligned axis at an origin o:

       ___   _________   ___
     /  |  \           /     \
    /   b   \         /       \
   |    |    |       |         |
   |-a--+    |   o-----r--+    |
   |         |       |         |
    \       /         \       /
     \     /           \     /
       ⁻⁻⁻   ⁻⁻⁻⁻⁻⁻⁻⁻⁻   ⁻⁻⁻

This PR does not fully integrate the Toroid surface, only adding the Toroid class (using prior Ferrari quartic solver implementation), an accompanying test file, and required SurfaceIO details.

…yet)

github-actions · 2025-12-15T00:50:52Z

Test summary

254 files 410 suites 9s ⏱️
1 318 tests 1 304 ✅ 14 💤 0 ❌
1 352 runs 1 348 ✅ 4 💤 0 ❌

Results for commit 109dd84.

♻️ This comment has been updated with latest results.

osanstrong · 2025-12-15T00:52:26Z

Update: both questions resolved
Two immediate questions: I wrote this referencing #1295 , but #1295 used CELER_NOT_IMPLEMENTED for surface_type() to avoid fully integrating the surface all at once. I considered using this, but wasn't able to replicate the use without it throwing compile-time errors (the function surface_type() is elsewhere defined using CELER_CONSTEXPR_FUNCTION but it seems like CELER_NOT_IMPLEMENTED is not constexpr itself. at least from compiler error below, so I'm not sure how the initial Involute implementation used that)

[2/4] Building CXX object test/orange/CMakeFiles/orange_surf_Toroid.dir/surf/Toroid.test.cc.o
FAILED: test/orange/CMakeFiles/orange_surf_Toroid.dir/surf/Toroid.test.cc.o
/usr/bin/c++ -DGTEST_LINKED_AS_SHARED_LIBRARY=1 -I/home/oss/ORNL/celeritas/celeritas/src -I/home/oss/ORNL/celeritas/celeritas/build/include -I/home/oss/ORNL/celeritas/celeritas/test -I/home/oss/ORNL/celeritas/celeritas/build/test -isystem /home/oss/spack/var/spack/environments/celeritas/.spack-env/view/include -g -std=c++17 -fPIE -MD -MT test/orange/CMakeFiles/orange_surf_Toroid.dir/surf/Toroid.test.cc.o -MF test/orange/CMakeFiles/orange_surf_Toroid.dir/surf/Toroid.test.cc.o.d -o test/orange/CMakeFiles/orange_surf_Toroid.dir/surf/Toroid.test.cc.o -c /home/oss/ORNL/celeritas/celeritas/test/orange/surf/Toroid.test.cc
In file included from /home/oss/ORNL/celeritas/celeritas/src/corecel/cont/detail/SpanImpl.hh:12,
                 from /home/oss/ORNL/celeritas/celeritas/src/corecel/cont/Span.hh:16,
                 from /home/oss/ORNL/celeritas/celeritas/src/corecel/cont/SpanIO.hh:12,
                 from /home/oss/ORNL/celeritas/celeritas/src/corecel/cont/ArrayIO.hh:14,
                 from /home/oss/ORNL/celeritas/celeritas/src/orange/surf/Toroid.hh:12,
                 from /home/oss/ORNL/celeritas/celeritas/src/orange/surf/Toroid.cc:7,
                 from /home/oss/ORNL/celeritas/celeritas/test/orange/surf/Toroid.test.cc:7:
/home/oss/ORNL/celeritas/celeritas/src/orange/surf/Toroid.hh: In static member function ‘static constexpr celeritas::SurfaceType celeritas::Toroid::surface_type()’:
/home/oss/ORNL/celeritas/celeritas/src/corecel/Assert.hh:186:41: error: call to non-‘constexpr’ function ‘void celeritas::throw_runtime_error(celeritas::RuntimeErrorDetails&&)’
  186 |         ::celeritas::throw_runtime_error({         \
      |         ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~
  187 |             WHICH,                                 \
      |             ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  188 |             WHAT,                                  \
      |             ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  189 |             COND,                                  \
      |             ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  190 |             __FILE__,                              \
      |             ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  191 |             __LINE__,                              \
      |             ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  192 |         })
      |         ~~
/home/oss/ORNL/celeritas/celeritas/src/corecel/Assert.hh:245:5: note: in expansion of macro ‘CELER_RUNTIME_THROW’
  245 |     CELER_RUNTIME_THROW(::celeritas::RuntimeError::not_impl_err_str, WHAT, {})
      |     ^~~~~~~~~~~~~~~~~~~
/home/oss/ORNL/celeritas/celeritas/src/orange/surf/Toroid.hh:63:9: note: in expansion of macro ‘CELER_NOT_IMPLEMENTED’
   63 |         CELER_NOT_IMPLEMENTED("runtime toroid");
      |         ^~~~~~~~~~~~~~~~~~~~~
/home/oss/ORNL/celeritas/celeritas/src/corecel/Assert.hh:382:19: note: ‘void celeritas::throw_runtime_error(celeritas::RuntimeErrorDetails&&)’ declared here
  382 | [[noreturn]] void throw_runtime_error(RuntimeErrorDetails&&);
      |                   ^~~~~~~~~~~~~~~~~~~
ninja: build stopped: cannot make progress due to previous errors.

As for the second question, I'm still new to linking & header files: how are the surfaces set up so you only need to include <name>.hh? (E.g. SimpleQuadric.test.cc: #include "orange/surf/SimpleQuadric.hh"). When I set up my test file, the compiler couldn't find the constructor definition in Toroid.cc if I included #include "orange/surf/Toroid.hh", and I had to instead include #include "orange/surf/Toroid.cc", but this feels like a cop-out solution/I'm missing something, since the other surfaces don't do that.

…sub not being declared inline properly

…ot integrated)

sethrj

This is really amazing work @osanstrong ! I have a couple of comments so far from a quick review, but I think what you've got is practically perfect.

sethrj · 2025-12-15T14:20:46Z

src/orange/surf/Toroid.hh

+/*!
+ * Shorthand for subtracting vector b from vector a
+ */
+CELER_FUNCTION Real3 Toroid::sub(Real3 const& a, Real3 const& b)


This and sq can be CELER_CONSTEXPR_FUNCTION; you can also include ArrayOperator.hh (which we discourage by default because it's easy to write surprisingly inefficient expressions like (x = a - b + c * d)) and just use a - b.

sethrj · 2025-12-15T14:21:40Z

src/orange/surf/Toroid.hh

+{
+    auto [x0, y0, z0] = sub(pos, origin_);
+
+    real_type d = std::sqrt(sq(x0) + sq(y0));


Use hypot from corecel/math/Algorithms.hh

Suggested change

real_type d = std::sqrt(sq(x0) + sq(y0));

real_type d = hypot(x0, y0);

sethrj · 2025-12-15T14:29:10Z

src/orange/surf/Toroid.hh

+    real_type A0 = 4 * sq(r_);
+    real_type B0 = sq(r_) - sq(a_);


We try to keep to the variable formatting standards even for math equations (no uppercase), and it looks like you might even be able to easily eliminate these two variables since they're only used once (q, u)

sethrj · 2025-12-15T14:29:25Z

src/orange/surf/Toroid.hh

+
+    real_type f = 1 - sq(az);
+    real_type g = f + p * sq(az);
+    real_type l = 2 * (x0 * ax + y0 * ay);


l is also dangerous since it looks like 1 and I 🙃

sethrj · 2025-12-15T14:31:36Z

src/orange/surf/Toroid.hh

+    if (val < 0)
+        return SignedSense::inside;
+    else if (val > 0)
+        return SignedSense::outside;
+    else
+        return SignedSense::on;


From orange/SenseUtils.hh, use real_to_sense

Suggested change

if (val < 0)

return SignedSense::inside;

else if (val > 0)

return SignedSense::outside;

else

return SignedSense::on;

return real_to_sense(val);

elliottbiondo

Looks great @osanstrong! Just a couple of things.

elliottbiondo · 2025-12-15T17:57:42Z

src/orange/surf/Toroid.hh

+ * \f[
+ *   (x^2 + y^2 + p*y^2 + B_0) - A_0 * (x^2 + y^2) = 0
+ * \f]
+ * where \f[p = a^2/b^2, A_0 = 4*r^2, and B_0 = (r^2-a^2)\f].


For inline LaTeX use \f$ ... \f$

Let's also add this to the user docs

elliottbiondo · 2025-12-15T17:58:02Z

src/orange/surf/Toroid.cc

+namespace celeritas
+{
+//---------------------------------------------------------------------------//
+/**


Suggested change

/**

/*

Suggested change

/**

/*!

elliottbiondo · 2025-12-15T17:58:16Z

src/orange/surf/Toroid.cc

+{
+//---------------------------------------------------------------------------//
+/**
+ * Construct toroid from origin point and radii


Suggested change

* Construct toroid from origin point and radii

* Construct toroid from origin point and radii.

elliottbiondo · 2025-12-15T18:00:54Z

src/orange/surf/Toroid.cc

+/**
+ * Construct toroid from origin point and radii
+ *
+ * \param origin 3d origin of the toroid.


Suggested change

* \param origin 3d origin of the toroid.

* \param Origin 3d origin of the toroid.

No, origin is the name of the parameter, which should be lowercase

elliottbiondo · 2025-12-15T18:02:53Z

src/orange/surf/Toroid.cc

+    , a_(ellipse_xy_radius)
+    , b_(ellipse_z_radius)
+{
+    CELER_EXPECT(major_radius > 0);


It doesn't matter too much, but usually we validate the member data rather than the arguments

The reason I validate data is it'll also catch the error or forgetting to initialize it :)

elliottbiondo · 2025-12-15T18:04:17Z

src/orange/surf/Toroid.hh

+ * around a central axis. This shape can be used in everything from pipe bends
+ * to tokamaks in fusion reactors. Possesses a major radius r, and ellipse
+ * radii a and b, as shown in the below diagram:
+ *     ___   _________   ___


Put ASCII art in a \verbatim block

elliottbiondo · 2025-12-15T18:23:57Z

src/orange/surf/Toroid.hh

+    CELER_FUNCTION real_type ellipse_z_radius() const { return b_; }
+
+    //! View of data for type-deleted storage
+    CELER_FUNCTION StorageSpan data() const { return {&origin_[0], 4}; }


The length should be 6: i.e., 3 for origin_ then r_, a_, b_.

@sethrj this is non-standard C++ and not actually guaranteed to work. Seems like a short function for manually serializing these would be better.

I think it's legal and we use it for the other shapes, but I agree an insert mechanism like the recent work with @amandalund is better. But for now let's stick with this.

Yeah it's fine here. For posterity, I don't think it's legal in the general case; see the layout section here:

https://en.cppreference.com/w/cpp/language/data_members.html

"Alignment requirements may necessitate padding between members, or after the last member of a class."

Since these members are are all standard-sized POD, specifically doubles, the compiler is not going to add padding, but it strictly speaking doing so would be legal.

Well it seems you're right; this is an old class and I had just been working in C so perhaps I misunderstood a C rule. Okeydoke, I'll add a construction utility to the to-do list 🙃

elliottbiondo · 2025-12-15T18:37:57Z

src/orange/surf/Toroid.hh

+ * Calculate the coefficients of the polynomial corresponding to the given
+ * ray's intersections with the toroid.
+ *
+ * Written referencing Graphics Gems II.


Add the citation key arvo_graphics-gems_1995

elliottbiondo · 2025-12-15T18:40:46Z

test/orange/surf/Toroid.test.cc

+    }
+}
+
+Real3 add(Real3 const a, Real3 const b)


Use ArrayOperator.hh

osanstrong added 12 commits December 11, 2025 19:08

Started Toroid header file with brief description

b3e38da

Added declarations for toroid functions and 'tor' SurfaceType

ed902ef

Aded toroid cc file

200205c

Added data span getter and span constructor

9eb4649

Started adding test file for Toroid (end of day commit, not finished …

7629f3a

…yet)

Added ray intersection test cases for toroid

68f8950

Unintegrated toroid for now from type lists

48548c6

Integrated toroid tests, added sense calculation

efc49d3

Added ray intersection algorithm to toroid class

762e40d

Removed print statements from debugging ray intersection

4f86208

Fixed typo in storage span declaraiton (said 6 instead of correct 4)

4e27209

Added Toroid IO functions

310b59e

osanstrong requested review from elliottbiondo and sethrj as code owners December 15, 2025 00:43

osanstrong added 2 commits December 14, 2025 20:14

Added Toroid.cc to CMakeLists, fixed static private functions sq and …

7bdfd75

…sub not being declared inline properly

Made surface type more clearly not implemented yet (since Toroid is n…

109dd84

…ot integrated)

sethrj added enhancement New feature or request orange Work on ORANGE geometry engine labels Dec 15, 2025

sethrj reviewed Dec 15, 2025

View reviewed changes

sethrj mentioned this pull request Dec 15, 2025

Implement torus #1829

Open

5 tasks

elliottbiondo requested changes Dec 15, 2025

View reviewed changes

	real_type d = std::sqrt(sq(x0) + sq(y0));
	real_type d = hypot(x0, y0);

	* Construct toroid from origin point and radii
	* Construct toroid from origin point and radii.

	* \param origin 3d origin of the toroid.
	* \param Origin 3d origin of the toroid.

Conversation

osanstrong commented Dec 15, 2025

Add z aligned, elliptical toroid surface.

Uh oh!

github-actions bot commented Dec 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Test summary

Uh oh!

osanstrong commented Dec 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sethrj left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

elliottbiondo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

github-actions bot commented Dec 15, 2025 •

edited

Loading

osanstrong commented Dec 15, 2025 •

edited

Loading