text.NewAtlas crashing application

[bfreis]

Hi,

This line crashes my application:

	atlas := text.NewAtlas(inconsolata.Regular8x16, []rune{'A'})

If I use basicfont.Face7x13 instead of Inconsolata, it works just fine. The crash looks like this:

goroutine 19 [running]:
image/draw.clip(0x4194500, 0xc42014e000, 0xc420042b38, 0x0, 0x0, 0xc420042b68, 0x0, 0x0, 0xc420042b88)
	/usr/local/Cellar/go/1.10.3/libexec/src/image/draw/draw.go:76 +0xde
image/draw.DrawMask(0x4194500, 0xc42014e000, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, ...)
	/usr/local/Cellar/go/1.10.3/libexec/src/image/draw/draw.go:107 +0xab
image/draw.Draw(0x4194500, 0xc42014e000, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, ...)
	/usr/local/Cellar/go/1.10.3/libexec/src/image/draw/draw.go:101 +0xc4
github.com/faiface/pixel/text.NewAtlas(0x4194800, 0x423ece0, 0xc420042f30, 0x1, 0x1, 0x0)
	/Users/bfreis/go/src/mod/github.com/faiface/[email protected]/text/atlas.go:64 +0x507

I'm using the most recent commit, 2ffc7a6

Anybody seen this?

Thanks!

[thegtproject]

If I can get the full source code you're trying to run I'll be more than happy to help you with this one.

[bfreis]

There's really no mystery in reproducing the bug. It's really just calling that line:

package main

import (
	"github.com/faiface/pixel/pixelgl"
	"github.com/faiface/pixel/text"
	"golang.org/x/image/font/inconsolata"
)

func main() {
	pixelgl.Run(run)
}

func run() {
	text.NewAtlas(inconsolata.Regular8x16, text.ASCII)
}

[faiface]

Hi, sorry for getting back to you so late. I've tested it on my machine and it crashes as well, which is weird. Currently, I don't have the time to investigate it much further, but if you, or anyone else could, it would be great if you guys figured this out.

[bfreis]

Hi, I didn't have time to look into this earlier, but I think I just found the reason for the crash.

https://github.com/faiface/pixel/blob/master/text/atlas.go#L63

On the atlas creation, you can see the following code:

	for r, fg := range fixedMapping {
		dr, mask, maskp, _, _ := face.Glyph(fg.dot, r)
		draw.Draw(atlasImg, dr, mask, maskp, draw.Src)
	}

The crash happens on the call to draw.Draw(...). The reason is that, when creating the atlas for the Inconsolata font, the mask parameter is nil during that iteration. And it is nil because apparently Inconsolata doesn't have a face.Glyph for the unicode.ReplacementChar rune that text.NewAtlas adds to the list of runes I pass.

Now, face.Glyph returns an ok bool as the last parameter:

// Glyph returns the draw.DrawMask parameters (dr, mask, maskp) to draw r's
// glyph at the sub-pixel destination location dot, and that glyph's
// advance width.
//
// It returns !ok if the face does not contain a glyph for r.
//
// The contents of the mask image returned by one Glyph call may change
// after the next Glyph call. Callers that want to cache the mask must make
// a copy.
Glyph(dot fixed.Point26_6, r rune) (
dr image.Rectangle, mask image.Image, maskp image.Point, advance fixed.Int26_6, ok bool)

So, what seems to be happening is that Pixel is just swallowing this ok parameter and trying to Draw the glyph even when it's not supposed to work!

The fix might actually be as simple as:

	for r, fg := range fixedMapping {
		if dr, mask, maskp, _, ok := face.Glyph(fg.dot, r); ok {
		  draw.Draw(atlasImg, dr, mask, maskp, draw.Src)
		}
	}

As I'm not very familiar with the rest of Pixel, I had no clue if this might have unintended issue with other parts of the system. But it seems logical to me that the check shouldn't hurt.

Any thoughts, @faiface ?

Cheers
Bruno

[bfreis]

Hey @faiface , any updates on this? Still happening with current HEAD. Thanks!

[faiface]

Oh, sorry, somehow forgot about this. The fix you suggested might be okay, have you tried it? Doesn't it cause incorrect rendering? If not, would you be so kind as to make a PR? :) (While you're at it, you could also remove this line: https://github.com/faiface/pixel/blob/master/text/atlas.go#L206 probably a forgotten debug print).

[delp]

@bfreis I'll try to take a look at this when I have time.

[DestyNova]

I just ran into this too and was about to give up until I discovered this issue which at least provides a workaround. I'll try the fix suggested by @bfreis and see if it helps.

[DestyNova]

Tested @bfreis's fix and it seems to work fine. Can we merge his commit?

[delp]

Good work you guys. Thanks. Getting this in.