Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

Call quadratic bezier function in QuadraticTo#114

Open
nileshpatra wants to merge 2 commits intofogleman:master from
nileshpatra:call-quadratic-bezier-function
Open

Call quadratic bezier function in QuadraticTo #114
nileshpatra wants to merge 2 commits intofogleman:master from
nileshpatra:call-quadratic-bezier-function

Conversation

@nileshpatra
Copy link

@nileshpatra nileshpatra commented Oct 20, 2020

In accordance with #113

PS: This also seems to fix failing tests on arm64

Copy link
Author

@fogleman please consider to review this. As this fails and results in a regression on arm64 which is important for me to be fixed.

Copy link
Owner

The underlying freetype/raster package natively supports quadratic bezier curves (but not cubic). The quadratic curves are rendered with those Add2 functions. What issues were you seeing with the original code?

Copy link
Author

nileshpatra commented Oct 20, 2020
edited
Loading

@fogleman the rendered image in arm64 which indirectly uses the QuadraticTo function renders an image with slight deltas. This leads to failing tests for the same.
However all other functions render perfectly identical graphics between both the architectures.

I suspect that this is somewhere due to minor rendering issues?

Please let me know.
Although I've tested it, but I'd also appreciate if you could let me know if this patch is not a "breaking" change?

Copy link
Owner

Can you post sample code and the output images?

Copy link
Author

@fogleman Sample code and output with the patch applied

package main
import "./ups/gg"
func main() {
	const S = 1024
	dc := gg.NewContext(S, S)
	dc.SetRGBA(0, 0, 0, 0.1)
	for i := 0; i < 360; i += 15 {
		dc.Push()
		dc.RotateAbout(gg.Radians(float64(i)), S/2, S/2)
		dc.DrawEllipse(S/2, S/2, S*7/16, S/8)
		dc.Fill()
		dc.Pop()
	}
	if im, err := gg.LoadImage("examples/gopher.png"); err == nil {
		dc.DrawImageAnchored(im, S/2, S/2, 0.5, 0.5)
	}
	dc.SavePNG("out.png")
}

out

Copy link
Author

Another one using QuadraticTo directly

package main
import "./ups/gg"
func main() {
	const S = 1000
	dc := gg.NewContext(S, S)
	dc.SetRGB(1, 1, 1)
	dc.Clear()
	dc.Translate(S/2, S/2)
	dc.Scale(40, 40)
	var x0, y0, x1, y1, x2, y2, x3, y3, x4, y4 float64
	x0, y0 = -10, 0
	x1, y1 = -5, -10
	x2, y2 = 0, 0
	x3, y3 = 5, 10
	x4, y4 = 10, 0
	dc.MoveTo(x0, y0)
	dc.LineTo(x1, y1)
	dc.LineTo(x2, y2)
	dc.LineTo(x3, y3)
	dc.LineTo(x4, y4)
	dc.SetHexColor("FF2D00")
	dc.SetLineWidth(8)
	dc.Stroke()
	dc.MoveTo(x0, y0)
	dc.QuadraticTo(x1, y1, x2, y2)
	dc.QuadraticTo(x3, y3, x4, y4)
	dc.SetHexColor("3E606F")
	dc.SetLineWidth(16)
	dc.FillPreserve()
	dc.SetRGB(0, 0, 0)
	dc.Stroke()
	dc.DrawCircle(x0, y0, 0.5)
	dc.DrawCircle(x1, y1, 0.5)
	dc.DrawCircle(x2, y2, 0.5)
	dc.DrawCircle(x3, y3, 0.5)
	dc.DrawCircle(x4, y4, 0.5)
	dc.SetRGB(1, 1, 1)
	dc.FillPreserve()
	dc.SetRGB(0, 0, 0)
	dc.SetLineWidth(4)
	dc.Stroke()
	dc.LoadFontFace("/Library/Fonts/Arial.ttf", 200)
	dc.DrawStringAnchored("g", -5, 5, 0.5, 0.5)
	dc.DrawStringAnchored("G", 5, -5, 0.5, 0.5)
	dc.SavePNG("out.png")
}

out

Copy link
Owner

But what does it look like without the fix?

Copy link
Author

@fogleman
Without fixes on amd64:

out
out

On arm64:
out
out1

Though they look identical but their md5sums are different

Copy link
Owner

OK, actually the tests in gg should probably be changed to permit small, indistinguishable differences since various platforms produce subtly different results. So, instead of using md5sums, comparing images and ensuring that the delta is small.

Copy link
Author

nileshpatra commented Oct 20, 2020
edited
Loading

OK, actually the tests in gg should probably be changed to permit small, indistinguishable differences since various platforms produce subtly different results. So, instead of using md5sums, comparing images and ensuring that the delta is small.

@fogleman
Thanks for clarifying. Two questions:

For the background: I actually maintain this as a package in Debian. And this fails for archs other than amd64 hence causing a regression

  1. Can I temporarily apply this patch and upload for now - or should I wait for the testing fixes? - Since I wish to be certain that this patch actually doesn't break anything
  2. Could you modify the tests by the next release?

This is actually blocking the migration of many other packages and hence I wished this to be fixed soonish.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

AltStyle によって変換されたページ (->オリジナル) /