95 implement embree runner by jmaack24 · Pull Request #96 · NatLabRockies/SolTrace

jmaack24 · 2026-01-13T18:07:34Z

Implementation of EmbreeRunner to utilize Intel's Embree ray tracing library (sold separately...).

…nner validation tests to include virtual stage

…nners

Copilot

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

Copilot

Pull request overview

Copilot reviewed 67 out of 68 changed files in this pull request and generated 8 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-01-15T21:05:57Z

+        // increment position by tiny amount to get off the element if 
+        // tracing to the same element


Fixed typo: 'increment' should start with capital 'I' to match comment style, and the comment should end with a period.

Suggested change

// increment position by tiny amount to get off the element if

// tracing to the same element

// Increment position by tiny amount to get off the element if

// tracing to the same element.

Copilot · 2026-01-15T21:05:57Z

+                    float (&min_coord_global)[3],
+                    float (&max_coord_global)[3]);
+
+} // namespace embree_helper


Namespace comment is incorrect. Should be '// namespace SolTrace::EmbreeRunner' to match the actual namespace declaration at line 8.

Suggested change

} // namespace embree_helper

} // namespace SolTrace::EmbreeRunner

Copilot · 2026-01-15T21:05:58Z

@@ -0,0 +1,90 @@
+#include "find_element_hit.hpp"


Incorrect include file name. Should be 'find_element_hit_embree.hpp' instead of 'find_element_hit.hpp'.

Suggested change

#include "find_element_hit.hpp"

#include "find_element_hit_embree.hpp"

Copilot · 2026-01-15T21:05:58Z

+TEST(Aperture, BoundingBox)
+{
+}


Empty test case should either be implemented or removed. The test name suggests it should test aperture bounding box functionality but has no implementation.

Copilot · 2026-01-15T21:05:58Z

+        assert(is_approx(x_minmax[0], -r, 1e-6));
+        assert(is_approx(x_minmax[1], r, 1e-6));


Using assert for input validation in production code. Consider using a proper error handling mechanism or documenting that these are debug-only checks. Asserts are typically disabled in release builds.

Suggested change

assert(is_approx(x_minmax[0], -r, 1e-6));

assert(is_approx(x_minmax[1], r, 1e-6));

if (!is_approx(x_minmax[0], -r, 1e-6))

throw std::invalid_argument("Cylinder::bounding_box: x_minmax[0] must be approximately -radius");

if (!is_approx(x_minmax[1], r, 1e-6))

throw std::invalid_argument("Cylinder::bounding_box: x_minmax[1] must be approximately +radius");

Copilot · 2026-01-15T21:05:59Z

                EXPECT_NE(rr->get_event(iidx), RayEvent::ABSORB);
                EXPECT_NE(rr->get_event(iidx), RayEvent::EXIT);
+
+                if(rr->get_event(iidx) == RayEvent::ABSORB)


Missing space after 'if' keyword. Should be 'if (rr->get_event(iidx) == RayEvent::ABSORB)'.

Copilot · 2026-01-15T21:05:59Z

            this->number_of_elements += el->get_number_of_elements();
            el->set_reference_element(this);
+            // Mark any added elements as virtual if needed
+            if(this->is_virtual())


Missing space after 'if' keyword. Should be 'if (this->is_virtual())'.

Suggested change

if(this->is_virtual())

if (this->is_virtual())

Copilot · 2026-01-15T21:05:59Z

+        // TODO: Fix ? This is really only the top half of the cylinder
+        //       Clyinder breaks the model since it is a mulit-valued fuction: each
+        //       x values produces two z values Returning only the positive root


Multiple spelling errors: 'Clyinder' should be 'Cylinder', 'mulit-valued fuction' should be 'multi-valued function', and missing period after 'root'.

Suggested change

// TODO: Fix ? This is really only the top half of the cylinder

// Clyinder breaks the model since it is a mulit-valued fuction: each

// x values produces two z values Returning only the positive root

// TODO: Fix ? This is really only the top half of the cylinder.

// Cylinder breaks the model since it is a multi-valued function: each

// x value produces two z values. Returning only the positive root.

Copilot

Pull request overview

Copilot reviewed 67 out of 68 changed files in this pull request and generated no new comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

…windows CI

…cy code build by default

Copilot

Pull request overview

Copilot reviewed 70 out of 71 changed files in this pull request and generated 5 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-01-22T18:14:51Z


+## Steps for Building SolTrace (work in progress)
+
+SolTrace has been updated to use multiple ray tracing engines in addition to the prior implementation. Currently, there is no graphical user interface (it is underdevelopment).


The phrase "it is underdevelopment" should be "it is under development" (missing space).

Suggested change

SolTrace has been updated to use multiple ray tracing engines in addition to the prior implementation. Currently, there is no graphical user interface (it is underdevelopment).

SolTrace has been updated to use multiple ray tracing engines in addition to the prior implementation. Currently, there is no graphical user interface (it is under development).

Copilot · 2026-01-22T18:14:52Z

+        // Increment position by tiny amount to get off the element if 
+        // tracing to the same element.
+        PosRayElement[0] = PosRayElement[0] + 1.0e-4 * CosRayElement[0];
+        PosRayElement[1] = PosRayElement[1] + 1.0e-4 * CosRayElement[1];
+        PosRayElement[2] = PosRayElement[2] + 1.0e-4 * CosRayElement[2];


In the comment on line 126, "Increment position by tiny amount to get off the element" - this hardcoded offset of 1.0e-4 could cause issues with very small or very large geometries. Consider making this epsilon value scale-dependent or configurable based on the element's characteristic dimensions.

Copilot · 2026-01-22T18:14:52Z

+        // TODO: Fix ? This is really only the top half of the cylinder.
+        //       Cylinder breaks the model since it is a multi-valued function: each


The typo "Clyinder" should be "Cylinder" in the comment.

Copilot · 2026-01-22T18:14:52Z

+    - name: Install embree (Windows)
+      if: matrix.os == 'windows-latest'
+      shell: pwsh
+      run: |
+        $embreeVersion = "4.4.0"
+        $embreeUrl = "https://github.com/embree/embree/releases/download/v$embreeVersion/embree-$embreeVersion.x64.windows.zip"
+        $embreeZip = "$env:TEMP\embree.zip"
+        $embreeDir = "C:\embree"
+
+        Write-Host "Downloading Embree $embreeVersion..."
+        Invoke-WebRequest -Uri $embreeUrl -OutFile $embreeZip
+
+        Write-Host "Extracting Embree..."
+        Expand-Archive -Path $embreeZip -DestinationPath $embreeDir -Force
+
+        # $embreeRoot = Get-ChildItem -Path $embreeDir -Directory -Filter "embree*" | Select-Object -First 1
+        $embreeCMake = Join-Path $embreeDir "lib\cmake\embree-$embreeVersion"
+        $embreeLib = Join-Path $embreeDir "bin"
+
+        # Write-Host "Embree root: $($embreeRoot.FullName)"
+        Write-Host "Embree CMake: $embreeCMake"
+        Write-Host "Embree Lib: $embreeLib"
+
+        echo "embree_DIR=$embreeCMake" >> $env:GITHUB_ENV
+        echo "$embreeLib" >> $env:GITHUB_PATH


The Windows Embree installation script extracts to a directory path that includes the version number in the zip, but the code assumes a specific directory structure. The commented-out lines suggest there was an attempt to find the embree directory dynamically. This could break if Embree's directory structure changes between versions. Consider making the path detection more robust.

Copilot · 2026-01-22T18:14:53Z

+        // Expand bounding boxes slightly to account for float precision
+        const float expand = 1e-3f;
+        x_minmax[0] -= expand;
+        x_minmax[1] += expand;
+        y_minmax[0] -= expand;
+        y_minmax[1] += expand;
+        z_minmax[0] -= expand;
+        z_minmax[1] += expand;


The bounding box expansion with a hardcoded value of 1e-3f could be problematic for different scales. For very small geometries this could be too large, and for very large geometries it might not be sufficient. Consider making this relative to the bounding box dimensions or element scale.

coveralls · 2026-01-22T19:26:32Z

Pull Request Test Coverage Report for Build 21417800998

Details

1022 of 1268 (80.6%) changed or added relevant lines in 23 files are covered.
14 unchanged lines in 7 files lost coverage.
Overall coverage increased (+0.2%) to 88.052%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
coretrace/simulation_runner/native_runner/thread_manager.cpp	2	3	66.67%
coretrace/simulation_runner/native_runner/trace.cpp	8	9	88.89%
coretrace/simulation_data/surface.hpp	21	23	91.3%
coretrace/simulation_runner/native_runner/native_runner.cpp	16	18	88.89%
coretrace/simulation_runner/native_runner/sun_to_primary_stage.cpp	0	2	0.0%
coretrace/simulation_data/aperture.hpp	0	3	0.0%
coretrace/simulation_runner/embree_runner/embree_runner.cpp	36	40	90.0%
coretrace/simulation_data/utilities.hpp	7	12	58.33%
coretrace/simulation_data/surface.cpp	142	148	95.95%
coretrace/simulation_runner/native_runner/determine_interaction_type.cpp	17	26	65.38%

Files with Coverage Reduction	New Missed Lines	%
coretrace/simulation_data/aperture.hpp	1	74.19%
coretrace/simulation_runner/native_runner/native_runner.cpp	1	92.17%
coretrace/simulation_runner/native_runner/sphere_calculator.hpp	1	0.0%
coretrace/simulation_runner/native_runner/surface_intersection_calculator.hpp	1	50.0%
coretrace/simulation_runner/native_runner/trace.cpp	1	95.0%
coretrace/simulation_data/aperture.cpp	3	69.32%
coretrace/simulation_runner/native_runner/native_runner_types.cpp	6	93.84%

Totals
Change from base Build 21258149251:	0.2%
Covered Lines:	6721
Relevant Lines:	7633

💛 - Coveralls

…explicitly mention adding embree dll path to system path in install instructions

qualand

Thanks for putting this together. Everything is working on my system. I did notice large sections of commented legacy code. Do we want to remove these in the future?

jmaack24 · 2026-01-27T22:28:01Z

Thanks for putting this together. Everything is working on my system. I did notice large sections of commented legacy code. Do we want to remove these in the future?

Do you remember where? I thought I removed most of the commented out code that didn't need to stick around. I can take a look and clean it up here.

qualand · 2026-01-27T22:36:52Z

Thanks for putting this together. Everything is working on my system. I did notice large sections of commented legacy code. Do we want to remove these in the future?

Do you remember where? I thought I removed most of the commented out code that didn't need to stick around. I can take a look and clean it up here.

I think the main ones that sticked out to me are:
coretrace/simulation_runner/native_runner/determine_interaction_type.cpp
coretrace/simulation_runner/embree_runner/embree_runner.hpp (under private)
coretrace/simulation_runner/embree_runner/bbox_calculator.cpp

jmaack24 · 2026-01-27T23:04:20Z

Thanks for putting this together. Everything is working on my system. I did notice large sections of commented legacy code. Do we want to remove these in the future?

Do you remember where? I thought I removed most of the commented out code that didn't need to stick around. I can take a look and clean it up here.

I think the main ones that sticked out to me are: coretrace/simulation_runner/native_runner/determine_interaction_type.cpp coretrace/simulation_runner/embree_runner/embree_runner.hpp (under private) coretrace/simulation_runner/embree_runner/bbox_calculator.cpp

So the stuff in determine_interaction_type.cpp and bbox_calculator.cpp are code that may be needed/useful in the future when implementing other features. (E.g., the commented out sections of determine_interaction_type.cpp are for user defined transmissivity and reflectivity tables).

The code in embree_runner.hpp can be removed. You should see the commit for that in a moment or two.

taylorbrown75

The embree runner works on my legacy GUI branch. It would be interesting to compare speed performance between embree and native and see how it compares with the legacy code. The embree code is about 33% faster than native with power tower optimizations on for the power-tower-surround_singlefacet test, which is slower than I expected, but that may be because the native runner is faster now thanks to the refactorization.

qualand · 2026-01-28T15:40:00Z

The embree runner works on my legacy GUI branch. It would be interesting to compare speed performance between embree and native and see how it compares with the legacy code. The embree code is about 33% faster than native with power tower optimizations on for the power-tower-surround_singlefacet test, which is slower than I expected, but that may be because the native runner is faster now thanks to the refactorization.

Do we have a power tower surround multi-facet test? We should see a greater speed up with more elements in the problem geometry.

jmaack24 · 2026-01-28T15:58:05Z

The embree runner works on my legacy GUI branch. It would be interesting to compare speed performance between embree and native and see how it compares with the legacy code. The embree code is about 33% faster than native with power tower optimizations on for the power-tower-surround_singlefacet test, which is slower than I expected, but that may be because the native runner is faster now thanks to the refactorization.

Do we have a power tower surround multi-facet test? We should see a greater speed up with more elements in the problem geometry.

Bothe the NativeRunner and EmbreeRunner ValidationTest2 runs the input file Power-tower-surround_singlefacet.stinput which has 6283 elements and traces 50k rays (this is the same test that Taylor is talking about). On my Mac I get the following times for a release build

Native Runner Power Tower OFF: 10782.5 ms
Native Runner Power Tower ON: 312.95 ms
Embree Runner: 87.7166 ms

So the EmbreeRunner is still ~120 times faster than NativeRunner with Power Tower OFF and ~3.5 times faster than NativeRunner with Power Tower ON. So at least on Mac with a release build, there is still a significant speedup for Embree.

jmaack24 added 5 commits January 8, 2026 16:12

Initial structure for EmbreeRunner

516450c

Bounding box calculations added to surface and aperture structures

6a4c716

Add tests for aperture bounding box

98dedec

Separate logging from thread manager

9d5e69c

Change embree runner to inherit from native runner

aa82d59

jmaack24 self-assigned this Jan 13, 2026

jmaack24 linked an issue Jan 13, 2026 that may be closed by this pull request

Implement Embree Runner #95

Closed

jmaack24 added 9 commits January 13, 2026 11:16

Fix CI builds

944e179

Add tests for surface bounding box

7ea1104

Add logger to trace_embree function

6b47441

Add EmbreeRunner validation test and another short unit test

5ec6524

Set virtual flag for reading input files; update embree and native ru…

fc02507

…nner validation tests to include virtual stage

Add unit tests for thread manager

2d2b811

Move embree initialization to part of runner setup

e55fad9

Commonize interaction type determination between embree and native ru…

e4465f2

…nners

EmbreeRunner multi-threading implementation

9b52531

jmaack24 marked this pull request as ready for review January 15, 2026 19:40

jmaack24 requested a review from Copilot January 15, 2026 19:40

Copilot AI reviewed Jan 15, 2026

View reviewed changes

Copilot started reviewing on behalf of jmaack24 January 15, 2026 20:05 View session

jmaack24 requested a review from Copilot January 15, 2026 21:03

Copilot started reviewing on behalf of jmaack24 January 15, 2026 21:04 View session

Copilot AI reviewed Jan 15, 2026

View reviewed changes

Address copilot comments

a5971e7

jmaack24 requested a review from Copilot January 15, 2026 21:18

Copilot started reviewing on behalf of jmaack24 January 15, 2026 21:19 View session

Copilot AI reviewed Jan 15, 2026

View reviewed changes

jmaack24 requested review from qualand and taylorbrown75 January 15, 2026 21:46

Update CI and coverage work flows to build Embree

4a911fa

jmaack24 added 8 commits January 21, 2026 13:01

Fix work flow errors

878f6eb

Change windows build to use downloaded binary

6fe5cce

Attempt to fix windows build

73919c2

Fix windows CI hopefully. Maybe.

cf6f3c0

Pass along number of threads value to embree; another attempt to fix …

5050963

…windows CI

Fix linux compiler warning; attempt whatever to fix windows ci

c4ff4bf

Try again

079ff23

Updates to README for Embree install and SolTrace build; disable lega…

17afb27

…cy code build by default

jmaack24 requested a review from Copilot January 22, 2026 18:09

Copilot started reviewing on behalf of jmaack24 January 22, 2026 18:09 View session

Copilot AI reviewed Jan 22, 2026

View reviewed changes

Merge branch 'develop' into 95-implement-embree-runner

8487343

Change cmake include directories to relative paths in embree runner; …

cf52049

…explicitly mention adding embree dll path to system path in install instructions

qualand approved these changes Jan 27, 2026

View reviewed changes

Remove commented code in embree runner header

1515bf6

taylorbrown75 approved these changes Jan 27, 2026

View reviewed changes

qualand merged commit a48ba85 into develop Jan 28, 2026
11 checks passed

qualand deleted the 95-implement-embree-runner branch January 28, 2026 15:55

		// increment position by tiny amount to get off the element if
		// tracing to the same element

	} // namespace embree_helper
	} // namespace SolTrace::EmbreeRunner

	#include "find_element_hit.hpp"
	#include "find_element_hit_embree.hpp"

		assert(is_approx(x_minmax[0], -r, 1e-6));
		assert(is_approx(x_minmax[1], r, 1e-6));

-        assert(is_approx(x_minmax[0], -r, 1e-6));
-        assert(is_approx(x_minmax[1], r, 1e-6));
+        if (!is_approx(x_minmax[0], -r, 1e-6))
+            throw std::invalid_argument("Cylinder::bounding_box: x_minmax[0] must be approximately -radius");
+        if (!is_approx(x_minmax[1], r, 1e-6))
+            throw std::invalid_argument("Cylinder::bounding_box: x_minmax[1] must be approximately +radius");


		## Steps for Building SolTrace (work in progress)

		SolTrace has been updated to use multiple ray tracing engines in addition to the prior implementation. Currently, there is no graphical user interface (it is underdevelopment).

		// TODO: Fix ? This is really only the top half of the cylinder.
		// Cylinder breaks the model since it is a multi-valued function: each

Conversation

jmaack24 commented Jan 13, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Jan 15, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 15, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 15, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 15, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 15, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 15, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 15, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 15, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Jan 22, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 22, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 22, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 22, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 22, 2026

Choose a reason for hiding this comment

Uh oh!

coveralls commented Jan 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Test Coverage Report for Build 21417800998

Details

💛 - Coveralls

Uh oh!

qualand left a comment

Choose a reason for hiding this comment

Uh oh!

jmaack24 commented Jan 27, 2026

Uh oh!

qualand commented Jan 27, 2026

Uh oh!

jmaack24 commented Jan 27, 2026

Uh oh!

taylorbrown75 left a comment

Choose a reason for hiding this comment

Uh oh!

qualand commented Jan 28, 2026

Uh oh!

Uh oh!

jmaack24 commented Jan 28, 2026

Uh oh!

coveralls commented Jan 22, 2026 •

edited

Loading