Re: Dmitry Timoshkov : gdiplus: GdipMeasureString operates internally in pixels but in/ out rectangles are in device units.
gdiplus: GdipMeasureString operates internally in pixels but in/out rectangles are in device units.
I still don't understand this patch.
Your subject implies that device units and pixels are not the same thing, but my understanding is that except for metafile HDCs (which we don't handle correctly anyway, and I suspect native doesn't either), they are.
You're scaling sizes based on graphics->unit. Yet the conversion in get_graphics_transform (which we were already using in GdipMeasureString to convert between world and device units, indirectly through GdipTransformPoints) already takes graphics->unit into account.
So I'm concerned that the original conversion apparently wasn't sufficient, and that you've added more code instead of fixing it.
And I'm still concerned about whether the sizes returned by GdipMeasureString will match the size of the strings rendered by GdipDrawString with this patch applied.
Vincent Povirk madewokherd@gmail.com wrote:
I still don't understand this patch.
Your subject implies that device units and pixels are not the same thing, but my understanding is that except for metafile HDCs (which we don't handle correctly anyway, and I suspect native doesn't either), they are.
Device units in gdiplus are the units specified in the call to GdipSetPageUnit. Since gdiplus doesn't change DC mapping mode to anything other than MM_TEXT, then the units accepted and returned by gdi32 are pixels. According to the tests GdipMeasureString receives and returns bounding and clipping rectangles in device units, so it needs to convert them before and after interacting with gdi32.
You're scaling sizes based on graphics->unit. Yet the conversion in get_graphics_transform (which we were already using in GdipMeasureString to convert between world and device units, indirectly through GdipTransformPoints) already takes graphics->unit into account.
Transforms performed by get_graphics_transform() and GdipTransformPoints have nothing to do with my patch.
OK, this is starting to make sense to me. If I'm understanding this, the point of your series is that the font has a consistent size based on the unit stored in the font. If it's a physical unit like inches then the font's size in world coordinates changes based on the graphics page unit, so that it maintains the same physical size. Since GdipMeasureString reports world coordinates you see the measurement change while the pixel size should stay the same based on the device dpi.
This would mean that get_font_hfont needs to account for font->unit, so that it can calculate the right pixel size based on the device dpi. It currently doesn't. In fact, it appears we're trying to do this calculation in GdipCreateFont, when we don't have enough information for it. This probably only works when the Graphics object has the same dpi as the display.
Since get_font_hfont is applying the page transformation to font sizes, we are still scaling our fonts based on the graphics page unit, when your tests show that in at least some cases we shouldn't be. I think what you've actually done is cancel out the convert_unit() factor that get_font_hfont had applied, by changing the measurement, and I don't think that's right. The pixel size should always relate to the world size based on the transform used by GdipTransformPoints. It's the pixel size that needs to change, not the relationship between pixel size and result.
This also raises some questions about the world transform and page scale. If I have a font with a size of 1.0 inches, and I render it on a graphics with no world transform or page scale, I guess it uses a pixel size equal to the dpi of the graphics object. If my graphics object has a scaling world transform or a page scale of 2.0, does the font render at twice the normal size in pixels, or is its size unaffected by those things? Are these cases where we'd see a difference between UnitWorld, UnitPixel, and UnitDisplay?
Vincent Povirk madewokherd@gmail.com wrote:
OK, this is starting to make sense to me. If I'm understanding this, the point of your series is that the font has a consistent size based on the unit stored in the font. If it's a physical unit like inches then the font's size in world coordinates changes based on the graphics page unit, so that it maintains the same physical size. Since GdipMeasureString reports world coordinates you see the measurement change while the pixel size should stay the same based on the device dpi.
Points, inches, meters and the like are logical units, pixel is the physical unit measure. Logical units always map to the same distance on the physical device, which uses pixels internally eventually. Using this terminology helps to avoid confusion and correlates with gdi32 APIs LPtoDP/DPtoLP.
Since get_font_hfont is applying the page transformation to font sizes, we are still scaling our fonts based on the graphics page unit, when your tests show that in at least some cases we shouldn't be. I think what you've actually done is cancel out the convert_unit() factor that get_font_hfont had applied, by changing the measurement, and I don't think that's right. The pixel size should always relate to the world size based on the transform used by GdipTransformPoints. It's the pixel size that needs to change, not the relationship between pixel size and result.
convert_unit() is a very poorly named helper, and its meening and behaviour has changed in time it seems. convert_unit() needs to be removed completely, and conversion between pixels and device/font units should be generalized.
For instance setting device unit to millimeters with GdipSetPageUnit() and then calling GdipDrawImageI() leads to completely wrong results because down in the path there is huge confusion between device units, source coordinates units and device/world transforms applied to both of them using only device X resolution and completely ignoring image resolution. What a mess.
OK, this is starting to make sense to me. If I'm understanding this, the point of your series is that the font has a consistent size based on the unit stored in the font. If it's a physical unit like inches then the font's size in world coordinates changes based on the graphics page unit, so that it maintains the same physical size. Since GdipMeasureString reports world coordinates you see the measurement change while the pixel size should stay the same based on the device dpi.
Points, inches, meters and the like are logical units, pixel is the physical unit measure.
Oh. Then what I mean to say is that if you specify the size in logical units, the size in pixels depends on the dpi of the graphics object. The size in world coordinates depends on the size in pixels, the page unit, page scale, and the world transform.
At least this is the way your tests lead me to believe native behaves. Builtin does not behave that way, so I think you have partially fixed some cases and broken others.
convert_unit() is a very poorly named helper, and its meening and behaviour has changed in time it seems. convert_unit() needs to be removed completely, and conversion between pixels and device/font units should be generalized.
Well, we should mostly be using GdipTransformPoints instead (so that we can account for world transform, page scale, and page unit in a general way) and avoiding convert_unit directly. I see we're essentially duplicating that logic in prepare_dc and transform_and_round_points.
In cases where we need to convert a logical measurement to a device measurement (maybe including in GdipTransformPoints) I think units_to_pixels/pixels_to_units is appropriate, but it should really take a Graphics object rather than a dpi (since the correct dpi cannot be known without a Graphics object). And we need testing to decide whether to account for the page scale in that calculation.
For instance setting device unit to millimeters with GdipSetPageUnit() and then calling GdipDrawImageI() leads to completely wrong results because down in the path there is huge confusion between device units, source coordinates units and device/world transforms applied to both of them using only device X resolution and completely ignoring image resolution. What a mess.
Yes, I'm aware that the DrawImage functions that don't take a destination size should be using the image resolution. I've been putting that off. But I think this only matters for images, fonts, and perhaps texture brushes. All other cases should use world coordinates.
Vincent Povirk madewokherd@gmail.com wrote:
Oh. Then what I mean to say is that if you specify the size in logical units, the size in pixels depends on the dpi of the graphics object. The size in world coordinates depends on the size in pixels, the page unit, page scale, and the world transform.
At least this is the way your tests lead me to believe native behaves. Builtin does not behave that way, so I think you have partially fixed some cases and broken others.
That's unavoidable in the mess in which currently gdiplus handles coordibates.
convert_unit() is a very poorly named helper, and its meening and behaviour has changed in time it seems. convert_unit() needs to be removed completely, and conversion between pixels and device/font units should be generalized.
Well, we should mostly be using GdipTransformPoints instead (so that we can account for world transform, page scale, and page unit in a general way) and avoiding convert_unit directly. I see we're essentially duplicating that logic in prepare_dc and transform_and_round_points.
Yes, consistently using GdipTransformPoints would help at least partially solve current mess.
In cases where we need to convert a logical measurement to a device measurement (maybe including in GdipTransformPoints) I think units_to_pixels/pixels_to_units is appropriate, but it should really take a Graphics object rather than a dpi (since the correct dpi cannot be known without a Graphics object). And we need testing to decide whether to account for the page scale in that calculation.
The helpers are useful for a font object too, that's why they don't use graphics object as input.
For instance setting device unit to millimeters with GdipSetPageUnit() and then calling GdipDrawImageI() leads to completely wrong results because down in the path there is huge confusion between device units, source coordinates units and device/world transforms applied to both of them using only device X resolution and completely ignoring image resolution. What a mess.
Yes, I'm aware that the DrawImage functions that don't take a destination size should be using the image resolution. I've been putting that off. But I think this only matters for images, fonts, and perhaps texture brushes. All other cases should use world coordinates.
Image resolution and using only X device resolution is only a tiny part of the mess described above.
In cases where we need to convert a logical measurement to a device measurement (maybe including in GdipTransformPoints) I think units_to_pixels/pixels_to_units is appropriate, but it should really take a Graphics object rather than a dpi (since the correct dpi cannot be known without a Graphics object). And we need testing to decide whether to account for the page scale in that calculation.
The helpers are useful for a font object too, that's why they don't use graphics object as input.
Maybe for GdipGetFontHeightGivenDPI it's appropriate, but the other uses in font.c are wrong and should be using the DPI from a Graphics object.
-
Dmitry Timoshkov -
Vincent Povirk