Skip to content
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.

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

Bevel shape outline beyond threshold #2741

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
42 changes: 42 additions & 0 deletions include/SFML/Graphics/Shape.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -134,6 +134,37 @@ class SFML_GRAPHICS_API Shape : public Drawable, public Transformable
////////////////////////////////////////////////////////////
void setOutlineThickness(float thickness);

////////////////////////////////////////////////////////////
/// \brief Set the limit on the ratio between miter length and outline thickness
///
/// Outline segments around each shape corner are joined either
/// with a miter or a bevel join.
/// - A miter join is formed by extending outline segments until
/// they intersect. The distance between the point of
/// intersection and the shape's corner is the miter length.
/// - A bevel join is formed by connecting outline segments with
/// a straight line perpendicular to the corner's bissector.
///
/// The miter limit is used to determine whether ouline segments
/// around a corner are joined with a bevel or a miter.
/// When the ratio between the miter length and outline thickness
/// exceeds the miter limit, a bevel is used instead of a miter.
///
/// The miter limit is linked to the maximum inner angle of a
/// corner below which a bevel is used by the following formula:
///
/// miterLimit = 1 / sin(angle / 2)
///
/// The miter limit must be greater than or equal to 1.
/// By default, the miter limit is 10.
///
/// \param miterLimit New miter limit
///
/// \see getMiterLimit
///
////////////////////////////////////////////////////////////
void setMiterLimit(float miterLimit);
eXpl0it3r marked this conversation as resolved.
Show resolved Hide resolved

////////////////////////////////////////////////////////////
/// \brief Get the source texture of the shape
///
Expand Down Expand Up @@ -188,6 +219,16 @@ class SFML_GRAPHICS_API Shape : public Drawable, public Transformable
////////////////////////////////////////////////////////////
float getOutlineThickness() const;

////////////////////////////////////////////////////////////
/// \brief Get the limit on the ratio between miter length and outline thickness
///
/// \return Limit on the ratio between miter length and outline thickness
Copy link
Member

Choose a reason for hiding this comment

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

This documentation could need a bit more explanation on what "miter" is. If I don't know what "miter" is, the documentation essentially explains things by self reference as one also wouldn't understand what "miter length" is.

Copy link
Contributor Author

@kimci86 kimci86 Nov 19, 2023

Choose a reason for hiding this comment

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

I tried to make it clearer adding documentation to setMiterLimit method. Feedback is welcome.

///
/// \see setMiterLimit
///
////////////////////////////////////////////////////////////
float getMiterLimit() const;
Copy link
Member

Choose a reason for hiding this comment

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

I want to challenge the "Miter" name usage. I believe it makes on a "domain" level a lot of sense to call it that, but as a simple users that wants to draw shapes, it's not easy to understand what "miter" or "miter limit" means.

Just searching the term "miter" leads to the "tall headdress of a bishop" or "joint made between two pieces of wood". You really have to search for "miter limit" or similar until you start getting documentation from SVG/canvas.

I don't have a better name ready just yet, and am also open to using "miter limit", but maybe we can brainstorm a bit, if there's something that describes it in a way that doesn't necessarily require a documentation study or web search.

Copy link
Member

Choose a reason for hiding this comment

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

The PR name uses the word bevel which I think is a good term to describe what's happening. This parameter controls how far the outline corner can stick out before it starts being beveled.

Screenshot 2023-11-09 at 1 01 25 PM

Copy link
Contributor Author

@kimci86 kimci86 Nov 9, 2023

Choose a reason for hiding this comment

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

I believe "miter limit" it is the correct name because it is already used by SVG as well as cairo library.
Edit: another example in Win32 API. And also Java.

It would be more confusing to come up with some other name. I agree the meaning is not obvious and it deserves more documentation. I will work on it.

Copy link
Member

@eXpl0it3r eXpl0it3r Nov 9, 2023

Choose a reason for hiding this comment

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

I understand that it's used like that within the domain of SVG/graphics, but we shouldn't just stop there and do as everyone else does. The term is not intuitively understood and as such it deserves a bit of a brainstorming in my opinion.

What speaks against bevel? Or bevel cutoff/limit?

Copy link
Member

Choose a reason for hiding this comment

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

I asked a bit on social media, to see what people would more intuitively call this.

  • Two mentioned miter, but more because they've learned the word from graphics applications.
  • sanitizeOutline
  • setOutlineExtensionLimit
  • limitOutlineExtension
  • clamp
  • cutting corners
  • corner style
  • capping (used in Godot)
  • Corner length
  • Cap
  • Clip
  • Limit
  • bevel

https://swiss.social/@darkcisum/111391908664038204
https://twitter.com/DarkCisum/status/1723282675878789343

One interesting thought that came out of these discussions is that this could in theory be generalized further and allow for different "styling" of the corners. I wouldn't to do this right now of course, but it might influence the thinking of naming.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To clarify my point, I am not saying the term "miter limit" is used by other libraries because it is the right choice, but rather that it is the right choice because it is used by other libraries.
It seems there is not an obvious intuitive choice. Not choosing "miter limit" would be inconsistent with other libraries on top of not being intuitive and as a consequence would not bring related results in search engine.

Copy link
Member

Choose a reason for hiding this comment

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

I'm personally fine the "miter limit" terminology.


////////////////////////////////////////////////////////////
/// \brief Get the total number of points of the shape
///
Expand Down Expand Up @@ -315,6 +356,7 @@ class SFML_GRAPHICS_API Shape : public Drawable, public Transformable
Color m_fillColor{Color::White}; //!< Fill color
Color m_outlineColor{Color::White}; //!< Outline color
float m_outlineThickness{}; //!< Thickness of the shape's outline
float m_miterLimit{10.f}; //!< Limit on the ratio between miter length and outline thickness
VertexArray m_vertices{PrimitiveType::TriangleFan}; //!< Vertex array containing the fill geometry
VertexArray m_outlineVertices{PrimitiveType::TriangleStrip}; //!< Vertex array containing the outline geometry
FloatRect m_insideBounds; //!< Bounding rectangle of the inside (fill)
Expand Down
114 changes: 84 additions & 30 deletions src/SFML/Graphics/Shape.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -32,18 +32,19 @@
#include <algorithm>

#include <cassert>
#include <cmath>
#include <cstddef>

namespace
{
// Compute the normal of a segment
sf::Vector2f computeNormal(const sf::Vector2f& p1, const sf::Vector2f& p2)
// Compute the direction of a segment
sf::Vector2f computeDirection(const sf::Vector2f& p1, const sf::Vector2f& p2)
{
sf::Vector2f normal = (p2 - p1).perpendicular();
const float length = normal.length();
sf::Vector2f direction = p2 - p1;
const float length = direction.length();
Copy link
Member

Choose a reason for hiding this comment

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

Do we sometimes expect p1 and p2 to be identical? What does it mean if they're identical?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is probably not very meaningful to have overlapping points, but it is a possible state.
If two consecutive points are identical, their (direction, normal) pair would be a pair of zero vectors. Again not very meaningful but the following computations will not explode.

if (length != 0.f)
normal /= length;
return normal;
direction /= length;
return direction;
}
} // namespace

Expand Down Expand Up @@ -121,7 +122,7 @@ const Color& Shape::getOutlineColor() const
void Shape::setOutlineThickness(float thickness)
{
m_outlineThickness = thickness;
update(); // recompute everything because the whole shape must be offset
updateOutline();
}


Expand All @@ -132,6 +133,22 @@ float Shape::getOutlineThickness() const
}


////////////////////////////////////////////////////////////
void Shape::setMiterLimit(float miterLimit)
{
assert(miterLimit >= 1.f && "Shape::setMiterLimit(float) cannot set miter limit to a value lower than 1");
m_miterLimit = miterLimit;
updateOutline();
}


////////////////////////////////////////////////////////////
float Shape::getMiterLimit() const
{
return m_miterLimit;
}


////////////////////////////////////////////////////////////
Vector2f Shape::getGeometricCenter() const
{
Expand Down Expand Up @@ -282,17 +299,28 @@ void Shape::updateTexCoords()
////////////////////////////////////////////////////////////
void Shape::updateOutline()
{
// Return if there is no outline
if (m_outlineThickness == 0.f)
// Return if there is no outline or no vertices
if (m_outlineThickness == 0.f || m_vertices.getVertexCount() < 2)
{
m_outlineVertices.clear();
m_bounds = m_insideBounds;
return;
}

const std::size_t count = m_vertices.getVertexCount() - 2;
m_outlineVertices.resize((count + 1) * 2);
m_outlineVertices.resize((count + 1) * 2); // We need at least that many vertices.
// We will add two more vertices each time we need a bevel.

// Determine if points are defined clockwise or counterclockwise. This will impact normals computation.
const bool flipNormals = [this, count]()
{
float twiceArea = 0.f;
for (std::size_t i = 0; i < count; ++i)
twiceArea += m_vertices[i + 1].position.cross(m_vertices[i + 2].position);
return twiceArea >= 0.f;
}();

std::size_t outlineIndex = 0;
for (std::size_t i = 0; i < count; ++i)
{
const std::size_t index = i + 1;
Expand All @@ -302,29 +330,55 @@ void Shape::updateOutline()
const Vector2f p1 = m_vertices[index].position;
const Vector2f p2 = m_vertices[index + 1].position;

// Compute their normal
Vector2f n1 = computeNormal(p0, p1);
Vector2f n2 = computeNormal(p1, p2);

// Make sure that the normals point towards the outside of the shape
// (this depends on the order in which the points were defined)
if (n1.dot(m_vertices[0].position - p1) > 0)
n1 = -n1;
if (n2.dot(m_vertices[0].position - p1) > 0)
n2 = -n2;

// Combine them to get the extrusion direction
const float factor = 1.f + (n1.x * n2.x + n1.y * n2.y);
const Vector2f normal = (n1 + n2) / factor;

// Update the outline points
m_outlineVertices[i * 2 + 0].position = p1;
m_outlineVertices[i * 2 + 1].position = p1 + normal * m_outlineThickness;
// Compute their direction
const Vector2f d1 = computeDirection(p0, p1);
const Vector2f d2 = computeDirection(p1, p2);

// Compute their normal pointing towards the outside of the shape
const Vector2f n1 = flipNormals ? -d1.perpendicular() : d1.perpendicular();
const Vector2f n2 = flipNormals ? -d2.perpendicular() : d2.perpendicular();

// Decide whether to add a bevel or not
const float twoCos2 = 1.f + n1.dot(n2);
const float squaredLengthRatio = m_miterLimit * m_miterLimit * twoCos2 / 2.f;
const bool isConvexCorner = d1.dot(n2) * m_outlineThickness >= 0.f;
const bool needsBevel = twoCos2 == 0.f || (squaredLengthRatio < 1.f && isConvexCorner);

if (needsBevel)
{
// Make room for two more vertices
m_outlineVertices.resize(m_outlineVertices.getVertexCount() + 2);

// Combine normals to get bevel edge's direction and normal vector pointing towards the outside of the shape
const float twoSin2 = 1.f - n1.dot(n2);
const Vector2f direction = (n2 - n1) / twoSin2; // Length is 1 / sin
const Vector2f extrusion = (flipNormals != (d1.dot(n2) >= 0.f) ? direction : -direction).perpendicular();

// Compute bevel corner position in (direction, extrusion) coordinates
const float sin = std::sqrt(twoSin2 / 2.f);
const float u = m_miterLimit * sin;
const float v = 1.f - std::sqrt(squaredLengthRatio);

// Update the outline points
m_outlineVertices[outlineIndex++].position = p1;
m_outlineVertices[outlineIndex++].position = p1 + (u * extrusion - v * direction) * m_outlineThickness;
m_outlineVertices[outlineIndex++].position = p1;
m_outlineVertices[outlineIndex++].position = p1 + (u * extrusion + v * direction) * m_outlineThickness;
eXpl0it3r marked this conversation as resolved.
Show resolved Hide resolved
}
else
{
// Combine normals to get the extrusion direction
const Vector2f extrusion = (n1 + n2) / twoCos2;

// Update the outline points
m_outlineVertices[outlineIndex++].position = p1;
m_outlineVertices[outlineIndex++].position = p1 + extrusion * m_outlineThickness;
}
}

// Duplicate the first point at the end, to close the outline
m_outlineVertices[count * 2 + 0].position = m_outlineVertices[0].position;
m_outlineVertices[count * 2 + 1].position = m_outlineVertices[1].position;
m_outlineVertices[outlineIndex++].position = m_outlineVertices[0].position;
m_outlineVertices[outlineIndex++].position = m_outlineVertices[1].position;

// Update outline colors
updateOutlineColors();
Expand Down
16 changes: 16 additions & 0 deletions test/Graphics/Shape.test.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,7 @@ TEST_CASE("[Graphics] sf::Shape", runDisplayTests())
CHECK(triangleShape.getFillColor() == sf::Color::White);
CHECK(triangleShape.getOutlineColor() == sf::Color::White);
CHECK(triangleShape.getOutlineThickness() == 0.0f);
CHECK(triangleShape.getMiterLimit() == 10.0f);
CHECK(triangleShape.getLocalBounds() == sf::FloatRect());
CHECK(triangleShape.getGlobalBounds() == sf::FloatRect());
}
Expand Down Expand Up @@ -100,6 +101,13 @@ TEST_CASE("[Graphics] sf::Shape", runDisplayTests())
CHECK(triangleShape.getOutlineThickness() == 3.14f);
}

SECTION("Set/get miter limit")
{
TriangleShape triangleShape({});
triangleShape.setMiterLimit(6.28f);
CHECK(triangleShape.getMiterLimit() == 6.28f);
}

SECTION("Virtual functions: getPoint, getPointCount, getGeometricCenter")
{
const TriangleShape triangleShape({2, 2});
Expand Down Expand Up @@ -130,5 +138,13 @@ TEST_CASE("[Graphics] sf::Shape", runDisplayTests())
CHECK(triangleShape.getLocalBounds() == Approx(sf::FloatRect({-7.2150f, -14.2400f}, {44.4300f, 59.2400f})));
CHECK(triangleShape.getGlobalBounds() == Approx(sf::FloatRect({-7.2150f, -14.2400f}, {44.4300f, 59.2400f})));
}

SECTION("Add beveled outline")
{
triangleShape.setMiterLimit(2);
triangleShape.setOutlineThickness(5);
CHECK(triangleShape.getLocalBounds() == Approx(sf::FloatRect({-7.2150f, -10.f}, {44.4300f, 55.f})));
CHECK(triangleShape.getGlobalBounds() == Approx(sf::FloatRect({-7.2150f, -10.f}, {44.4300f, 55.f})));
}
}
}