Fix findCharacterPos not using local bounds

[dogunbound]

findCharacterPos was giving me a weird offset in it's return result. This fixes the miscalculation the function was returning

Description

findcharactorPos calculation was taking into consideration local bounds before spitting out it's result. Fix is to include this. I believe this should be done in 2.6.x as it literally makes this function unusable.

This PR is related to the issue #

Tasks

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

How to test this PR?

Run this code, click the screen and see where the box around the letter lands. Also, probably should do a review on this test 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;
}

[dogunbound]

Still needs more testing. I'm a little tired and will work on testing later.

With the fix, the blue box covers the letter:

[dogunbound]

Some updated test 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};
}

void update_mouse_dot_and_letter_box(const sf::Event::MouseButtonEvent &event,
                                     const sf::Text &text,
                                     sf::CircleShape &mouse_dot,
                                     sf::RectangleShape &letter_box) {
  sf::Vector2f mouse_position(event.x, event.y);
  mouse_dot.setPosition(mouse_position);

  letter_box.setSize(text.getGlobalBounds().getSize());
  letter_box.setPosition(text.getGlobalBounds().getPosition());

  size_t number_of_newlines = 1;
  for (const auto &c : text.getString()) {
    if (c == '\r' || c == '\n') {
      number_of_newlines += 1;
    }
  }
  int line_height = text.getGlobalBounds().height / number_of_newlines;

  for (size_t idx = 0; idx < text.getString().getSize(); idx++) {
    sf::Vector2f letter_position = text.findCharacterPos(idx);
    sf::Vector2f letter_size(get_character_width_at_idx(text, idx).value_or(0), line_height);
    sf::FloatRect letter_bounds = sf::FloatRect(letter_position, letter_size);

    if (letter_bounds.contains(mouse_position)) {
      letter_box.setSize(letter_bounds.getSize());
      letter_box.setPosition(letter_bounds.getPosition());
      return;
    }
  }
}

constexpr char *TEST_STRING = R"(This
Should
work
eh?)";

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(TEST_STRING);
  text.setPosition(sf::Vector2f(10, 10));

  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: 
        update_mouse_dot_and_letter_box(event.mouseButton, text, mouse_dot, letter_box);
       break;
      default:
        break;
      }
    }
    window.clear(sf::Color::Black);
    window.draw(text);
    window.draw(mouse_dot);
    window.draw(letter_box);
    window.display();
  }

  return 0;
}

[kimci86]

This is not a correct solution. It doesn't make sense for findCharacterPos result to depend on other characters. Local bounds are computed from the entire string.
Example when the string starts with a line made of an underscore:

See also: #2988 (comment)

[ChrisThrasher]

If you retarget this PR to master then you gain the advantage of a test suite to detect and test changes to how this function behaves.

[dogunbound]

Thank you @kimci86 for catching that. I am starting to see the small intricate issues with this change. Leaving this API alone seems to be the best solution, even though it does look a little funny. The issue is that I'm relating global bounds with glyph bounds, and they are sort of 2 different things.

If you retarget this PR to master then you gain the advantage of a test suite to detect and test changes to how this function behaves.

I'm tihnking of removing this PR entirely now. Me and kimci discussed on discord that all we need is a way to fetch an axis-aligned bounding box of a character in a sf::Text. That solves many of the requirements I want here. Since that's a new feature, I'm going to target master for this change and remove it as a 2.6 change. Thanks to kmici86, I now know ways to handle this find character position a bit better.