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

Update freetype imports #216

Merged
merged 4 commits into from
Sep 10, 2015
Merged

Update freetype imports #216

merged 4 commits into from
Sep 10, 2015

Conversation

jle
Copy link
Contributor

@jle jle commented Aug 27, 2015

Looks like code.google.com/p/freetype-go redirects to github.com/golang/freetype now. This fixes go get for this project, but some dependent projects like draw2d may still refer to the old path.

@eaburns
Copy link
Member

eaburns commented Aug 27, 2015

If the example plots work, LGTM. I don't think we do anything that would conflict if draw2d still uses the old, so I suspect it's fine.

@kortschak
Copy link
Member

Please send a PR to fix the imports in draw2d or or file an issue there. I cannot see the reason for the failures (travis and my laptop disagree about some things), but I also cannot build to test here (go.googlesource.com returns a 500) which may be informative.

Can we hold off until both of these things are resolved?

@jle
Copy link
Contributor Author

jle commented Aug 27, 2015

Definitely can hold off. Here's the PR in draw2d, where they are also holding off:

llgcode/draw2d#80

llgcode/draw2d#82 (comment)

@kortschak
Copy link
Member

OK, so that's very good reason to wait. Disappointing that it was redirected before the alternative was ready. @nigeltao, is this reversible?

@nigeltao
Copy link

Sorry, the redirection happened because it had to happen eventually, and code.google.com went read-only on August 24 (http://google-opensource.blogspot.com.au/2015/03/farewell-to-google-code.html) so I couldn't wait any longer.

I didn't announce it yet on the mailing lists because I still have other API changes to make, but I guess I should, if people are having "go get" problems.

If you can give me specific build failures, I can suggest what code changes to make, but there'll have to be code changes.

@kortschak
Copy link
Member

Thanks @nigeltao, the build failure on travis at the moment appears to be due to downstream failures (due to the redirect). After fixing those (locally) and making the following changes we are now in a state where everyone has to make a move together:

diff --git a/vg/font.go b/vg/font.go
index 6572c9c..689fa38 100644
--- a/vg/font.go
+++ b/vg/font.go
@@ -18,6 +18,8 @@ import (
        "path/filepath"
        "sync"

+       "golang.org/x/image/math/fixed"
+
        "github.com/golang/freetype"
        "github.com/golang/freetype/truetype"
 )
@@ -147,7 +149,7 @@ type FontExtents struct {

 // Extents returns the FontExtents for a font.
 func (f *Font) Extents() FontExtents {
-       bounds := f.font.Bounds(f.Font().FUnitsPerEm())
+       bounds := f.font.Bounds(fixed.Int26_6(f.Font().FUnitsPerEm()))
        scale := f.Size / Points(float64(f.Font().FUnitsPerEm()))
        return FontExtents{
                Ascent:  Points(float64(bounds.YMax)) * scale,
@@ -166,9 +168,9 @@ func (f *Font) Width(s string) Length {
        for _, rune := range s {
                index := f.font.Index(rune)
                if hasPrev {
-                       width += int(f.font.Kerning(f.font.FUnitsPerEm(), prev, index))
+                       width += int(f.font.Kern(fixed.Int26_6(f.font.FUnitsPerEm()), prev, index))
                }
-               width += int(f.font.HMetric(f.font.FUnitsPerEm(), index).AdvanceWidth)
+               width += int(f.font.HMetric(fixed.Int26_6(f.font.FUnitsPerEm()), index).AdvanceWidth)
                prev, hasPrev = index, true
        }
        return Points(float64(width)) * scale
# github.com/gonum/plot/vg/vgimg
vg/vgimg/vgimg.go:264: cannot use font.Font() (type *"github.com/golang/freetype/truetype".Font) as type *"code.google.com/p/freetype-go/freetype/truetype".Font in argument to draw2d.RegisterFont

Fetching the changes at #216 to see if that fixes says that the rabbit hole is deep.

@nigeltao
Copy link

golang-nuts post about the move to github.com/golang/freetype:
https://groups.google.com/d/topic/golang-nuts/tr-MftD7kbo/discussion

@sbinet
Copy link
Member

sbinet commented Sep 8, 2015

it seems draw2d has migrated to the new freetype location+API.

the ball is in our camp, now:

# github.com/gonum/plot/vg
vg/font.go:150: cannot use f.Font().FUnitsPerEm() (type int32) as type fixed.Int26_6 in argument to f.font.Bounds
vg/font.go:153: bounds.YMax undefined (type fixed.Rectangle26_6 has no field or method YMax)
vg/font.go:154: bounds.YMin undefined (type fixed.Rectangle26_6 has no field or method YMin)
vg/font.go:155: bounds.YMax undefined (type fixed.Rectangle26_6 has no field or method YMax)
vg/font.go:155: bounds.YMin undefined (type fixed.Rectangle26_6 has no field or method YMin)
vg/font.go:169: f.font.Kerning undefined (type *truetype.Font has no field or method Kerning)
vg/font.go:171: cannot use f.font.FUnitsPerEm() (type int32) as type fixed.Int26_6 in argument to f.font.HMetric

some of these might be fixed after applying Dan's patch from #216 (comment)

- Use math/fixed

- Use new functions

  * Kerning => Kern
  * A => Alpha
@kortschak
Copy link
Member

Fixes #218.

@kortschak
Copy link
Member

Please run go test -regen github.com/gonum/plot/vg and commit the changes.

@jle
Copy link
Contributor Author

jle commented Sep 9, 2015

Note: I re-generated testdata/issue179.jpg from the test but the x11 test doesn't seem too happy about it.

@kortschak
Copy link
Member

How did you do the regen? That test should also have a regen flag - but we can deal with that later.

All the rest of the tests pass on my machine, but travis is obviously really sick today.

@kortschak
Copy link
Member

I've just had a look at what vgx11 is making, and it's clearly wrong.

issue179

@sbinet
Copy link
Member

sbinet commented Sep 9, 2015

I suppose xgbutil would need to migrate to x/image/math/fixed too:
https://github.com/BurntSushi/xgbutil/blob/master/xgraphics/text.go

@kortschak
Copy link
Member

Do you want to file an issue there?

@sbinet
Copy link
Member

sbinet commented Sep 9, 2015

done: BurntSushi/xgbutil#30

@kortschak
Copy link
Member

Maybe we should just merge this now. vgx11 is not a high enough priority to keep the rest waiting - and the natives are getting restless.

@eaburns @sbinet

@sbinet
Copy link
Member

sbinet commented Sep 10, 2015

fine by me.

@eaburns
Copy link
Member

eaburns commented Sep 10, 2015

Fine by me.

kortschak added a commit that referenced this pull request Sep 10, 2015
@kortschak kortschak merged commit 54bb08b into gonum:master Sep 10, 2015
@kortschak
Copy link
Member

@jle would you please send a PR adding yourself to A+C at https://github.com/gonum/license

Thank you.

@jle
Copy link
Contributor Author

jle commented Sep 30, 2015

@kortschak Thanks for the merge. Sorry I wasn't able to get around to fixing x11 tests. I'd love to be part of A+C but I am highly skeptical that my minor patch warrants such an act. Thanks again.

@kortschak
Copy link
Member

Smallest patch counts.

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

Successfully merging this pull request may close these issues.

5 participants