Bevel shape outline beyond threshold #2741

kimci86 · 2023-10-21T23:04:19Z

Description

This changes sf::Shape outline computation by beveling outline around small angles.
This is to fix #2727.

I noticed and fixed another issue that was visible with my test program below. See the first commit.

Here are videos showing the behavior of the test program before and after this PR commits.

before.mp4

after2.mp4

Initial result of the PR before review was:

after.mp4

Tasks

Write more documentation

How to test this PR?

Moving the mouse and scrolling when running this program, seeing that the outline looks good.

#include <SFML/Graphics.hpp>

int main()
{
    constexpr auto worldSize = sf::Vector2f{800, 600};

    auto shape = sf::ConvexShape{3};
    shape.setPosition(worldSize / 2.f);
    shape.setPoint(0, {0, 100});
    shape.setPoint(1, {-100, 0});
    shape.setPoint(2, {100, 0});
    shape.setFillColor(sf::Color::White);
    shape.setOutlineColor({0, 255, 0, 128});
    shape.setOutlineThickness(20);

    auto window = sf::RenderWindow{sf::VideoMode{sf::Vector2u{worldSize}}, "Shape example"};
    window.setVerticalSyncEnabled(true);
    window.setMouseCursorVisible(false);

    while (window.isOpen())
    {
        for (auto event = sf::Event{}; window.pollEvent(event);)
        {
            switch (event.type)
            {
                case sf::Event::Closed:
                    window.close();
                    break;
                case sf::Event::MouseMoved:
                {
                    const auto pixels      = sf::Vector2i{event.mouseMove.x, event.mouseMove.y};
                    const auto coords      = window.mapPixelToCoords(pixels);
                    const auto shapeCoords = shape.getTransform().getInverse() * coords;
                    shape.setPoint(0, shapeCoords);
                }
                break;
                case sf::Event::MouseWheelScrolled:
                    shape.setOutlineThickness(shape.getOutlineThickness() + event.mouseWheelScroll.delta);
                    break;
                default:
                    break;
            }
        }

        window.clear();
        window.draw(shape);
        window.display();
    }
}

Program can be built with:

cmake_minimum_required(VERSION 3.22)
project(shape-test CXX)

include(FetchContent)
FetchContent_Declare(SFML
    GIT_REPOSITORY https://github.com/kimci86/SFML.git
    GIT_TAG feature/shape_miter_limit)
FetchContent_MakeAvailable(SFML)

add_executable(shape-test main.cpp)
target_link_libraries(shape-test PRIVATE SFML::Graphics)

codecov · 2023-10-21T23:07:19Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 41.97%. Comparing base (6eaf300) to head (6383b34).
Report is 12 commits behind head on master.

❗ Current head 6383b34 differs from pull request most recent head 0b9c80d. Consider uploading reports for the commit 0b9c80d to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #2741      +/-   ##
==========================================
- Coverage   43.65%   41.97%   -1.69%     
==========================================
  Files         253      251       -2     
  Lines       20940    21135     +195     
  Branches     5139     5196      +57     
==========================================
- Hits         9142     8871     -271     
- Misses      10785    11271     +486     
+ Partials     1013      993      -20

Files	Coverage Δ
include/SFML/Graphics/Shape.hpp	`75.00% <100.00%> (+3.57%)`	⬆️
src/SFML/Graphics/Shape.cpp	`93.42% <100.00%> (-2.71%)`	⬇️

... and 172 files with indirect coverage changes

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4315c3d...0b9c80d. Read the comment docs.

kimci86 · 2023-10-22T00:22:00Z

I forgot about getGeometricCenter being virtual. I will rework my code to take advantage of this potential optimization.
Edit: Done.

ChrisThrasher · 2023-10-23T04:21:49Z

I'm so glad you implemented this :) I have a few design questions

Why not make the miter size a parameter? Is there a particularly useful reason why it ought to be a hardcoded constant? Is it simply not useful to add an API for it?
Why make this feature on by default? My impression at first was that this would be an opt-in feature since it changes how drawing works to something that is perhaps not what users expect at first. This ties into the first question since to opt into this feature entails adding a new API.
The transition from a non-mitered to a mitered state is rather abrupt. Is there anything we can do to make that a bit smoother? What I was initially picturing was that as an internal angle gets more acute, instead of the external point on the outlet moving farther out, we see the miter open up. This is in contrast to the current implementation where the point on the triangle get shorter, so to speak. That abrupt shortening is what I'd prefer to eliminate if feasible.

I can help figure out how to test this once we talk through more of the higher level API concerns.

kimci86 · 2023-10-23T06:05:28Z

No reason besides me not being brave enough to write the documentation right away. The limit probably should be a parameter.
The feature being off means outlining a shape around a zero angle is a bug. I think the limit should never be off but I agree users may want to set a higher limit.
I took inspiration from SVG behavior not to need to get creative but we can implement whatever we want for sure. I will experiment with some smoother alternative soon, ~~hopefully tonight~~.

Thank you for your feedback!

ChrisThrasher · 2023-10-23T15:40:06Z

I noticed and fixed another issue that was visible with my test program below. See 71826de.

I just realized you did this. Nice find! This is a huge vote of confident for getGeometricCenter as a feature.

ChrisThrasher · 2023-10-23T16:48:22Z

https://developer.mozilla.org/en-US/docs/Web/SVG/Attribute/stroke-miterlimit

In the graphic that shows 3 different miter sizes, the middle one is what looks more desirable to me. Is this a fundamentally different behavior or simply demonstrating a different miter value? If the middle behavior is achievable via a different miter size then maybe you don't need to change the underlying algorithm.

kimci86 · 2023-10-26T22:34:25Z

In the graphic that shows 3 different miter sizes, the middle one is what looks more desirable to me. Is this a fundamentally different behavior or simply demonstrating a different miter value?

It is the same behavior but with different miter limit value.

I started over. I was not really satisfied with the geometric center because, even though we are not supposed to draw crossed polygons, it can look very wrong using the geometric center instead of looking just a little wrong with the bounding box's center.

Regarding the transition between miter and bevel, I came up with something smooth. Here is what it looks like:

smooth.mp4

Next step: adding method to set the miter limit, and also some optimization (vectors are computed twice instead of being reused).

kimci86 · 2023-10-28T22:48:20Z

Now works with negative outline too as it was supposed to be.
Added getter and setter for the miter limit.

lapinozz · 2023-10-29T03:32:49Z

2023-10-28.23-30-22.mp4

In paint.net when I reach the miter limit it doesn't "shrink" the end. imo it looks cleaner.
I don't know what kind of algorithm it involves. Just thought it might be something to look at.

kimci86 · 2023-10-29T18:55:06Z

@lapinozz Thank you for the suggestion, I like the idea. I just added a commit implementing it.

lapinozz · 2023-10-30T14:57:22Z

With the third point at {-100, -0.25f} and miter limit set to 1 I get

Maybe it's too close to all points being collinear and shouldn't be considered a bug? There are other somewhat glitchy behaviours around that point.

I also think it shouldn't crash for miter limit under 1

kimci86 · 2023-10-30T18:16:16Z

Maybe it's too close to all points being collinear and shouldn't be considered a bug?

I don't see a problem in the screenshot. What would you expect?

There are other somewhat glitchy behaviours around that point.

True... Not sure what can be done about that. At least it is not worse than on master.

I also think it shouldn't crash for miter limit under 1

What do you suggest? We could clamp the user provided value to be at least 1. Would that deserve printing a warning to the console?

ChrisThrasher · 2023-10-30T18:34:42Z

We could clamp the user provided value to be at least 1. Would that deserve printing a warning to the console?

I think the assert is good. Our API is only valid for a subset of float values so it's reasonable to assert against the invalid values. Doing so will help users catch bugs earlier in development.

Perhaps we also add a std::max call to ensure that in release builds the value remains greater than or equal to 1? I guess it depends how bad it is to use a value less than 1. Does that trigger UB? Does it create visual glitches? Some other problem?

src/SFML/Graphics/Shape.cpp

kimci86 · 2023-10-30T18:52:44Z

Does that trigger UB? Does it create visual glitches? Some other problem?

For an angle that is large enough, a miter limit lower than 1 will give a glitchy look. It can trigger assert in Vector2 division operator in debug mode.

I don't really see a valid use case.

Note that stroke-miterlimit SVG property documentation mentions that constraint:

The value of stroke-miterlimit must be greater than or equal to 1.

lapinozz · 2023-10-31T02:20:25Z

I don't see a problem in the screenshot. What would you expect?

The clipped off corner don't make any sense

kimci86 · 2023-10-31T18:45:40Z

The clipped off corner don't make any sense

The outlined shape is a right triangle with one very small side. For me it makes sense. What result would you expect?

kimci86 · 2023-11-04T00:47:49Z

Rebased on master, squashed related commits and updated video of the result.

ChrisThrasher

I finally ran this PR and the results look so good! This feature behaves exactly how I hoped it would. I have a few minor nits about code style but no major comments on the structure the implementation.

Two requests/questions:

What was the rationale for picking 4 as the default value? I think I'd prefer a value closer to 10 judging by some local tests I ran. I think 4 is a bit too aggressive of a default value.

Can you write some sf::ConvexShape tests which test the bounds of a shape before and after setting the miter limit so we can ensure that the bounds do in fact change? You can use https://github.com/SFML/SFML/pull/2762 for inspiration.

ChrisThrasher · 2023-11-05T21:56:15Z

src/SFML/Graphics/Shape.cpp

-    sf::Vector2f normal = (p2 - p1).perpendicular();
-    const float  length = normal.length();
+    sf::Vector2f direction = (p2 - p1);
+    const float  length    = direction.length();


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

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.

src/SFML/Graphics/Shape.cpp

include/SFML/Graphics/Shape.hpp

kimci86 · 2023-11-06T19:53:08Z

What was the rationale for picking 4 as the default value?

I opened Inkscape editor and looked at the default value there. I will change to 10. (Done)

eXpl0it3r · 2023-11-09T19:58:50Z

include/SFML/Graphics/Shape.hpp

+    /// \see setMiterLimit
+    ///
+    ////////////////////////////////////////////////////////////
+    float getMiterLimit() const;


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.

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.

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.

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?

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.

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.

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

eXpl0it3r

Just a few small comments 😃

eXpl0it3r · 2023-11-09T20:00:15Z

include/SFML/Graphics/Shape.hpp

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


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.

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

eXpl0it3r · 2023-11-09T20:04:40Z

src/SFML/Graphics/Shape.cpp

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


Not sure if we should write "defensive" code like that. Yes, it will warn in debug mode, but it will also silently "work" in release mode when you set a value less than 1.

What's the effect of allowing negative numbers? Would it crash or just render distorted?

Miter limit lower than 1 is asking outline not to grow bigger around angle than the outline thickness. Basically the limit is always reached. It makes little sense.
Having miter limit lower than 1 gives strange look and can run into Vector2 assert about division by 0.

Here are some example of what it looks like starting at 1 (the lowest valid limit) and going down. The result is especially wrong when angles are large enough (3rd example).

@Bromeon Since we've had discussion about bad input handling, what's your view on doing assert + limit?

I'm not sure if we do assert + limit anywhere else. It may be more in line with other APIs if we assert but otherwise allow the original value to be used. For example sf::WindowBase::setMinimumSize asserts a valid minimum size (it can't be large than the maximum window size) but it doesn't clamp the minimum size to be valid. The invalid value continues to be used and whatever happens after that is unspecified.

Difficult question. In an ideal world, the developer would catch the assert during development.

But if it slips through, it's probably better if it doesn't cause catastrophic damage.
However, "distorted rendering" is not catastrophic in my opinion (on the contrary, it could increase chances of finding such bugs). So I'm fine with just doing assert (no limit), if that only results in problematic visuals.

This is a hard problem, and I haven't seen any library/framework/engine handle errors always gracefully. Attempts to do so often result in excessively verbose error handling code, and other approaches (exceptions, panics, aborts) often crash the app/game for minuscule bugs.

Ok, I removed the std::max call.

src/SFML/Graphics/Shape.cpp

kimci86 · 2023-12-04T20:10:36Z

Rebased on master and cleaned up history.

Hapaxia · 2023-12-30T16:08:35Z

Since I rarely use sf::Shape and pretty much never use sf::ConvexShape, I wasn't aware this issue exists. Adding an outline to a sf::Shape is effectively creating a thick polyline (spline) around that shape and there are a few issues I've encountered with thick polylines. I'll try to recall what I can and share some insight if I remember anything of use.

As far naming, I'd say, "bevel" is a better name (for corner cutting) and it's certainly what I've experienced most of the time when working with vector graphics and their editors.

The previous implementation could fail to determine if normals had to be flipped or not: it assumed the first vertex position was strictly inside the shape. This position being set to the bounding box's center, it is possible for it be on an edge of the shape (consider a right triangle).

coveralls · 2024-04-20T16:07:36Z

Pull Request Test Coverage Report for Build 8764194507

Details

53 of 53 (100.0%) changed or added relevant lines in 2 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage increased (+0.1%) to 53.149%

Totals
Change from base Build 8736987266:	0.1%
Covered Lines:	10386
Relevant Lines:	18544

💛 - Coveralls

kimci86 force-pushed the feature/shape_miter_limit branch from 96b7ccd to c23ab57 Compare October 22, 2023 12:16

eXpl0it3r added bug m:sfml-graphics labels Oct 22, 2023

eXpl0it3r added this to the 3.0 milestone Oct 22, 2023

ChrisThrasher requested review from eXpl0it3r and binary1248 October 23, 2023 04:22

kimci86 force-pushed the feature/shape_miter_limit branch from c23ab57 to e6d0d66 Compare October 26, 2023 22:23

kimci86 marked this pull request as draft October 26, 2023 22:39

kimci86 force-pushed the feature/shape_miter_limit branch 2 times, most recently from 5ff9901 to 812ee1d Compare October 28, 2023 22:43

kimci86 marked this pull request as ready for review October 28, 2023 22:48

ChrisThrasher requested changes Oct 30, 2023

View reviewed changes

src/SFML/Graphics/Shape.cpp Outdated Show resolved Hide resolved

kimci86 requested a review from ChrisThrasher October 30, 2023 19:40

kimci86 force-pushed the feature/shape_miter_limit branch from 05755d7 to 32d119f Compare November 4, 2023 00:42

ChrisThrasher mentioned this pull request Nov 4, 2023

Add tests for how outlines change the bounds of an sf::Shape #2762

Merged

ChrisThrasher reviewed Nov 5, 2023

View reviewed changes

include/SFML/Graphics/Shape.hpp Show resolved Hide resolved

kimci86 force-pushed the feature/shape_miter_limit branch 2 times, most recently from 72427c0 to 183da8b Compare November 6, 2023 21:17

kimci86 requested a review from ChrisThrasher November 6, 2023 21:20

ChrisThrasher approved these changes Nov 6, 2023

View reviewed changes

ChrisThrasher requested a review from Bromeon November 6, 2023 22:10

eXpl0it3r reviewed Nov 9, 2023

View reviewed changes

eXpl0it3r requested changes Nov 9, 2023

View reviewed changes

kimci86 force-pushed the feature/shape_miter_limit branch 2 times, most recently from 7dd8bcb to f324459 Compare November 19, 2023 23:12

kimci86 requested a review from eXpl0it3r November 19, 2023 23:17

kimci86 force-pushed the feature/shape_miter_limit branch from f324459 to fc1533d Compare December 4, 2023 20:09

kimci86 force-pushed the feature/shape_miter_limit branch from fc1533d to 6383b34 Compare December 10, 2023 16:20

ChrisThrasher modified the milestones: 3.0, 3.1 Jan 25, 2024

kimci86 added 2 commits April 19, 2024 00:44

Bevel shape outline corners beyond threshold

0b9c80d

kimci86 force-pushed the feature/shape_miter_limit branch from 6383b34 to 0b9c80d Compare April 20, 2024 09:49

Bevel shape outline beyond threshold #2741

Are you sure you want to change the base?

Bevel shape outline beyond threshold #2741

Conversation

kimci86 commented Oct 21, 2023 • edited

Description

Tasks

How to test this PR?

codecov bot commented Oct 21, 2023 • edited

Codecov Report

kimci86 commented Oct 22, 2023 • edited

ChrisThrasher commented Oct 23, 2023

kimci86 commented Oct 23, 2023 • edited

ChrisThrasher commented Oct 23, 2023

ChrisThrasher commented Oct 23, 2023 • edited

kimci86 commented Oct 26, 2023 • edited

kimci86 commented Oct 28, 2023

lapinozz commented Oct 29, 2023

kimci86 commented Oct 29, 2023

lapinozz commented Oct 30, 2023

kimci86 commented Oct 30, 2023

ChrisThrasher commented Oct 30, 2023 • edited

kimci86 commented Oct 30, 2023

lapinozz commented Oct 31, 2023

kimci86 commented Oct 31, 2023

kimci86 commented Nov 4, 2023

ChrisThrasher left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kimci86 commented Nov 6, 2023 • edited

Choose a reason for hiding this comment

ChrisThrasher Nov 9, 2023 • edited

Choose a reason for hiding this comment

kimci86 Nov 9, 2023 • edited

Choose a reason for hiding this comment

eXpl0it3r Nov 9, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ChrisThrasher Nov 14, 2023 • edited

Choose a reason for hiding this comment

eXpl0it3r left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kimci86 Nov 19, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Bromeon Nov 15, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kimci86 commented Dec 4, 2023

Hapaxia commented Dec 30, 2023

coveralls commented Apr 20, 2024

Pull Request Test Coverage Report for Build 8764194507

Details

💛 - Coveralls

kimci86 commented Oct 21, 2023 •

edited

codecov bot commented Oct 21, 2023 •

edited

kimci86 commented Oct 22, 2023 •

edited

kimci86 commented Oct 23, 2023 •

edited

ChrisThrasher commented Oct 23, 2023 •

edited

kimci86 commented Oct 26, 2023 •

edited

ChrisThrasher commented Oct 30, 2023 •

edited

kimci86 commented Nov 6, 2023 •

edited

ChrisThrasher Nov 9, 2023 •

edited

kimci86 Nov 9, 2023 •

edited

eXpl0it3r Nov 9, 2023 •

edited

ChrisThrasher Nov 14, 2023 •

edited

kimci86 Nov 19, 2023 •

edited

Bromeon Nov 15, 2023 •

edited