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

Scaling affects layout #450

Closed
Rinzwind opened this issue Feb 22, 2024 · 11 comments
Closed

Scaling affects layout #450

Rinzwind opened this issue Feb 22, 2024 · 11 comments

Comments

@Rinzwind
Copy link
Contributor

In Roassal pull request #56 I made RSAthensMorph apply the world renderer canvas scale factor. I now tried to do the same for BlMorphicHost: commit 3db44c8.

That however has an effect that I didn’t expect in the following example (based on the one opened by doing BlMorphicHost example):

1 to: 2 do: [ :scale |
	| morph host space |
	OSWorldRenderer canvasScaleFactor: scale.
	(morph := Morph new)
		extent: 400 asPoint;
		layoutPolicy: TableLayout new;
		openInWindowLabeled: ('Example (scale: {1})' format: { scale }).
	(host := BlMorphicHost new)
		containerMorph: morph.
	(space := BlSpace new)
		host: host;
		show.
	space root layout: BlFlowLayout horizontal.
	1 to: 50 do: [ :n | 
		space root addChild:
			(BlTextElement new
				text: ('Bloc' asRopedText fontSize: 40; yourself);
				padding: (BlInsets all: 5);
				background: (Color h: n * 7 s: 0.5 v: 1.0);
				yourself) ].
	World doOneCycle ]

The layout is different in the two windows:

Is this a bug in Bloc or am I just making a mistake?

@Rinzwind
Copy link
Contributor Author

It looks like the elements don’t have the same width:

@Rinzwind
Copy link
Contributor Author

The example can be changed to add just one BlTextElement like this:

	space root addChild:
		(BlTextElement new
			text: ((' ' join: (Array new: 5 withAll: 'Bloc')) asRopedText
				fontSize: 40;
				yourself);
			yourself).

Then the text in the two windows does not have the same width:

That is unexpected to me, see Pharo pull request #14503 for comparison.

@tinchodias
Copy link
Collaborator

Hello @Rinzwind I reproduced your commit changes in my image and see same weird results. I will dig more and come back

@Rinzwind
Copy link
Contributor Author

Rinzwind commented Mar 2, 2024

OK! There are some other changes you may need to apply, the pull request hasn’t been merged yet: Pharo pull request #16225. It fixes this error when a ScalingCanvas scales a BlExternalForm:

@tinchodias
Copy link
Collaborator

tinchodias commented Mar 2, 2024

Thanks, I applied your change. I am not sure why I didn't need it, it may be because I tested it with scale factor = 1. But now I applied your change in OSSDL2ExternalForm>>magnifyBy:smoothing:

@tinchodias
Copy link
Collaborator

Aha! I've checked what happens at the low-level of just plain Cairo+Freetype, and it is related to text hinting, in font options.

The difference vanishes when I changed AeCanvas>>#setHighQualityOptions from hintMetrics: AeCairoHintMetrics on to be off.

(BTW, the criteria to set either "low" or "high" quality from Bloc in the AeCanvas can be revisited.)

@tinchodias
Copy link
Collaborator

Hinting can be enabled for rendering (AeCairoFontOptions>>hintStyle:) and disabled for calculating metrics (AeCairoFontOptions>>hintMetrics:), i.e. the set of a text extents.

Freetype library document about hinting: https://freetype.org/freetype2/docs/hinting/subpixel-hinting.html

FYI, inspect:

fontSize := 45.
string := 'Bloc'.
surfaceSize := 100 @ 50.
fontOptions :=
	AeCairoFontOptions new
		hintMetrics: AeCairoHintMetrics off;
		hintStyle: AeCairoHintStyle full;
		yourself.

#(0.5 0.75 1.0 2.0 3.0 10.0) collect: [ :scale |
	| aSurface aContext aFTLibrary aFTFace aScaledFont |
	aSurface := AeCairoImageSurface
		  extent: surfaceSize * scale
		  format: AeCairoSurfaceFormat argb32.
	aSurface deviceScaleX: scale y: scale.
	aContext := aSurface newContext.

	"Set up scaled font in the context"
	aFTLibrary := AeFTLibrary newInitialized.
	aFTFace := AeSourceSansPro_Bold firstFaceUsing: aFTLibrary.
	aContext
		fontFace: aFTFace newCairoFontFace;
		fontSize: fontSize;
		fontOptions: fontOptions.

	aScaledFont := aContext scaledFont.
	glyphs := aScaledFont glyphArrayForString: string.

	"Draw text"
	aContext
		translateByX: 5 y: fontSize;
		sourceColor: Color black;
		showGlyphs: glyphs.

	{scale. aScaledFont fontExtents. aScaledFont glyphExtentsOf: glyphs. aSurface } ].

With AeCairoHintMetrics off, cairo calculates that font height is 56.565 for all scales. When it is ON, the font height is between 56 and 57.

@Rinzwind
Copy link
Contributor Author

Rinzwind commented Mar 3, 2024

Ah OK, I see there’s a comment about that on AeCairoHintMetrics as well: “Specifies whether to hint font metrics; hinting font metrics means quantizing them so that they are integer values in device space. Doing this improves the consistency of letter and line spacing, however it also means that text will be laid out differently at different zoom factors.”

So would it be better to change #initializeForSurface:and: on BASpaceRenderer to always perform the #setLowQualityOptions as that uses ‘off’ for the font metric hinting?

@tinchodias
Copy link
Collaborator

I think we can set both low and high quality use AeCairoHintMetrics off, to fix this issue. AFAIU the worst case is a < 1 pixel mismatch between a BlTextElement extent and the actual pixels rendered. For example, that 'Bloc' is calculated as 30 pixels width while the rendering is actually 31. May this mismatch worse than "scaling affects layout"?

In another issue we can discuss if we either remove or improve the heuristic in BASpaceRenderer:

	"On High DPI displays, high quality is not visible enough, and often 
	much larger surface areas are painted."
	aBlHostRendererSurface scaleFactor > 1.0 ifTrue: [ aeCanvas setLowQualityOptions ]

Rinzwind added a commit to Rinzwind/Alexandrie that referenced this issue Mar 6, 2024
…ting, as is done in #setLowQualityOptions, to avoid text being laid out differently at different scale factors (see: pharo-graphics/Bloc#450).
@Rinzwind
Copy link
Contributor Author

Rinzwind commented Mar 6, 2024

OK, I have opened Alexandrie pull request #51 to change #setHighQualityOptions to not use font metric hinting, and Bloc pull request #465 to merge commit 3db44c8.

@Rinzwind
Copy link
Contributor Author

I think this can be closed. The two pull requests have been merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants