Skip to content

Commit

Permalink
3755: MapDocument: fix error handling for csgHollow and csgSubtract
Browse files Browse the repository at this point in the history 
- Brush::subtract now ignores any brushes that fail to create,
  previously these would block the csgSubtract operation
- in csgHollow, if one brush is too small to hollow, don't prevent
  the whole operation

Fixes #3755
  • Loading branch information
@ericwa
ericwa committed Mar 8, 2021
1 parent d95ce43 commit 8c0eff1
Show file tree
Hide file tree
Showing 4 changed files with 97 additions and 65 deletions.
27 changes: 22 additions & 5 deletions common/src/Model/Brush.cpp
View file
Expand Up @@ -880,7 +880,7 @@ namespace TrenchBroom {
return updateGeometryFromFaces(worldBounds);
}

kdl::result<std::vector<Brush>, BrushError> Brush::subtract(const MapFormat mapFormat, const vm::bbox3& worldBounds, const std::string& defaultTextureName, const std::vector<const Brush*>& subtrahends) const {
std::tuple<std::vector<Brush>, std::optional<BrushError>> Brush::subtract(const MapFormat mapFormat, const vm::bbox3& worldBounds, const std::string& defaultTextureName, const std::vector<const Brush*>& subtrahends) const {
auto result = std::vector<BrushGeometry>{*m_geometry};

for (const auto* subtrahend : subtrahends) {
Expand All @@ -894,12 +894,29 @@ namespace TrenchBroom {
result = std::move(nextResults);
}

return kdl::for_each_result(result, [&](const auto& geometry) {
return createBrush(mapFormat, worldBounds, defaultTextureName, geometry, subtrahends);
});
// create Brushes and discard any that fail to create
std::vector<Brush> resultBrushes;
std::optional<BrushError> firstError;

resultBrushes.reserve(result.size());

for (auto&& geometry : result) {
createBrush(mapFormat, worldBounds, defaultTextureName, geometry, subtrahends)
.and_then([&](Brush&& brush) {
resultBrushes.push_back(brush);
})
.handle_errors([&](const BrushError& e) {
if (!firstError) {
// let the caller know that there was at least one failed brush
// (probably a harmless microbrush)
firstError = e;
}
});
}
return std::make_tuple(resultBrushes, firstError);
}

kdl::result<std::vector<Brush>, BrushError> Brush::subtract(const MapFormat mapFormat, const vm::bbox3& worldBounds, const std::string& defaultTextureName, const Brush& subtrahend) const {
std::tuple<std::vector<Brush>, std::optional<BrushError>> Brush::subtract(const MapFormat mapFormat, const vm::bbox3& worldBounds, const std::string& defaultTextureName, const Brush& subtrahend) const {
return subtract(mapFormat, worldBounds, defaultTextureName, std::vector<const Brush*>{&subtrahend});
}

Expand Down
8 changes: 5 additions & 3 deletions common/src/Model/Brush.h
View file
Expand Up @@ -210,10 +210,12 @@ namespace TrenchBroom {
* Subtracts the given subtrahends from `this`, returning the result but without modifying `this`.
*
* @param subtrahends brushes to subtract from `this`. The passed-in brushes are not modified.
* @return the subtraction result
* @return the subtraction result, and an optional error if any brushes were discarded.
* Note, the subtraction result should still be valid even if a BrushError is returned.
* It's a hint to the user to double check the result, and potentially report a bug.
*/
kdl::result<std::vector<Brush>, BrushError> subtract(MapFormat mapFormat, const vm::bbox3& worldBounds, const std::string& defaultTextureName, const std::vector<const Brush*>& subtrahends) const;
kdl::result<std::vector<Brush>, BrushError> subtract(MapFormat mapFormat, const vm::bbox3& worldBounds, const std::string& defaultTextureName, const Brush& subtrahend) const;
std::tuple<std::vector<Brush>, std::optional<BrushError>> subtract(MapFormat mapFormat, const vm::bbox3& worldBounds, const std::string& defaultTextureName, const std::vector<const Brush*>& subtrahends) const;
std::tuple<std::vector<Brush>, std::optional<BrushError>> subtract(MapFormat mapFormat, const vm::bbox3& worldBounds, const std::string& defaultTextureName, const Brush& subtrahend) const;

/**
* Intersects this brush with the given brush.
Expand Down
117 changes: 65 additions & 52 deletions common/src/View/MapDocument.cpp
View file
Expand Up @@ -2102,33 +2102,33 @@ namespace TrenchBroom {

const auto minuendNodes = std::vector<Model::BrushNode*>{selectedNodes().brushes()};
const auto subtrahends = kdl::vec_transform(subtrahendNodes, [](const auto* subtrahendNode) { return &subtrahendNode->brush(); });

return kdl::for_each_result(minuendNodes, [&](Model::BrushNode* minuendNode) {
const Model::Brush& minuend = minuendNode->brush();
return minuend.subtract(m_world->mapFormat(), m_worldBounds, currentTextureName(), subtrahends)
.and_then([&](std::vector<Model::Brush>&& brushes) -> kdl::result<std::pair<Model::BrushNode*, std::vector<Model::Brush>>> {
return std::make_pair(minuendNode, std::move(brushes));
});
}).and_then([&](std::vector<std::pair<Model::BrushNode*, std::vector<Model::Brush>>>&& subtractionResults) {
auto toAdd = std::map<Model::Node*, std::vector<Model::Node*>>{};
auto toRemove = std::vector<Model::Node*>{std::begin(subtrahendNodes), std::end(subtrahendNodes)};

for (auto& [minuendNode, resultBrushes] : subtractionResults) {
if (!resultBrushes.empty()) {
auto resultNodes = kdl::vec_transform(std::move(resultBrushes), [&](auto b) { return new Model::BrushNode(std::move(b)); });
auto& toAddForParent = toAdd[minuendNode->parent()];
toAddForParent = kdl::vec_concat(std::move(toAddForParent), std::move(resultNodes));
std::vector<std::pair<Model::BrushNode*, std::vector<Model::Brush>>> subtractionResults =
kdl::vec_transform(minuendNodes, [&](Model::BrushNode* minuendNode) {
const Model::Brush& minuend = minuendNode->brush();
auto [brushes, optionalWarning] = minuend.subtract(m_world->mapFormat(), m_worldBounds, currentTextureName(), subtrahends);
if (optionalWarning) {
error() << "Could not create brush: " << *optionalWarning;
}
toRemove.push_back(minuendNode);
return std::make_pair(minuendNode, std::move(brushes));
});

auto toAdd = std::map<Model::Node*, std::vector<Model::Node*>>{};
auto toRemove = std::vector<Model::Node*>{std::begin(subtrahendNodes), std::end(subtrahendNodes)};

for (auto& [minuendNode, resultBrushes] : subtractionResults) {
if (!resultBrushes.empty()) {
auto resultNodes = kdl::vec_transform(std::move(resultBrushes), [&](auto b) { return new Model::BrushNode(std::move(b)); });
auto& toAddForParent = toAdd[minuendNode->parent()];
toAddForParent = kdl::vec_concat(std::move(toAddForParent), std::move(resultNodes));
}
toRemove.push_back(minuendNode);
}

deselectAll();
const auto added = addNodes(toAdd);
removeNodes(toRemove);
select(added);
}).handle_errors([&](const Model::BrushError& e) {
error() << "Could not create brush: " << e;
});
deselectAll();
const auto added = addNodes(toAdd);
removeNodes(toRemove);
select(added);
return true;
}

Expand Down Expand Up @@ -2173,38 +2173,51 @@ namespace TrenchBroom {
return false;
}

return kdl::for_each_result(brushNodes, [&](Model::BrushNode* brushNode) {
const auto& originalBrush = brushNode->brush();

auto shrunkenBrush = originalBrush;
return shrunkenBrush.expand(m_worldBounds, -1.0 * static_cast<FloatType>(m_grid->actualSize()), true)
.and_then([&]() {
return originalBrush.subtract(m_world->mapFormat(), m_worldBounds, currentTextureName(), shrunkenBrush);
}).and_then([&](auto&& fragments) -> kdl::result<std::pair<Model::BrushNode*, std::vector<Model::Brush>>> {
return std::make_pair(brushNode, std::move(fragments));
});
}).and_then([&](std::vector<std::pair<Model::BrushNode*, std::vector<Model::Brush>>>&& fragmentsAndSourceNodes) {
auto toAdd = std::map<Model::Node*, std::vector<Model::Node*>>{};
auto toRemove = std::vector<Model::Node*>{};
bool didHollowAnything = false;
std::vector<std::pair<Model::BrushNode*, std::vector<Model::Brush>>> fragmentsAndSourceNodes =
kdl::vec_transform(brushNodes, [&](Model::BrushNode* brushNode) {
const auto& originalBrush = brushNode->brush();

for (auto& [sourceNode, fragments] : fragmentsAndSourceNodes) {
auto fragmentNodes = kdl::vec_transform(std::move(fragments), [](auto&& b) {
return new Model::BrushNode(std::move(b));
});
auto shrunkenBrush = originalBrush;
std::vector<Model::Brush> fragments;
shrunkenBrush.expand(m_worldBounds, -1.0 * static_cast<FloatType>(m_grid->actualSize()), true)
.and_then([&]() {
didHollowAnything = true;
auto [brushes, optionalWarning] = originalBrush.subtract(m_world->mapFormat(), m_worldBounds, currentTextureName(), shrunkenBrush);
if (optionalWarning) {
error() << "Could not create brush: " << *optionalWarning;
}
fragments = std::move(brushes);
}).handle_errors([&](const Model::BrushError& e) {
error() << "Could not hollow brush: " << e;
fragments = { originalBrush };
});

auto& toAddForParent = toAdd[sourceNode->parent()];
toAddForParent = kdl::vec_concat(std::move(toAddForParent), fragmentNodes);
toRemove.push_back(sourceNode);
}
return std::make_pair(brushNode, std::move(fragments));
});
if (!didHollowAnything) {
return false;
}
auto toAdd = std::map<Model::Node*, std::vector<Model::Node*>>{};
auto toRemove = std::vector<Model::Node*>{};

Transaction transaction(this, "CSG Hollow");
deselectAll();
const auto added = addNodes(toAdd);
removeNodes(toRemove);
select(added);
}).handle_errors([&](const Model::BrushError& e) {
error() << "Could not hollow brush: " << e;
});
for (auto& [sourceNode, fragments] : fragmentsAndSourceNodes) {
auto fragmentNodes = kdl::vec_transform(std::move(fragments), [](auto&& b) {
return new Model::BrushNode(std::move(b));
});

auto& toAddForParent = toAdd[sourceNode->parent()];
toAddForParent = kdl::vec_concat(std::move(toAddForParent), fragmentNodes);
toRemove.push_back(sourceNode);
}

Transaction transaction(this, "CSG Hollow");
deselectAll();
const auto added = addNodes(toAdd);
removeNodes(toRemove);
select(added);

return true;
}

bool MapDocument::clipBrushes(const vm::vec3& p1, const vm::vec3& p2, const vm::vec3& p3) {
Expand Down
10 changes: 5 additions & 5 deletions common/test/src/Model/BrushTest.cpp
View file
Expand Up @@ -3084,7 +3084,7 @@ namespace TrenchBroom {
const Brush minuend = builder.createCuboid(vm::bbox3(vm::vec3(-32.0, -16.0, -32.0), vm::vec3(32.0, 16.0, 32.0)), minuendTexture).value();
const Brush subtrahend = builder.createCuboid(vm::bbox3(vm::vec3(-16.0, -32.0, -64.0), vm::vec3(16.0, 32.0, 0.0)), subtrahendTexture).value();

const std::vector<Brush> result = minuend.subtract(MapFormat::Standard, worldBounds, defaultTexture, subtrahend).value();
const auto [result, optionalWarning] = minuend.subtract(MapFormat::Standard, worldBounds, defaultTexture, subtrahend);
CHECK(result.size() == 3u);

const Brush* left = nullptr;
Expand Down Expand Up @@ -3168,7 +3168,7 @@ namespace TrenchBroom {
const Brush brush1 = builder.createCuboid(brush1Bounds, "texture").value();
const Brush brush2 = builder.createCuboid(brush2Bounds, "texture").value();

const std::vector<Brush> result = brush1.subtract(MapFormat::Standard, worldBounds, "texture", brush2).value();
const auto [result, optionalWarning] = brush1.subtract(MapFormat::Standard, worldBounds, "texture", brush2);
CHECK(result.size() == 1u);

const Brush& subtraction = result.at(0);
Expand All @@ -3186,7 +3186,7 @@ namespace TrenchBroom {
const Brush brush1 = builder.createCuboid(brush1Bounds, "texture").value();
const Brush brush2 = builder.createCuboid(brush2Bounds, "texture").value();

const std::vector<Brush> result = brush1.subtract(MapFormat::Standard, worldBounds, "texture", brush2).value();
const auto [result, optionalWarning] = brush1.subtract(MapFormat::Standard, worldBounds, "texture", brush2);
CHECK(result.size() == 0u);
}

Expand Down Expand Up @@ -3260,7 +3260,7 @@ namespace TrenchBroom {
const Brush& minuend = static_cast<BrushNode*>(minuendNodes.front())->brush();
const Brush& subtrahend = static_cast<BrushNode*>(subtrahendNodes.front())->brush();

const std::vector<Brush> result = minuend.subtract(MapFormat::Valve, worldBounds, "some_texture", subtrahend).value();
const auto [result, optionalWarning] = minuend.subtract(MapFormat::Valve, worldBounds, "some_texture", subtrahend);
CHECK_FALSE(result.empty());

kdl::col_delete_all(minuendNodes);
Expand Down Expand Up @@ -3336,7 +3336,7 @@ namespace TrenchBroom {
const Brush& minuend = static_cast<BrushNode*>(minuendNodes.front())->brush();
const Brush& subtrahend = static_cast<BrushNode*>(subtrahendNodes.front())->brush();

const std::vector<Brush> result = minuend.subtract(MapFormat::Standard, worldBounds, "some_texture", subtrahend).value();
const auto [result, optionalWarning] = minuend.subtract(MapFormat::Standard, worldBounds, "some_texture", subtrahend);
CHECK(result.size() == 8u);

kdl::col_delete_all(minuendNodes);
Expand Down

0 comments on commit 8c0eff1

Please sign in to comment.