Skip to content

Commit

Permalink
Add function result::if_error and remove automatic conversion to bool
Browse files Browse the repository at this point in the history
  • Loading branch information
@kduske
kduske committed Feb 1, 2023
1 parent 3c42415 commit 98cc117
Show file tree
Hide file tree
Showing 7 changed files with 139 additions and 64 deletions.
5 changes: 2 additions & 3 deletions common/src/Model/Brush.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -400,7 +400,7 @@ kdl::result<void, BrushError> Brush::expand(
for (auto& face : m_faces)
{
const vm::vec3 moveAmount = face.boundary().normal * delta;
if (!face.transform(vm::translation_matrix(moveAmount), lockTexture))
if (!face.transform(vm::translation_matrix(moveAmount), lockTexture).is_success())
{
return BrushError::InvalidFace;
}
Expand Down Expand Up @@ -1191,8 +1191,7 @@ kdl::result<void, BrushError> Brush::transform(
{
for (auto& face : m_faces)
{
if (const auto transformResult = face.transform(transformation, lockTextures);
!transformResult)
if (!face.transform(transformation, lockTextures).is_success())
{
return BrushError::InvalidFace;
}
Expand Down
6 changes: 2 additions & 4 deletions common/src/View/ExtrudeTool.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -450,14 +450,12 @@ bool splitBrushesOutward(
document.selectNodes(addedNodes);
dragState.currentDragFaces = std::move(newDragFaces);
dragState.totalDelta = delta;
return true;
})
.or_else([&](const Model::BrushError e) {
.if_error([&](const Model::BrushError e) {
document.error() << "Could not extrude brush: " << e;
kdl::map_clear_and_delete(newNodes);
return false;
})
.value();
.is_success();
}

/**
Expand Down
28 changes: 16 additions & 12 deletions common/src/View/MapDocument.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3046,7 +3046,8 @@ bool MapDocument::createBrush(const std::vector<vm::vec3>& points)

return kdl::void_success;
})
.or_else([&](const auto& e) { error() << "Could not create brush: " << e; });
.if_error([&](const auto& e) { error() << "Could not create brush: " << e; })
.is_success();
}

bool MapDocument::csgConvexMerge()
Expand Down Expand Up @@ -3125,8 +3126,9 @@ bool MapDocument::csgConvexMerge()
selectNodes({brushNode});
transaction.commit();
})
.or_else(
[&](const Model::BrushError e) { error() << "Could not create brush: " << e; });
.if_error(
[&](const Model::BrushError e) { error() << "Could not create brush: " << e; })
.is_success();
}

bool MapDocument::csgSubtract()
Expand Down Expand Up @@ -3196,9 +3198,10 @@ bool MapDocument::csgIntersect()
auto* brushNode = *it;
const auto& brush = brushNode->brush();
valid = intersection.intersect(m_worldBounds, brush)
.or_else([&](const Model::BrushError e) {
.if_error([&](const Model::BrushError e) {
error() << "Could not intersect brushes: " << e;
});
})
.is_success();
}

const auto toRemove = std::vector<Model::Node*>{std::begin(brushes), std::end(brushes)};
Expand Down Expand Up @@ -3350,9 +3353,10 @@ bool MapDocument::clipBrushes(const vm::vec3& p1, const vm::vec3& p2, const vm::
{
return ReplaceClippedBrushesError{};
}
return {};
return kdl::void_success;
})
.or_else([&](const auto& e) { error() << "Could not clip brushes: " << e; });
.if_error([&](const auto& e) { error() << "Could not clip brushes: " << e; })
.is_success();
}

bool MapDocument::setProperty(
Expand Down Expand Up @@ -3848,7 +3852,7 @@ MapDocument::MoveVerticesResult MapDocument::moveVertices(
newVertexPositions =
kdl::vec_concat(std::move(newVertexPositions), std::move(newPositions));
})
.or_else([&](const Model::BrushError e) {
.if_error([&](const Model::BrushError e) {
error() << "Could not move brush vertices: " << e;
})
.is_success();
Expand Down Expand Up @@ -3929,7 +3933,7 @@ bool MapDocument::moveEdges(
newEdgePositions =
kdl::vec_concat(std::move(newEdgePositions), std::move(newPositions));
})
.or_else([&](const Model::BrushError e) {
.if_error([&](const Model::BrushError e) {
error() << "Could not move brush edges: " << e;
})
.is_success();
Expand Down Expand Up @@ -3997,7 +4001,7 @@ bool MapDocument::moveFaces(
newFacePositions =
kdl::vec_concat(std::move(newFacePositions), std::move(newPositions));
})
.or_else([&](const Model::BrushError e) {
.if_error([&](const Model::BrushError e) {
error() << "Could not move brush faces: " << e;
})
.is_success();
Expand Down Expand Up @@ -4049,7 +4053,7 @@ bool MapDocument::addVertex(const vm::vec3& vertexPosition)
}

return brush.addVertex(m_worldBounds, vertexPosition)
.or_else([&](const Model::BrushError e) {
.if_error([&](const Model::BrushError e) {
error() << "Could not add brush vertex: " << e;
})
.is_success();
Expand Down Expand Up @@ -4106,7 +4110,7 @@ bool MapDocument::removeVertices(
}

return brush.removeVertices(m_worldBounds, verticesToRemove)
.or_else([&](const Model::BrushError e) {
.if_error([&](const Model::BrushError e) {
error() << "Could not remove brush vertices: " << e;
})
.is_success();
Expand Down
10 changes: 5 additions & 5 deletions common/test/src/Model/tst_BrushFace.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -218,14 +218,14 @@ static void checkTextureLockOffWithTransform(
// reset alignment, transform the face (texture lock off)
BrushFace face = origFace;
resetFaceTextureAlignment(face);
REQUIRE(face.transform(transform, false));
REQUIRE(face.transform(transform, false).is_success());
face.resetTexCoordSystemCache();

// reset alignment, transform the face (texture lock off), then reset the alignment
// again
BrushFace resetFace = origFace;
resetFaceTextureAlignment(resetFace);
REQUIRE(resetFace.transform(transform, false));
REQUIRE(resetFace.transform(transform, false).is_success());
resetFaceTextureAlignment(resetFace);

// UVs of the verts of `face` and `resetFace` should be the same now
Expand Down Expand Up @@ -296,7 +296,7 @@ static void checkTextureLockOnWithTransform(

// transform the face
BrushFace face = origFace;
REQUIRE(face.transform(transform, true));
REQUIRE(face.transform(transform, true).is_success());
face.resetTexCoordSystemCache();

// transform the verts
Expand Down Expand Up @@ -511,7 +511,7 @@ static void checkTextureLockOffWithVerticalFlip(const Brush& cube)

// transform the face (texture lock off)
BrushFace face = origFace;
REQUIRE(face.transform(transform, false));
REQUIRE(face.transform(transform, false).is_success());
face.resetTexCoordSystemCache();

// UVs of the verts of `face` and `origFace` should be the same now
Expand Down Expand Up @@ -541,7 +541,7 @@ static void checkTextureLockOffWithScale(const Brush& cube)

// transform the face (texture lock off)
BrushFace face = origFace;
REQUIRE(face.transform(transform, false));
REQUIRE(face.transform(transform, false).is_success());
face.resetTexCoordSystemCache();

// get UV at mins; should be equal
Expand Down
48 changes: 32 additions & 16 deletions common/test/src/View/tst_UpdateLinkedGroupsHelper.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -103,8 +103,10 @@ TEST_CASE_METHOD(UpdateLinkedGroupsHelperTest, "UpdateLinkedGroupsHelperTest.own
{
{
auto helper = UpdateLinkedGroupsHelper{{linkedNode}};
REQUIRE(helper.applyLinkedGroupUpdates(
*static_cast<MapDocumentCommandFacade*>(document.get())));
REQUIRE(helper
.applyLinkedGroupUpdates(
*static_cast<MapDocumentCommandFacade*>(document.get()))
.is_success());
}
CHECK(deleted);
}
Expand All @@ -113,8 +115,10 @@ TEST_CASE_METHOD(UpdateLinkedGroupsHelperTest, "UpdateLinkedGroupsHelperTest.own
{
{
auto helper = UpdateLinkedGroupsHelper{{linkedNode}};
REQUIRE(helper.applyLinkedGroupUpdates(
*static_cast<MapDocumentCommandFacade*>(document.get())));
REQUIRE(helper
.applyLinkedGroupUpdates(
*static_cast<MapDocumentCommandFacade*>(document.get()))
.is_success());
helper.undoLinkedGroupUpdates(
*static_cast<MapDocumentCommandFacade*>(document.get()));
}
Expand Down Expand Up @@ -183,8 +187,10 @@ TEST_CASE_METHOD(

// propagate changes
auto helper = UpdateLinkedGroupsHelper{{groupNode}};
REQUIRE(helper.applyLinkedGroupUpdates(
*static_cast<MapDocumentCommandFacade*>(document.get())));
REQUIRE(
helper
.applyLinkedGroupUpdates(*static_cast<MapDocumentCommandFacade*>(document.get()))
.is_success());

/*
world
Expand Down Expand Up @@ -413,8 +419,10 @@ TEST_CASE_METHOD(
SECTION("First propagate changes to innerGroupNode, then outerGroupNode")
{
auto helper1 = UpdateLinkedGroupsHelper{{innerGroupNode}};
CHECK(helper1.applyLinkedGroupUpdates(
*static_cast<MapDocumentCommandFacade*>(document.get())));
CHECK(
helper1
.applyLinkedGroupUpdates(*static_cast<MapDocumentCommandFacade*>(document.get()))
.is_success());

/*
world
Expand Down Expand Up @@ -451,17 +459,21 @@ TEST_CASE_METHOD(
== originalBrushBounds.translate(vm::vec3(32.0, 0.0, 8.0)));

auto helper2 = UpdateLinkedGroupsHelper{{outerGroupNode}};
CHECK(helper2.applyLinkedGroupUpdates(
*static_cast<MapDocumentCommandFacade*>(document.get())));
CHECK(
helper2
.applyLinkedGroupUpdates(*static_cast<MapDocumentCommandFacade*>(document.get()))
.is_success());

// see end of test for assertions of final state
}

SECTION("First propagate changes to outerGroupNode, then innerGroupNode")
{
auto helper1 = UpdateLinkedGroupsHelper{{outerGroupNode}};
REQUIRE(helper1.applyLinkedGroupUpdates(
*static_cast<MapDocumentCommandFacade*>(document.get())));
REQUIRE(
helper1
.applyLinkedGroupUpdates(*static_cast<MapDocumentCommandFacade*>(document.get()))
.is_success());

/*
world
Expand Down Expand Up @@ -496,8 +508,10 @@ TEST_CASE_METHOD(
== originalBrushBounds.translate(vm::vec3(32.0, 16.0, 8.0)));

auto helper2 = UpdateLinkedGroupsHelper{{innerGroupNode}};
REQUIRE(helper2.applyLinkedGroupUpdates(
*static_cast<MapDocumentCommandFacade*>(document.get())));
REQUIRE(
helper2
.applyLinkedGroupUpdates(*static_cast<MapDocumentCommandFacade*>(document.get()))
.is_success());

// see end of test for assertions of final state
}
Expand All @@ -515,8 +529,10 @@ TEST_CASE_METHOD(
}

auto helper = UpdateLinkedGroupsHelper{groupNodes};
REQUIRE(helper.applyLinkedGroupUpdates(
*static_cast<MapDocumentCommandFacade*>(document.get())));
REQUIRE(
helper
.applyLinkedGroupUpdates(*static_cast<MapDocumentCommandFacade*>(document.get()))
.is_success());
}

/*
Expand Down
62 changes: 44 additions & 18 deletions lib/kdl/include/kdl/result.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -483,6 +483,30 @@ class [[nodiscard]] result
}
}

/**
* Applies the given function to any error contained in this result, and returns this
* result.
*
* Use this function for error logging and such, but not for fixing any errors.
*/
template <typename F>
auto if_error(F&& f) const&
{
visit(kdl::overload([](const Value&) {}, [&](const auto& e) { f(e); }));
return *this;
}

/**
* See the previous function. The only difference is that the value contained in this
* result is passed to `f` by rvalue reference to allow moving.
*/
template <typename F>
auto if_error(F&& f) &&
{
std::move(*this).visit(kdl::overload([](const Value&) {}, [&](auto&& e) { f(e); }));
return std::move(*this);
}

/**
* Returns the value contained in this result if it is successful. Otherwise, throws
* `bad_result_access`.
Expand Down Expand Up @@ -592,12 +616,6 @@ class [[nodiscard]] result
return std::holds_alternative<E>(m_value);
}

/**
* Indicates whether the given result contains a value.
*/
// NOLINTNEXTLINE
operator bool() const { return is_success(); }

friend bool operator==(const result& lhs, const result& rhs)
{
return lhs.m_value == rhs.m_value;
Expand Down Expand Up @@ -694,12 +712,6 @@ class result<void>
*/
bool is_error() const { return !is_success(); }

/**
* Indicates whether this result is empty.
*/
// NOLINTNEXTLINE
operator bool() const { return is_success(); }

friend bool operator==(const result&, const result&) { return true; }

friend bool operator!=(const result& lhs, const result& rhs) { return !(lhs == rhs); }
Expand Down Expand Up @@ -1009,6 +1021,26 @@ class [[nodiscard]] result<void, Errors...>
}
}

/**
* See result<Value, Errors...>::if_error.
*/
template <typename F>
auto if_error(F&& f) const&
{
visit(kdl::overload([]() {}, [&](const auto& e) { f(e); }));
return *this;
}

/**
* See result<Value, Errors...>::if_error.
*/
template <typename F>
auto if_error(F&& f) &&
{
std::move(*this).visit(kdl::overload([]() {}, [&](auto&& e) { f(e); }));
return std::move(*this);
}

/**
* Returns a the error contained in this result if it not successful. Otherwise, throws
* `bad_result_access`.
Expand Down Expand Up @@ -1063,12 +1095,6 @@ class [[nodiscard]] result<void, Errors...>
return std::holds_alternative<E>(m_value);
}

/**
* Indicates whether this result is empty.
*/
// NOLINTNEXTLINE
operator bool() const { return is_success(); }

friend bool operator==(const result& lhs, const result& rhs)
{
return lhs.m_value == rhs.m_value;
Expand Down
Loading

0 comments on commit 98cc117

Please sign in to comment.