Skip to content

feat!: add overwrite option to Frame construction method #349

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
loft-nicolas-sanchez wants to merge 5 commits into
base: main
Choose a base branch
from
Open

feat!: add overwrite option to Frame construction method #349

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
7e6b3e9
feat: add overwrite option to Frame construction method
loft-nicolas-sanchez Oct 27, 2025
e199305
docs: correct typo in error message and update constructor documentation
loft-nicolas-sanchez Oct 27, 2025
0fc0b6a
chore: apply suggestions
loft-nicolas-sanchez Oct 28, 2025
2adbb9b
fix: add missing comma after arg_v
loft-nicolas-sanchez Oct 28, 2025
43d99be
fix: include missing Frame Manager header in tests
loft-nicolas-sanchez Oct 28, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions bindings/python/src/OpenSpaceToolkitPhysicsPy/Coordinate/Frame.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -348,6 +348,7 @@ inline void OpenSpaceToolkitPhysicsPy_Coordinate_Frame(pybind11::module& aModule
arg("is_quasi_inertial"),
arg("parent_frame"),
arg("provider"),
arg("overwrite") = false,
R"doc(
Construct a frame.

Expand All @@ -356,6 +357,7 @@ inline void OpenSpaceToolkitPhysicsPy_Coordinate_Frame(pybind11::module& aModule
is_quasi_inertial (bool): True if quasi-inertial.
parent_frame (Frame): Parent frame.
provider (Provider): Provider.
overwrite (bool): True to overwrite existing frame with same name (default: False).

Returns:
Frame: Frame.
Expand Down
4 changes: 3 additions & 1 deletion include/OpenSpaceToolkit/Physics/Coordinate/Frame.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -112,12 +112,14 @@ class Frame : public std::enable_shared_from_this<Frame>
/// @param [in] isQuasiInertialT True is frame is quasi-inertial
/// @param [in] aParentFrame A shared pointer to the parent frame
/// @param [in] aProvider A shared pointer to the transform provider
/// @param [in] overwrite True to overwrite existing frame with same name

static Shared<const Frame> Construct(
const String& aName,
bool isQuasiInertial,
const Shared<const Frame>& aParentFrame,
const Shared<const Provider>& aProvider
const Shared<const Provider>& aProvider,
bool overwrite = false
);

static void Destruct(const String& aName);
Expand Down
12 changes: 10 additions & 2 deletions src/OpenSpaceToolkit/Physics/Coordinate/Frame.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -420,12 +420,20 @@ Shared<const Frame> Frame::Construct(
const String& aName,
bool isQuasiInertial,
const Shared<const Frame>& aParentFrame,
const Shared<const Provider>& aProvider
const Shared<const Provider>& aProvider,
bool overwrite
)
{
if (FrameManager::Get().hasFrameWithName(aName))
{
throw ostk::core::error::RuntimeError("Frame with name [{}] already exist.", aName);
if (overwrite)
{
FrameManager::Get().removeFrameWithName(aName);
}
else
{
throw ostk::core::error::RuntimeError("Frame with name [{}] already exist.", aName);
}
}

return Frame::Emplace(aName, isQuasiInertial, aParentFrame, aProvider);
Expand Down
23 changes: 23 additions & 0 deletions test/OpenSpaceToolkit/Physics/Coordinate/Frame.test.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -742,6 +742,29 @@ TEST_F(OpenSpaceToolkit_Physics_Coordinate_Frame, Construct)

EXPECT_ANY_THROW(Frame::Construct(name, isQuasiInertial_, Frame::GCRF(), providerSPtr_));
}

// Test overwrite functionality
{
const String name = "Custom B";

// Create first frame
const Shared<const Frame> firstFrameSPtr =
Frame::Construct(name, isQuasiInertial_, Frame::GCRF(), providerSPtr_);
EXPECT_TRUE(firstFrameSPtr->isDefined());

// Attempt to create another frame with same name without overwrite - should throw
EXPECT_ANY_THROW(Frame::Construct(name, isQuasiInertial_, Frame::ITRF(), providerSPtr_, false));

// Create another frame with same name with overwrite - should succeed
const Shared<const Frame> secondFrameSPtr =
Frame::Construct(name, isQuasiInertial_, Frame::ITRF(), providerSPtr_, true);
EXPECT_TRUE(secondFrameSPtr->isDefined());

// Verify the new frame has the expected parent (ITRF instead of GCRF)
EXPECT_EQ(Frame::ITRF(), secondFrameSPtr->accessParent());

Frame::Destruct(name);
}
Comment on lines +747 to +769
Copy link

@coderabbitai coderabbitai bot Oct 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion | 🟠 Major

Verify the frame was actually replaced with the new parent.

The test correctly validates that overwrite throws when false and succeeds when true, but line 766 only checks that a frame with the name exists—it doesn't confirm the frame was replaced with the new properties (ITRF parent instead of GCRF).

Add verification that the parent frame changed:

         // Verify the new frame has the expected parent (ITRF instead of GCRF)
         EXPECT_TRUE(Manager::Get().hasFrameWithName(name));
+        EXPECT_EQ(Frame::ITRF(), Frame::WithName(name)->accessParent());
 
         Frame::Destruct(name);
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
// Test overwrite functionality
{
const String name = "Custom B";
// Create first frame
const Shared<const Frame> firstFrameSPtr =
Frame::Construct(name, isQuasiInertial_, Frame::GCRF(), providerSPtr_);
EXPECT_TRUE(firstFrameSPtr->isDefined());
// Attempt to create another frame with same name without overwrite - should throw
EXPECT_ANY_THROW(Frame::Construct(name, isQuasiInertial_, Frame::ITRF(), providerSPtr_, false));
// Create another frame with same name with overwrite - should succeed
const Shared<const Frame> secondFrameSPtr =
Frame::Construct(name, isQuasiInertial_, Frame::ITRF(), providerSPtr_, true);
EXPECT_TRUE(secondFrameSPtr->isDefined());
// Verify the new frame has the expected parent (ITRF instead of GCRF)
EXPECT_TRUE(Manager::Get().hasFrameWithName(name));
Frame::Destruct(name);
}
// Test overwrite functionality
{
const String name = "Custom B";
// Create first frame
const Shared<const Frame> firstFrameSPtr =
Frame::Construct(name, isQuasiInertial_, Frame::GCRF(), providerSPtr_);
EXPECT_TRUE(firstFrameSPtr->isDefined());
// Attempt to create another frame with same name without overwrite - should throw
EXPECT_ANY_THROW(Frame::Construct(name, isQuasiInertial_, Frame::ITRF(), providerSPtr_, false));
// Create another frame with same name with overwrite - should succeed
const Shared<const Frame> secondFrameSPtr =
Frame::Construct(name, isQuasiInertial_, Frame::ITRF(), providerSPtr_, true);
EXPECT_TRUE(secondFrameSPtr->isDefined());
// Verify the new frame has the expected parent (ITRF instead of GCRF)
EXPECT_TRUE(Manager::Get().hasFrameWithName(name));
EXPECT_EQ(Frame::ITRF(), Frame::WithName(name)->accessParent());
Frame::Destruct(name);
}
🤖 Prompt for AI Agents 
In test/OpenSpaceToolkit/Physics/Coordinate/Frame.test.cpp around lines 747 to
769, after creating secondFrameSPtr with overwrite=true you need to verify the
replacement actually changed the parent: obtain the frame instance from the
manager (or use secondFrameSPtr) and assert its parent is ITRF (not GCRF);
replace the loose Manager::Get().hasFrameWithName(name) check with or add an
EXPECT_TRUE/EXPECT_EQ that the retrieved frame's parent equals Frame::ITRF() to
confirm the overwrite updated the parent frame.

}

TEST_F(OpenSpaceToolkit_Physics_Coordinate_Frame, Destruct)
Expand Down
Loading