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

sf::Text::findCharacterPos() doesn't report the correct Y location #2988

Open
3 tasks done
dogunbound opened this issue May 9, 2024 · 8 comments · May be fixed by #2999
Open
3 tasks done

sf::Text::findCharacterPos() doesn't report the correct Y location #2988

dogunbound opened this issue May 9, 2024 · 8 comments · May be fixed by #2999

Comments

@dogunbound
Copy link
Contributor

Prerequisite Checklist

Describe your issue here

findCharacterPos is offsetting the Y position too high and reporting incorrect values.

Your Environment

  • OS / distro / window manager: linux / kde / x11
  • SFML version: 2.6.1
  • Compiler / toolchain: CMake sample project
  • Special compiler / CMake flags: N/A

Steps to reproduce

Run this code:

#include "SFML/Graphics/CircleShape.hpp"
#include "SFML/Graphics/Color.hpp"
#include "SFML/Graphics/Font.hpp"
#include "SFML/Graphics/Rect.hpp"
#include "SFML/Graphics/RectangleShape.hpp"
#include "SFML/Graphics/RenderWindow.hpp"
#include "SFML/Graphics/Text.hpp"
#include "SFML/System/Vector2.hpp"
#include "SFML/Window/Event.hpp"
#include "SFML/Window/VideoMode.hpp"
#include <cstddef>
#include <cstdio>
#include <optional>

void print_float_rect(const sf::FloatRect &rect) {
  printf("top: %lf left: %lf width: %lf height: %lf\n", rect.top, rect.left,
         rect.width, rect.height);
}

void print_float_vector(const sf::Vector2f &vec) {
  printf("x: %lf y:%lf\n", vec.x, vec.y);
}

std::optional<float> get_character_width_at_idx(const sf::Text &text,
                                                size_t idx) {
  float width = text.findCharacterPos(idx + 1).x - text.findCharacterPos(idx).x;
  if (width < 0) {
    return {};
  }

  return {width};
}

int main() {
  sf::RenderWindow window(sf::VideoMode(200, 200), "SFML Works!");
  window.setFramerateLimit(60);

  sf::Font font;
  if (!font.loadFromFile("SourceCodePro-SemiBold.ttf")) {
    printf("Failed to load font!\n");
    return 1;
  }

  sf::Text text;
  text.setFont(font);
  text.setCharacterSize(32);
  text.setString("M");
  text.setPosition(sf::Vector2f(100, 100));

  sf::CircleShape mouse_dot;
  mouse_dot.setFillColor(sf::Color::Red);
  float radius = 3;
  mouse_dot.setRadius(radius);
  mouse_dot.setOrigin(radius / 2, radius / 2);
  mouse_dot.setPosition(sf::Vector2f(-100, -100));

  sf::RectangleShape letter_box;
  letter_box.setOutlineColor(sf::Color::Blue);
  letter_box.setOutlineThickness(2);
  letter_box.setFillColor(sf::Color::Transparent);
  letter_box.setPosition(sf::Vector2f(-100, -100));

  while (window.isOpen()) {
    sf::Event event;
    while (window.pollEvent(event)) {
      switch (event.type) {
      case sf::Event::Closed:
        window.close();
        break;
      case sf::Event::MouseButtonPressed: {
        sf::Vector2f position(event.mouseButton.x, event.mouseButton.y);
        sf::Vector2f char_pos = text.findCharacterPos(0);
        sf::FloatRect rect(char_pos.x, char_pos.y,
                           get_character_width_at_idx(text, 0).value_or(0),
                           text.getGlobalBounds().height);

        print_float_rect(rect);
        print_float_vector(position);
        printf("letter contains mouse %s\n",
               rect.contains(position) ? "true" : "false");

        letter_box.setSize(rect.getSize());
        letter_box.setPosition(rect.getPosition());
        mouse_dot.setPosition(position);
      } break;
      default:
        break;
      }
    }
    window.clear(sf::Color::Black);
    window.draw(text);
    window.draw(mouse_dot);
    window.draw(letter_box);
    window.display();
  }

  return 0;
}

Expected behavior

That blue box around the letter should actually be around the letter.

Actual behavior

The blue box is offset up due to an incorrect calculation from findCharacterPos

image

@kimci86
Copy link
Contributor

kimci86 commented May 9, 2024

I am not sure this is a bug. I think there is a misunderstanding of what findCharacterPos is doing. Probably the documentation is too vague.

You can achieve what you want with glyph bounds provided by the font.
Example:

#include <SFML/Graphics.hpp>

int main() {
  sf::RenderWindow window(sf::VideoMode(400, 400), "SFML Works!");
  window.setFramerateLimit(60);
  window.setView(sf::View(sf::FloatRect({}, {100, 100})));

  sf::Font font;
  if (!font.loadFromFile("SourceCodePro-SemiBold.ttf")) {
    return 1;
  }

  sf::Text text("M", font, 32);
  text.setPosition({50.f, 10.f});
  text.setScale({2.f, 2.f});
  text.setRotation(20.f);
  const sf::FloatRect glyphBounds =
      font.getGlyph(text.getString()[0], text.getCharacterSize(), false).bounds;

  sf::RectangleShape box;
  box.setOutlineColor(sf::Color::Blue);
  box.setOutlineThickness(2);
  box.setFillColor(sf::Color::Transparent);
  // relative to text coordinate system
  box.setPosition(text.getInverseTransform() * text.findCharacterPos(0) +
                  sf::Vector2f(0, static_cast<float>(text.getCharacterSize())) +
                  glyphBounds.getPosition());
  box.setSize(glyphBounds.getSize());
  const sf::FloatRect boxRect(box.getPosition(), box.getSize());

  while (window.isOpen()) {
    for (sf::Event event; window.pollEvent(event);) {
      switch (event.type) {
      case sf::Event::Closed:
        window.close();
        break;
      case sf::Event::MouseMoved: {
        const sf::Vector2i mouseInWindow(event.mouseMove.x, event.mouseMove.y);
        const sf::Vector2f mouseInWorld =
            window.mapPixelToCoords(mouseInWindow);
        const sf::Vector2f mouseInText =
            text.getInverseTransform() * mouseInWorld;
        box.setOutlineColor(boxRect.contains(mouseInText) ? sf::Color::Green
                                                          : sf::Color::Blue);
      } break;
      default:
        break;
      }
    }

    window.clear();
    window.draw(text);
    window.draw(box, text.getTransform());
    window.display();
  }
}

@dogunbound
Copy link
Contributor Author

For reference, this is the documentation you are talking about: https://www.sfml-dev.org/documentation/2.6.1/classsf_1_1Text.php#a2e252d8dcae3eb61c6c962c0bc674b12

It says it applies all transformations including translation. Currently it doesn't look like it is applying the local bound translation, hence why I added it. Also, it seems a little weird from a user perspective that I have to do all of that to find the actual character position. If the current implementation is correct, wouldn't that suggest we should rename the function?

@dogunbound
Copy link
Contributor Author

dogunbound commented May 10, 2024

I am not sure this is a bug. I think there is a misunderstanding of what findCharacterPos is doing. Probably the documentation is too vague.

You can achieve what you want with glyph bounds provided by the font. Example:

#include <SFML/Graphics.hpp>

int main() {
  sf::RenderWindow window(sf::VideoMode(400, 400), "SFML Works!");
  window.setFramerateLimit(60);
  window.setView(sf::View(sf::FloatRect({}, {100, 100})));

  sf::Font font;
  if (!font.loadFromFile("SourceCodePro-SemiBold.ttf")) {
    return 1;
  }

  sf::Text text("M", font, 32);
  text.setPosition({50.f, 10.f});
  text.setScale({2.f, 2.f});
  text.setRotation(20.f);
  const sf::FloatRect glyphBounds =
      font.getGlyph(text.getString()[0], text.getCharacterSize(), false).bounds;

  sf::RectangleShape box;
  box.setOutlineColor(sf::Color::Blue);
  box.setOutlineThickness(2);
  box.setFillColor(sf::Color::Transparent);
  // relative to text coordinate system
  box.setPosition(text.getInverseTransform() * text.findCharacterPos(0) +
                  sf::Vector2f(0, static_cast<float>(text.getCharacterSize())) +
                  glyphBounds.getPosition());
  box.setSize(glyphBounds.getSize());
  const sf::FloatRect boxRect(box.getPosition(), box.getSize());

  while (window.isOpen()) {
    for (sf::Event event; window.pollEvent(event);) {
      switch (event.type) {
      case sf::Event::Closed:
        window.close();
        break;
      case sf::Event::MouseMoved: {
        const sf::Vector2i mouseInWindow(event.mouseMove.x, event.mouseMove.y);
        const sf::Vector2f mouseInWorld =
            window.mapPixelToCoords(mouseInWindow);
        const sf::Vector2f mouseInText =
            text.getInverseTransform() * mouseInWorld;
        box.setOutlineColor(boxRect.contains(mouseInText) ? sf::Color::Green
                                                          : sf::Color::Blue);
      } break;
      default:
        break;
      }
    }

    window.clear();
    window.draw(text);
    window.draw(box, text.getTransform());
    window.display();
  }
}

Also i really think it's a bug now that I've done more digging. If you look at my test code, you will see the code I'm using to run this test.

So the blue box is an sf::Rect that uses the global bounds from the text object to represent itself. This is what it looks like:
image

But with the current 2.6.x code, whenever you use the value from "findCharacterPos" as the top left corner of your rect that surrounds the character, you get this:
image

I don't think it makes sense for the return value of the "top left" coordinate to be above the global bounds value that you get from rect.

For reference, if you do the fix lapinozz suggests in the PR I opened, you will see the top left corner match the global bounds top left corner. This is a picture running the same exact test code, but with the SFML fix:
image

I really think it's a bug here and not a documentation issue.

@kimci86
Copy link
Contributor

kimci86 commented May 11, 2024

For reference, if you do the fix lapinozz suggests in the PR I opened, you will see the top left corner match the global bounds top left corner.

This works for the first letter but it is wrong in general.
Example running your test code with your suggested fix when the first character is an underscore.

Capture d’écran du 2024-05-11 20-11-28
Capture d’écran du 2024-05-11 20-11-44


Consider the following program and a corresponding screenshot.

I think findCharacterPos current behavior is valid and useful to position a vertical cursor in a text field or to highlight selected text. I use it to draw the background pattern in my example below. Note it also gives a sensible output for the space character.
One problem I see is that having a local position would be more useful that a global position when there is some rotation (that is why I transform findCharacterPos output with text.getInverseTransform()), but returning global position is the documented behavior so all good.

To get a tight bounding box on a specific character, glyph bounds give the needed information.

#include <SFML/Graphics.hpp>

int main()
{
    auto window = sf::RenderWindow{sf::VideoMode{800, 600}, "Text", sf::Style::Default};
    window.setFramerateLimit(60);

    auto font = sf::Font{};
    if (!font.loadFromFile("resources/tuffy.ttf"))
        return 1;

    constexpr auto characterSize = 200;

    auto text = sf::Text{L"ïyÏ~Ô @!", font, characterSize};
    text.setPosition(100.f, 100.f);
    text.setRotation(10.f);

    auto backgroundRects = std::vector<sf::RectangleShape>{};
    for (auto i = std::size_t{}; i < text.getString().getSize(); i++)
    {
        const auto position = text.getInverseTransform() * text.findCharacterPos(i);
        const auto next     = text.getInverseTransform() * text.findCharacterPos(i + 1);
        const auto size     = sf::Vector2f{next.x - position.x, font.getLineSpacing(characterSize)};
        auto&      rect     = backgroundRects.emplace_back(size);
        rect.setPosition(position);
        rect.setFillColor(i % 2 ? sf::Color{64, 64, 0} : sf::Color{0, 64, 64});
    }

    auto letterBounds = std::vector<sf::RectangleShape>{};
    for (auto i = std::size_t{}; i < text.getString().getSize(); i++)
    {
        const auto bounds   = font.getGlyph(text.getString()[i], characterSize, false).bounds;
        const auto position = text.getInverseTransform() * text.findCharacterPos(i) + sf::Vector2f{0.f, characterSize} +
                              bounds.getPosition();
        auto& rect = letterBounds.emplace_back(bounds.getSize());
        rect.setPosition(position);
        rect.setFillColor(sf::Color::Transparent);
        rect.setOutlineColor(sf::Color::Green);
        rect.setOutlineThickness(1.f);
    }

    auto textBounds = sf::RectangleShape{};
    {
        const auto bounds = text.getLocalBounds();
        textBounds.setPosition(bounds.getPosition());
        textBounds.setSize(bounds.getSize());
        textBounds.setFillColor(sf::Color::Transparent);
        textBounds.setOutlineColor(sf::Color::Red);
        textBounds.setOutlineThickness(1.f);
    }

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

        window.clear();
        for (const auto& rect : backgroundRects)
            window.draw(rect, text.getTransform());
        window.draw(text);
        for (const auto& bounds : letterBounds)
            window.draw(bounds, text.getTransform());
        window.draw(textBounds, sf::RenderStates{sf::BlendAdd, text.getTransform(), nullptr, nullptr});
        window.display();
    }

    return 0;
}

Capture d’écran

@dogunbound
Copy link
Contributor Author

dogunbound commented May 12, 2024

For reference, if you do the fix lapinozz suggests in the PR I opened, you will see the top left corner match the global bounds top left corner.

This works for the first letter but it is wrong in general. Example running your test code with your suggested fix when the first character is an underscore.

Capture d’écran du 2024-05-11 20-11-28 Capture d’écran du 2024-05-11 20-11-44

Consider the following program and a corresponding screenshot.

I think findCharacterPos current behavior is valid and useful to position a vertical cursor in a text field or to highlight selected text. I use it to draw the background pattern in my example below. Note it also gives a sensible output for the space character. One problem I see is that having a local position would be more useful that a global position when there is some rotation (that is why I transform findCharacterPos output with text.getInverseTransform()), but returning global position is the documented behavior so all good.

To get a tight bounding box on a specific character, glyph bounds give the needed information.

#include <SFML/Graphics.hpp>

int main()
{
    auto window = sf::RenderWindow{sf::VideoMode{800, 600}, "Text", sf::Style::Default};
    window.setFramerateLimit(60);

    auto font = sf::Font{};
    if (!font.loadFromFile("resources/tuffy.ttf"))
        return 1;

    constexpr auto characterSize = 200;

    auto text = sf::Text{L"ïyÏ~Ô @!", font, characterSize};
    text.setPosition(100.f, 100.f);
    text.setRotation(10.f);

    auto backgroundRects = std::vector<sf::RectangleShape>{};
    for (auto i = std::size_t{}; i < text.getString().getSize(); i++)
    {
        const auto position = text.getInverseTransform() * text.findCharacterPos(i);
        const auto next     = text.getInverseTransform() * text.findCharacterPos(i + 1);
        const auto size     = sf::Vector2f{next.x - position.x, font.getLineSpacing(characterSize)};
        auto&      rect     = backgroundRects.emplace_back(size);
        rect.setPosition(position);
        rect.setFillColor(i % 2 ? sf::Color{64, 64, 0} : sf::Color{0, 64, 64});
    }

    auto letterBounds = std::vector<sf::RectangleShape>{};
    for (auto i = std::size_t{}; i < text.getString().getSize(); i++)
    {
        const auto bounds   = font.getGlyph(text.getString()[i], characterSize, false).bounds;
        const auto position = text.getInverseTransform() * text.findCharacterPos(i) + sf::Vector2f{0.f, characterSize} +
                              bounds.getPosition();
        auto& rect = letterBounds.emplace_back(bounds.getSize());
        rect.setPosition(position);
        rect.setFillColor(sf::Color::Transparent);
        rect.setOutlineColor(sf::Color::Green);
        rect.setOutlineThickness(1.f);
    }

    auto textBounds = sf::RectangleShape{};
    {
        const auto bounds = text.getLocalBounds();
        textBounds.setPosition(bounds.getPosition());
        textBounds.setSize(bounds.getSize());
        textBounds.setFillColor(sf::Color::Transparent);
        textBounds.setOutlineColor(sf::Color::Red);
        textBounds.setOutlineThickness(1.f);
    }

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

        window.clear();
        for (const auto& rect : backgroundRects)
            window.draw(rect, text.getTransform());
        window.draw(text);
        for (const auto& bounds : letterBounds)
            window.draw(bounds, text.getTransform());
        window.draw(textBounds, sf::RenderStates{sf::BlendAdd, text.getTransform(), nullptr, nullptr});
        window.display();
    }

    return 0;
}

Capture d’écran

So after looking at this image, I ask myself if people really want a feature like this. For me, I don't need this. If I'm the only one advocating for something like this, then we shouldn't add more features than necessary. I was definitely using / expecting the wrong things from the "findCharacterPos" function. It's truly finding the glyph's global charactar position which seems like that is what it really should do. Whenever I was comparing it to the text's global bounds, it made it look weird. Thing is, that wasn't the point of the function. I definitely think there could be some more touching up in the documentation as to what this function does specifically.

@eXpl0it3r
Copy link
Member

eXpl0it3r commented May 12, 2024

Looking at the refactoring from local coordinates to global coordinates, it seems suspicious that Y is never updated for "normal" characters, but only special characters.

While the calculation is still for local bounds first and then transformed to global coordinates afterwards, I wonder whether we shouldn't still consider the vertical spacing for the local bounds for normal characters.

5bae08a#diff-22c1fd53bbab429b4a348662278b21abb1d7a063aa14b175a8f3a5451c783b6cR152-R193

Then again, the function is about the visual representation and not the glyph bounding box, so @kimci86 might be right 🤔

@kimci86
Copy link
Contributor

kimci86 commented May 12, 2024

This seems related to #2172, i.e. lacking documentation on font metrics.

@dogunbound
Copy link
Contributor Author

This pr addresses that the findcharactarpos will return the top of the line position.

@eXpl0it3r eXpl0it3r changed the title findCharacterPos doesn't report the correct Y location sf::Text::findCharacterPos() doesn't report the correct Y location May 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants