diff --git a/rviz_rendering/include/rviz_rendering/objects/movable_text.hpp b/rviz_rendering/include/rviz_rendering/objects/movable_text.hpp index 1d322ab3e..d4cd03c91 100644 --- a/rviz_rendering/include/rviz_rendering/objects/movable_text.hpp +++ b/rviz_rendering/include/rviz_rendering/objects/movable_text.hpp @@ -245,6 +245,9 @@ class MovableText : public Ogre::SimpleRenderable Ogre::Real char_height_; Ogre::Real line_spacing_; Ogre::Real space_width_; + // When false, a space takes the width of the preceding glyph; when true, the + // explicit space_width_ set via setSpaceWidth() is used instead. + bool space_width_set_; bool needs_update_; bool needs_color_update_; diff --git a/rviz_rendering/src/rviz_rendering/objects/movable_text.cpp b/rviz_rendering/src/rviz_rendering/objects/movable_text.cpp index 397a2cdce..e0ff37024 100644 --- a/rviz_rendering/src/rviz_rendering/objects/movable_text.cpp +++ b/rviz_rendering/src/rviz_rendering/objects/movable_text.cpp @@ -81,6 +81,7 @@ MovableText::MovableText( color_(color), line_spacing_(0.01f), space_width_(0), + space_width_set_(false), needs_color_update_(true), on_top_(false), global_translation_(0.0f), @@ -178,8 +179,9 @@ void MovableText::setLineSpacing(Ogre::Real height) void MovableText::setSpaceWidth(Ogre::Real width) { - if (width != space_width_) { + if (width != space_width_ || !space_width_set_) { space_width_ = width; + space_width_set_ = true; needs_update_ = true; } } @@ -231,7 +233,10 @@ struct TextBuffer explicit TextBuffer(float * buffer) : buffer_(buffer), min_(std::numeric_limits::max()), - max_(std::numeric_limits::min()), + // Use lowest() (= -FLT_MAX), not min() which is the smallest positive + // normal value. With min(), makeCeil would never lower the y/z components + // to actual vertex values like 0, leaving FLT_MIN garbage in the AABB. + max_(std::numeric_limits::lowest()), max_squared_radius_(0), top_(0), left_(0) @@ -387,15 +392,18 @@ MovableText::calculateTotalDimensionsForPositioning(float & total_height, float total_height = effective_char_height; total_width = 0.0f; float current_width = 0.0f; + float previous_glyph_width = space_width_; for (auto & character : caption_) { if (character == '\n') { total_height += effective_char_height + line_spacing_; total_width = current_width > total_width ? current_width : total_width; current_width = 0; + previous_glyph_width = space_width_; } else if (character == ' ') { - current_width += space_width_; + current_width += space_width_set_ ? space_width_ : previous_glyph_width; } else { - current_width += font_->getGlyphAspectRatio(character) * effective_char_height; + previous_glyph_width = font_->getGlyphAspectRatio(character) * effective_char_height; + current_width += previous_glyph_width; } } total_width = current_width > total_width ? current_width : total_width; @@ -439,15 +447,17 @@ void MovableText::fillVertexBuffer( buffer.left_ = starting_left; buffer.top_ = top; + float previous_glyph_width = space_width_; for (auto & character : caption_) { if (character == '\n') { buffer.left_ = starting_left; buffer.top_ -= effective_char_height + line_spacing_; + previous_glyph_width = space_width_; continue; } if (character == ' ') { - buffer.left_ += space_width_; + buffer.left_ += space_width_set_ ? space_width_ : previous_glyph_width; continue; } @@ -463,7 +473,8 @@ void MovableText::fillVertexBuffer( buffer.addBottomLeft(char_height_); buffer.addBottomRight(char_width, char_height_); - buffer.left_ += char_aspect_ratio * effective_char_height; + previous_glyph_width = char_aspect_ratio * effective_char_height; + buffer.left_ += previous_glyph_width; } position_and_texture_buffer->unlock(); diff --git a/rviz_rendering/test/rviz_rendering/objects/movable_text_test.cpp b/rviz_rendering/test/rviz_rendering/objects/movable_text_test.cpp index 9a1f40e39..4308ff65c 100644 --- a/rviz_rendering/test/rviz_rendering/objects/movable_text_test.cpp +++ b/rviz_rendering/test/rviz_rendering/objects/movable_text_test.cpp @@ -53,6 +53,10 @@ class MovableTextTestFixture : public ::testing::Test testing_environment_->setUpOgreTestEnvironment(); } + void TearDown() override + { + testing_environment_.reset(); + } std::shared_ptr testing_environment_; }; @@ -253,3 +257,36 @@ TEST_F(MovableTextTestFixture, EmptyCaptionDoesNotCrash) { movable_text->update(); ASSERT_THAT(movable_text->getBoundingBox(), Eq(Ogre::AxisAlignedBox::BOX_NULL)); } + +TEST_F(MovableTextTestFixture, bounding_box_max_components_are_not_FLT_MIN_garbage) { + auto movable_text = std::make_shared("A"); + + auto max = movable_text->getBoundingBox().getMaximum(); + // Regression: TextBuffer's max_ must be initialized to lowest() (= -FLT_MAX), + // not min() (= smallest positive normal). With the old init, makeCeil left + // FLT_MIN in y/z whenever no vertex pushed those components above ~1.18e-38. + ASSERT_EQ(max.y, 0.0f); + ASSERT_EQ(max.z, 0.0f); +} + +float calculateGlyphWidth(std::shared_ptr movable_text, char character) +{ + auto font = Ogre::FontManager::getSingleton().getByName("Liberation Sans", "rviz_rendering").get(); + font->load(); + return font->getGlyphAspectRatio(character) * movable_text->getCharacterHeight() * 2.0f; +} + +TEST_F(MovableTextTestFixture, centerAlignmentWorksCorrectlyWithSpaces) +{ + auto movable_text = std::make_shared("G G"); + movable_text->setTextAlignment( + rviz_rendering::MovableText::HorizontalAlignment::H_CENTER, + rviz_rendering::MovableText::VerticalAlignment::V_BELOW); + movable_text->update(); + float char_width = calculateGlyphWidth(movable_text, 'G'); + EXPECT_THAT( + movable_text->getBoundingBox(), + AllOf( + HasMinimum(Ogre::Vector3(-3 * char_width / 2, -2, 0)), + HasMaximum(Ogre::Vector3(3 * char_width / 2, 0, 0)))); +}