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

Add sf::Shape::getGeometricCenter() #2536

Closed
wants to merge 1 commit into from

Conversation

TedLyngmo
Copy link
Contributor

@TedLyngmo TedLyngmo commented Apr 24, 2023

This adds

virtual Vector2f sf::Shape::getGeometricCenter() const = 0;

as discussed here:
https://en.sfml-dev.org/forums/index.php?topic=29011

One practical use can be to set a rotation point in the middle of a circle, rectangle or convex shape.

Example:

#include <SFML/Graphics.hpp>

#include <initializer_list>

class Boulder : public sf::ConvexShape {
public:
    inline Boulder(std::initializer_list<sf::Vector2f> pnts) :
        sf::ConvexShape(pnts.size())
    {
        for(std::size_t idx = 0; idx < pnts.size(); ++idx)
        {
            setPoint(idx, pnts.begin()[idx]);
        }

        // Set the rotation point at the geometrical center of the convex shape:
        setOrigin(getGeometricCenter());
    }
};

int main()
{
    sf::RenderWindow window(sf::VideoMode({1280, 720}), "Minimal, complete and verifiable example");
    window.setFramerateLimit(60);

    Boulder boulder1({{20,0}, {60,0}, {80,40}, {70,60}, {30,80}, {10,60}, {0,30}});
    boulder1.setPosition({1280/3, 720/3});

    Boulder boulder2({{20,0}, {60,0}, {180,40}, {70,60}, {30,80}, {10,60}, {0,30}});
    boulder2.setPosition({1280*2/3, 720/3});

    sf::RectangleShape rectangle({100,50});
    rectangle.setOrigin(rectangle.getGeometricCenter());
    rectangle.setPosition({1280/2, 720*2/3});

    sf::CircleShape circle1(40); // center slightly off to wobble a bit:
    circle1.setOrigin(circle1.getGeometricCenter() + sf::Vector2(5.f,5.f));
    circle1.setPosition({1280/3, 720*2/3});

    sf::CircleShape circle2(40,3);
    circle2.setOrigin(circle1.getGeometricCenter());
    circle2.setPosition({1280*2/3, 720*2/3});

    while (window.isOpen())
    {
        for (sf::Event event; window.pollEvent(event);)
        {
            if (event.type == sf::Event::Closed)
                window.close();
        }

        boulder1.rotate(sf::degrees(2));
        boulder2.rotate(sf::degrees(3));
        rectangle.rotate(sf::degrees(4));
        circle1.rotate(sf::degrees(5));
        circle2.rotate(sf::degrees(5));

        window.clear();
        window.draw(boulder1);
        window.draw(boulder2);
        window.draw(rectangle);
        window.draw(circle1);
        window.draw(circle2);
        window.display();
    }
}
  • Has this change been discussed on the forum or in an issue before?
  • Does the code follow the SFML Code Style Guide?
  • Have you provided some example/test code for your changes?
  • If you have additional steps which need to be performed list them as tasks!

Tasks

  • Tested on Linux
  • Tested on Windows
  • Tested on macOS
  • Tested on iOS
  • Tested on Android

@codecov
Copy link

codecov bot commented Apr 24, 2023

Codecov Report

Merging #2536 (8e391ca) into master (97c00d4) will increase coverage by 0.11%.
The diff coverage is 96.87%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2536      +/-   ##
==========================================
+ Coverage   27.15%   27.26%   +0.11%     
==========================================
  Files         228      228              
  Lines       19637    19669      +32     
  Branches     4710     4716       +6     
==========================================
+ Hits         5332     5363      +31     
- Misses      13826    13847      +21     
+ Partials      479      459      -20     
Impacted Files Coverage Δ
include/SFML/Graphics/CircleShape.hpp 100.00% <ø> (ø)
include/SFML/Graphics/ConvexShape.hpp 100.00% <ø> (ø)
include/SFML/Graphics/RectangleShape.hpp 100.00% <ø> (ø)
include/SFML/Graphics/Shape.hpp 100.00% <ø> (ø)
src/SFML/Graphics/ConvexShape.cpp 97.72% <95.45%> (-2.28%) ⬇️
src/SFML/Graphics/CircleShape.cpp 100.00% <100.00%> (ø)
src/SFML/Graphics/RectangleShape.cpp 97.05% <100.00%> (+0.39%) ⬆️

... and 3 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 97c00d4...8e391ca. Read the comment docs.

@TedLyngmo TedLyngmo force-pushed the shape_getGeometricCenter branch 5 times, most recently from c53b8b8 to f4b49aa Compare April 24, 2023 10:34
Copy link
Member

@vittorioromeo vittorioromeo left a comment

Choose a reason for hiding this comment

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

Indifferent on whether we should add this to SFML or not in general

include/SFML/Graphics/RectangleShape.hpp Show resolved Hide resolved
src/SFML/Graphics/RectangleShape.cpp Outdated Show resolved Hide resolved
src/SFML/Graphics/ConvexShape.cpp Outdated Show resolved Hide resolved
src/SFML/Graphics/ConvexShape.cpp Outdated Show resolved Hide resolved
if (not m_points.empty())
return m_points.front(); // a single point

return Vector2f{}; // empty
Copy link
Member

Choose a reason for hiding this comment

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

Not too sure about this one. It's impossible to differentiate between a polygon whose geometrical center is in 0,0 and a polygon with no points. Maybe this function should return std::optional<Vector2f>?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's an option or it could throw an exception or leave the case UB

Copy link
Member

Choose a reason for hiding this comment

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

Or assert.

I'm not sure about optional here, the original problem is that sf::ConvexShape can be in a "degenerate" state -- it would just make users dereference the optional all the time, possibly causing more UB than assert (depending on whether optional::operator* has also an assert).

Copy link
Contributor Author

@TedLyngmo TedLyngmo Apr 24, 2023

Choose a reason for hiding this comment

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

@Bromeon Sounds like a good idea. What to do in NDEBUG builds? std::terminate or throw std::out_of_range?

Comment on lines +81 to +82
if (m_pointCount == 3)
return (getPoint(0) + getPoint(1) + getPoint(2)) / 3.f;
Copy link
Member

Choose a reason for hiding this comment

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

What's the significance of special-casing for 3 points?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@vittorioromeo It's the one case where the geometric center is not @ Vector2f(m_radius, m_radius).

src/SFML/Graphics/ConvexShape.cpp Outdated Show resolved Hide resolved
@TedLyngmo TedLyngmo force-pushed the shape_getGeometricCenter branch 4 times, most recently from 049638b to c380f45 Compare April 24, 2023 11:24
@TedLyngmo
Copy link
Contributor Author

@vittorioromeo & @Bromeon Would it perhaps be better to place the generic algorithm in Shape instead of ConvexShape?

@TedLyngmo TedLyngmo force-pushed the shape_getGeometricCenter branch 2 times, most recently from 863b908 to 93a70fa Compare April 24, 2023 16:18
This adds

    virtual Vector2f getGeometricCenter() const = 0;

Signed-off-by: Ted Lyngmo <ted@lyncon.se>
@ChrisThrasher
Copy link
Member

Which PR do you want to keep open. This one or #2537? I don't think we need both.

@TedLyngmo
Copy link
Contributor Author

Which PR do you want to keep open. This one or #2537? I don't think we need both.

I wasn't sure if you'd be ok with adding the implementation to the Shape class so I made two separate patches - but it seems we all agree that the other patch is better so I'll close this.

@TedLyngmo TedLyngmo closed this Apr 24, 2023
@TedLyngmo TedLyngmo deleted the shape_getGeometricCenter branch April 24, 2023 21:12
@eXpl0it3r eXpl0it3r added this to the 3.0 milestone May 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

5 participants