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

(0.5 feedback) Text::dimensions height behavior changed #569

Open
ozkriff opened this Issue Jan 29, 2019 · 7 comments

Comments

Projects
None yet
2 participants
@ozkriff
Copy link
Contributor

ozkriff commented Jan 29, 2019

Was that intentional? What is the reasonong? It breaks Zemeroth's UI code, because I'm scaling labels and buttons to 1/12th of the screen's height.

GGEZ v0.4 vs v0.5:

image image

If it was intentional, is there another method to get "old-fashined" bounding box of the text?

@ozkriff

This comment has been minimized.

Copy link
Contributor Author

ozkriff commented Jan 29, 2019

"..." and "abc" and strings return same height, so the difference comes to the handling of the descender and ascender parts of the glyps (that are under baseline or above the mean line).

image

@icefoxen

This comment has been minimized.

Copy link
Contributor

icefoxen commented Feb 1, 2019

My first guess is that it is due to winit's hidpi functionality lying about how big the display is. If the result of graphics::os_hidpi_factor() is >1.0 then that is probably it. See the latter half of #391 for details.

In general this isn't intentional, if we need to fix the text bounding box code to be accurate then we should do that.

@ozkriff

This comment has been minimized.

Copy link
Contributor Author

ozkriff commented Feb 2, 2019

No, that's definitely not a HDPI problem. Here's a screenshot with both versions overlapping.

image

Strings with descender parts in them ("strength:", etc) are of the same size and strings like "moves:" clearly has less pixels in them (and thus they looks bigger when scaled to the same screen height).

In general this isn't intentional, if we need to fix the text bounding box code to be accurate then we should do that.

Ok, I'll try to look into the code.

@icefoxen

This comment has been minimized.

Copy link
Contributor

icefoxen commented Feb 3, 2019

Wow. Yeah that looks awful. Good call on noticing the descender and ascender parts are what make it different. I expect that we're just using the wrong font metrics for sizing somewhere. I will work on this as well, if we can't find it in a few days we might just want to ask for help from alexheretic.

@icefoxen icefoxen added this to the 0.5 milestone Feb 5, 2019

@ozkriff

This comment has been minimized.

Copy link
Contributor Author

ozkriff commented Feb 13, 2019

So, I've experimented a little bit with this and the solution to bring the old behavior (same text's height regardless of what characters are in the string) for one-liners is:

     fn calculate_dimensions(&self, context: &mut Context) -> (u32, u32) {
         let mut max_width = 0;
         let mut max_height = 0;
         {
             let varied_section = self.generate_varied_section(Point2::new(0.0, 0.0), None);
+            for section in &varied_section.text {
+                let h = section.scale.y.ceil() as _;
+                if h > max_height {
+                    max_height = h;
+                }
+            }
             use glyph_brush::GlyphCruncher;
             let glyphs = context.gfx_context.glyph_brush.glyphs(varied_section);

This works for Zemeroth's ZGui at least.

But it doesn't address multiline Text, like "test\ntext" and "test\njjj" still have different height. And I don't see any simple way to handle this atm.

@ozkriff

This comment has been minimized.

Copy link
Contributor Author

ozkriff commented Feb 14, 2019

Here's an updated patch (thanks, @not-fl3!) that seems to work for multiline Text:

diff --git a/src/graphics/text.rs b/src/graphics/text.rs
index bd66dc2..6763343 100644
--- a/src/graphics/text.rs
+++ b/src/graphics/text.rs
@@ -320,16 +320,16 @@ impl Text {
             let glyphs = context.gfx_context.glyph_brush.glyphs(varied_section);
             for positioned_glyph in glyphs {
                 if let Some(rect) = positioned_glyph.pixel_bounding_box() {
-                    if rect.max.x > max_width {
-                        max_width = rect.max.x;
-                    }
-                    if rect.max.y > max_height {
-                        max_height = rect.max.y;
-                    }
+                    let font = positioned_glyph.font().expect("Glyph doesn't have a font");
+                    let v_metrics = font.v_metrics(positioned_glyph.scale());
+                    let max_y = positioned_glyph.position().y + positioned_glyph.scale().y - v_metrics.ascent;
+                    let max_y = max_y.ceil() as u32;
+                    max_width = std::cmp::max(max_width, rect.max.x as u32);
+                    max_height = std::cmp::max(max_height, max_y);
                 }
             }
         }
-        let (width, height) = (max_width as u32, max_height as u32);
+        let (width, height) = (max_width, max_height);
         if let Ok(mut metrics) = self.cached_metrics.try_borrow_mut() {
             metrics.width = Some(width);
             metrics.height = Some(height);

What do you think? I can make a PR if this change seems fine to you.

@icefoxen

This comment has been minimized.

Copy link
Contributor

icefoxen commented Feb 24, 2019

Can't reproduce? What am I doing wrong?

image

The code is just:

        let t1 = Text::new(TextFragment::new("test\ntext"));
        let t2 = Text::new(TextFragment::new("test\njjj"));
        let t3 = Text::new(TextFragment::new("asdfjpgklti"));
        graphics::queue_text(
            ctx,
            &t1,
            Point2::new(0.0, 0.0),
            None,
        );
        graphics::draw_queued_text(
            ctx,
            DrawParam::new()
                .dest(Point2::new(400.0, 400.0))
        )?;
        graphics::queue_text(
            ctx,
            &t2,
            Point2::new(30.0, 0.0),
            None,
        );
        graphics::draw_queued_text(
            ctx,
            DrawParam::new()
                .dest(Point2::new(400.0, 400.0))
        )?;
        
        graphics::queue_text(
            ctx,
            &t3,
            Point2::new(60.0, 0.0),
            None,
        );
        graphics::draw_queued_text(
            ctx,
            DrawParam::new()
                .dest(Point2::new(400.0, 400.0))
        )?;
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.