From eadbce51bd207346b0bc4905ed512dcf7a06c72d Mon Sep 17 00:00:00 2001 From: leonarski_f Date: Fri, 19 Jun 2026 11:03:40 +0200 Subject: [PATCH] common: take ROIAzimuthal phi as plain floats, not optionals The phi bounds were stored as plain floats internally but passed through std::optional in the constructor, which obscured the intent. Take them as float arguments defaulting to 0 instead. The convention is explicit: phi_min == phi_max means the full ring (all angles), a sector wraps across 0 when phi_min > phi_max, and otherwise phi_min <= phi_max. The "both bounds or none" rule belongs at the API/file boundary (OpenAPIConvert, reader), not in the core class, so it lives there now. Co-Authored-By: Claude Opus 4.8 --- broker/OpenAPIConvert.cpp | 7 ++++--- common/ROIAzimuthal.cpp | 25 +++++++++---------------- common/ROIAzimuthal.h | 11 +++++------ reader/JFJochHDF5Reader.cpp | 8 +++++--- tests/ROIMapTest.cpp | 8 +++++--- 5 files changed, 28 insertions(+), 31 deletions(-) diff --git a/broker/OpenAPIConvert.cpp b/broker/OpenAPIConvert.cpp index 4dc07646..d137c2e1 100644 --- a/broker/OpenAPIConvert.cpp +++ b/broker/OpenAPIConvert.cpp @@ -462,11 +462,12 @@ ROIDefinition Convert(const org::openapitools::server::model::Roi_definitions& i for (const auto &i: input.getCircle().getRois()) output.circles.emplace_back(ROICircle(i.getName(), i.getCenterXPxl(), i.getCenterYPxl(), i.getRadiusPxl())); for (const auto &i: input.getAzim().getRois()) { - std::optional phi_min, phi_max; - if (i.phiMinDegIsSet()) + // A sector needs both bounds; if only one is given, treat it as a full ring. + float phi_min = 0, phi_max = 0; + if (i.phiMinDegIsSet() && i.phiMaxDegIsSet()) { phi_min = i.getPhiMinDeg(); - if (i.phiMaxDegIsSet()) phi_max = i.getPhiMaxDeg(); + } output.azimuthal.emplace_back(ROIAzimuthal(i.getName(), (i.getQMaxRecipA() == 0.0) ? 0.0 : 2.0f * PI / i.getQMaxRecipA(), (i.getQMinRecipA() == 0.0) ? 0.0 : 2.0f * PI / i.getQMinRecipA(), diff --git a/common/ROIAzimuthal.cpp b/common/ROIAzimuthal.cpp index 3e716c19..4c0d2103 100644 --- a/common/ROIAzimuthal.cpp +++ b/common/ROIAzimuthal.cpp @@ -14,8 +14,10 @@ static float NormalizePhi_deg(float phi) { } ROIAzimuthal::ROIAzimuthal(const std::string &in_name, float in_d_min_A, float in_d_max_A, - std::optional in_phi_min_deg, std::optional in_phi_max_deg) - : ROIElement(in_name) { + float in_phi_min_deg, float in_phi_max_deg) + : ROIElement(in_name), + phi_min_deg(NormalizePhi_deg(in_phi_min_deg)), + phi_max_deg(NormalizePhi_deg(in_phi_max_deg)) { if ((in_d_min_A <= 0) || (in_d_max_A <= 0)) throw JFJochException(JFJochExceptionCategory::InputParameterInvalid, "Resolution cannot be zero or negative"); @@ -26,12 +28,6 @@ ROIAzimuthal::ROIAzimuthal(const std::string &in_name, float in_d_min_A, float i d_max_A = in_d_max_A; d_min_A = in_d_min_A; } - - if (in_phi_min_deg && in_phi_max_deg) { - has_phi = true; - phi_min_deg = NormalizePhi_deg(in_phi_min_deg.value()); - phi_max_deg = NormalizePhi_deg(in_phi_max_deg.value()); - } } float ROIAzimuthal::GetDMin_A() const { @@ -43,7 +39,7 @@ float ROIAzimuthal::GetDMax_A() const { } bool ROIAzimuthal::HasPhi() const { - return has_phi; + return phi_min_deg != phi_max_deg; } float ROIAzimuthal::GetPhiMin_deg() const { @@ -57,13 +53,11 @@ float ROIAzimuthal::GetPhiMax_deg() const { bool ROIAzimuthal::CheckROI(int64_t x, int64_t y, float resolution, float phi_deg) const { if (resolution < d_min_A || resolution > d_max_A) return false; - if (!has_phi) - return true; - // A sector with phi_min > phi_max wraps across 0 degrees. + if (phi_min_deg == phi_max_deg) + return true; // full ring: all angles if (phi_min_deg <= phi_max_deg) return (phi_deg >= phi_min_deg) && (phi_deg <= phi_max_deg); - else - return (phi_deg >= phi_min_deg) || (phi_deg <= phi_max_deg); + return (phi_deg >= phi_min_deg) || (phi_deg <= phi_max_deg); // sector wraps across 0 } float ROIAzimuthal::GetQMax_recipA() const { @@ -81,7 +75,6 @@ ROIConfig ROIAzimuthal::ExportMetadata() const { .type = ROIConfig::ROIType::Azim, .name = name, .azim = ROIConfigAzim{.qmin = qmin, .qmax = qmax, - .phi_min = has_phi ? phi_min_deg : 0.0f, - .phi_max = has_phi ? phi_max_deg : 0.0f} + .phi_min = phi_min_deg, .phi_max = phi_max_deg} }; } diff --git a/common/ROIAzimuthal.h b/common/ROIAzimuthal.h index d51271b6..23cbe7e3 100644 --- a/common/ROIAzimuthal.h +++ b/common/ROIAzimuthal.h @@ -3,24 +3,23 @@ #pragma once -#include #include "ROIElement.h" class ROIAzimuthal : public ROIElement { float d_min_A, d_max_A; - bool has_phi = false; - float phi_min_deg = 0; // normalized to [0, 360), used only when has_phi + // Azimuthal-angle sector, normalized to [0, 360). phi_min_deg == phi_max_deg means + // the full ring (all angles); a sector with phi_min_deg > phi_max_deg wraps across 0. + float phi_min_deg = 0; float phi_max_deg = 0; public: ROIAzimuthal(const std::string &name, float d_min_A, float d_max_A, - std::optional phi_min_deg = std::nullopt, - std::optional phi_max_deg = std::nullopt); + float phi_min_deg = 0, float phi_max_deg = 0); ~ROIAzimuthal() override = default; [[nodiscard]] float GetDMin_A() const; [[nodiscard]] float GetDMax_A() const; [[nodiscard]] float GetQMin_recipA() const; [[nodiscard]] float GetQMax_recipA() const; - [[nodiscard]] bool HasPhi() const; + [[nodiscard]] bool HasPhi() const; // a sector is set (phi_min_deg != phi_max_deg) [[nodiscard]] float GetPhiMin_deg() const; [[nodiscard]] float GetPhiMax_deg() const; diff --git a/reader/JFJochHDF5Reader.cpp b/reader/JFJochHDF5Reader.cpp index 30b14b8a..212aa29a 100644 --- a/reader/JFJochHDF5Reader.cpp +++ b/reader/JFJochHDF5Reader.cpp @@ -274,9 +274,11 @@ void JFJochHDF5Reader::ReadROIMetadata(HDF5ReadOnlyFile &file, JFJochReaderDatas else if (type == "azim") { const float qmin = file.GetFloat(base + "/q_min_recipA"); const float qmax = file.GetFloat(base + "/q_max_recipA"); - std::optional phi_min, phi_max; - if (file.Exists(base + "/phi_min_deg")) phi_min = file.GetFloat(base + "/phi_min_deg"); - if (file.Exists(base + "/phi_max_deg")) phi_max = file.GetFloat(base + "/phi_max_deg"); + float phi_min = 0, phi_max = 0; + if (file.Exists(base + "/phi_min_deg") && file.Exists(base + "/phi_max_deg")) { + phi_min = file.GetFloat(base + "/phi_min_deg"); + phi_max = file.GetFloat(base + "/phi_max_deg"); + } const float d_min = (qmax == 0.0f) ? 0.0f : 2.0f * static_cast(PI) / qmax; const float d_max = (qmin == 0.0f) ? 0.0f : 2.0f * static_cast(PI) / qmin; defs.azimuthal.emplace_back(name, d_min, d_max, phi_min, phi_max); diff --git a/tests/ROIMapTest.cpp b/tests/ROIMapTest.cpp index 578064e6..1c4b52c8 100644 --- a/tests/ROIMapTest.cpp +++ b/tests/ROIMapTest.cpp @@ -226,9 +226,11 @@ TEST_CASE("ROIAzimuthal_Phi_Normalize", "[ROIMap]") { REQUIRE(sector.GetPhiMin_deg() == 330.0f); REQUIRE(sector.GetPhiMax_deg() == 40.0f); - // Only one phi bound provided -> treated as full ring - ROIAzimuthal one_bound("roi2", 2.0, 4.0, 30.0f); - REQUIRE_FALSE(one_bound.HasPhi()); + // Default (no phi bounds) and equal bounds are both the full ring + ROIAzimuthal full("roi2", 2.0, 4.0); + REQUIRE_FALSE(full.HasPhi()); + ROIAzimuthal equal_bounds("roi3", 2.0, 4.0, 45.0f, 45.0f); + REQUIRE_FALSE(equal_bounds.HasPhi()); } TEST_CASE("ROIAzimuthal_Phi_MarkROI", "[ROIMap]") {