x/image/font/opentype: Face isn't compliant with font.Face

ShadiestGoat commented 1 year ago

What version of Go are you using (`go version`)?

$ go version
go version go1.19.5 linux/amd64

Does this issue reproduce with the latest release?

Yes.

What operating system and processor architecture are you using (`go env`)?

go env Output

$ go env
GO111MODULE=""
GOARCH="amd64"
GOBIN=""
GOCACHE="/home/shady/.cache/go-build"
GOENV="/home/shady/.config/go/env"
GOEXE=""
GOEXPERIMENT=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOINSECURE=""
GOMODCACHE="/home/shady/go/pkg/mod"
GONOPROXY=""
GONOSUMDB=""
GOOS="linux"
GOPATH="/home/shady/go"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/usr/lib/go"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/usr/lib/go/pkg/tool/linux_amd64"
GOVCS=""
GOVERSION="go1.19.5"
GCCGO="gccgo"
GOAMD64="v1"
AR="ar"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD="/dev/null"
GOWORK=""
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -Wl,--no-gc-sections -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build457832407=/tmp/go-build -gno-record-gcc-switches"

What did you do?

I created a new face from opentype. Then, I called Glyph() on a non existing character.

What did you expect to see?

I expected it to return !ok, as stated in the font.Face documentation.

What did you see instead?

It returns ok.

mknyszek commented 1 year ago

I don't see an owner listed on https://dev.golang.org/owners, but most of the code seems to have been written by @nigeltao.

ShadiestGoat commented 1 year ago

Hi! I proposed a small change here that should fix this issue.

nigeltao commented 1 year ago

I'm tempted to close this as Working As Intended.

Glyph index 0 is a valid index, and by convention it's the .notdef glyph used for missing characters. See:

Calling opentype.Face.Glyph with a rune that isn't covered by the font face should therefore arguably return ok == true and this .notdef glyph (typically an empty rectangle). The Face does contain a glyph for the rune argument. It just happens to be a fallback glyph.

There is a separate question of "if I have a string, what runes in it are covered by this Face (excluding .notdef glyphs)", but that is a much more complicated question, related to Harfbuzz-like layout and shaping and fontconfig-like font selection, and surely deserves separate API.

For example, Han Unification means that "does this Face have a non-zero glyph index for this rune" isn't sufficient to answer "is this the right font for this rune". At a minimum, this should probably involve some concept of the user's locale, but I don't think Go has a good cross-platform abstraction for this yet.

For example, Android's minikin library's author said that:

Much of the internal structure of Minikin is dedicated to itemization, which is done largely on the basis of the cmap coverage of the fonts in the fallback chain. Doing that query font-by-font for every character would be expensive, so there are fancy bitmap acceleration structures.

Again, font selection and coverage calculation is a bigger problem needing separate API.

nigeltao commented 1 year ago

Consider the opentype example_test.go file, changing the "e" in the "jelly" string to a Unicode U+2603 Snowman, and running go test:

diff --git a/font/opentype/example_test.go b/font/opentype/example_test.go
index 4d7ee85..c8d522a 100644
--- a/font/opentype/example_test.go
+++ b/font/opentype/example_test.go
@@ -46,7 +46,7 @@ func ExampleNewFace() {
                Dot:  fixed.P(startingDotX, startingDotY),
        }
        fmt.Printf("The dot is at %v\n", d.Dot)
-       d.DrawString("jel")
+       d.DrawString("j\u2603l")
        fmt.Printf("The dot is at %v\n", d.Dot)
        d.Src = image.NewUniform(color.Gray{0x7F})
        d.DrawString("ly")

Without your patch, this draws something roughly like "j▯lly" with an empty rectangle.
With your patch, this draws "jlly" with nothing at all between the "j" and the "l".

I think (1) is the better output.

ShadiestGoat commented 1 year ago

Hello! Thank you for your response.

Glyph index 0 is a valid index, and by convention it's the .notdef glyph used for missing characters. See

I agree! Glyph index 0 is a valid index, however, it implies that the glyph you are trying to draw isn't present. The fact that the .notdef is being returned should mean that ok == false, as stated in the font.Face documentation.

It returns !ok if the face does not contain a glyph for r.

(Glyph, GlyphAdvance, GlyphBounds)

To me, this sounds like it must return an ok == false, and the .notdef glyph bounds/advance/mask/etc. After all, what is the point of the ok variable if not to determine that it is using a fallback glyph? The only other thing I see is for error handling, and to me, in an ideal world errors would mean itd draw the .notdef glyph.

The benefit of this patch is that libraries like multiface could work properly. It would make users be able to use fallback glyphs without the need for called Index() ourselves.

There is a separate question of "if I have a string, what runes in it are covered by this Face (excluding .notdef glyphs)", but that is a much more complicated question, related to Harfbuzz-like layout and shaping and fontconfig-like font selection, and surely deserves separate API.

For example, Han Unification means that "does this Face have a non-zero glyph index for this rune" isn't sufficient to answer "is this the right font for this rune". At a minimum, this should probably involve some concept of the user's locale, but I don't think Go has a good cross-platform abstraction for this yet.

There could be a separate structure and API for this, but I'm not sure to what extent that is relevant to the issue at hand.

I don't think this patch is significantly slower than before. It introduces 1-2 checks per function. The index() calls were already done before, it was just inline.

Regarding the example_test.go, you are right. That test should output something like j▯lly. However, that sounds like an issue that should be fixed, rather than an issue that should be used to cancel this pr. I will look into how to fix this as soon as I can.

ShadiestGoat commented 1 year ago

Consider the opentype example_test.go file, changing the "e" in the "jelly" string to a Unicode U+2603 Snowman, and running go test:

diff --git a/font/opentype/example_test.go b/font/opentype/example_test.go
index 4d7ee85..c8d522a 100644
--- a/font/opentype/example_test.go
+++ b/font/opentype/example_test.go
@@ -46,7 +46,7 @@ func ExampleNewFace() {
                Dot:  fixed.P(startingDotX, startingDotY),
        }
        fmt.Printf("The dot is at %v\n", d.Dot)
-       d.DrawString("jel")
+       d.DrawString("j\u2603l")
        fmt.Printf("The dot is at %v\n", d.Dot)
        d.Src = image.NewUniform(color.Gray{0x7F})
        d.DrawString("ly")

1. Without your patch, this draws something roughly like "j▯lly" with an empty rectangle.

2. With your patch, this draws "jlly" with nothing at all between the "j" and the "l".

I think (1) is the better output.

This is an issue due to image/font/font.go, DrawString, !ok has a TODO and a continue. When the check is removed, it would still draw ▯. Its better not to do the check regardless, as for errors return a 0 advance, and .notdef values still return ▯. I am however unsure of this. It sounds a bit out of scope, what do you think @nigeltao?

ShadiestGoat commented 1 year ago

Hello, may I please get an update on this, @nigeltao?

gopherbot commented 1 year ago

Change https://go.dev/cl/474376 mentions this issue: font: have Glyph return !ok for U+FFFD substitute

gopherbot commented 11 months ago

Change https://go.dev/cl/552616 mentions this issue: shiny/text: drop panic when GlyphAdvance returns !ok

golang / go