steinbergmedia / vstgui

A user interface toolkit mainly for audio plug-ins
Other
870 stars 124 forks source link

[Linux] Issues in cairocontext.cpp #126

Open ryukau opened 4 years ago

ryukau commented 4 years ago

Hi. I'm currently trying to make VSTGUI works on Linux and found several issues in cairocontext.cpp.

Modified cairocontext.cpp that worked in my environment is available on the link below.

1. No degree to radian conversion in drawArc

CDrawContext::drawArc takes angles in degree. However, drawArc in cairocontext.cpp directly passes degrees to cairo_arc without converting them to radian.

cairo_arc (cr, 0, 0, 1, startAngle1, endAngle2); // line 354

Multiplying 2.0 * M_PI / 360.0 fixed this issue.

cairo_arc (cr, 0, 0, 1, startAngle1 * M_PI / 180.0, endAngle2 * M_PI / 180.0);

2. Scaling factors in drawArc and drawEllipse are incorrect

In current code, numerator and denominator of scaling factor are flipped. Code below is from drawEllipse but it's same in drawArc.

CPoint center = rect.getCenter (); // line 364
cairo_translate (cr, center.x, center.y);
cairo_scale (cr, 2.0 / rect.getWidth (), 2.0 / rect.getHeight ());
cairo_arc (cr, 0, 0, 1, 0, 2 * M_PI);
draw (drawStyle);

Only flipping numerator and denominator didn't solve this issue. I also added cairo_save and cairo_restore to make transform to be local. Also this should make line width to be uniform, according to Ellipses With Uniform Stroke Width from cairo documentation.

cairo_save (cr);
CPoint center = rect.getCenter ();
cairo_translate (cr, center.x, center.y);
cairo_scale (cr, rect.getWidth () / 2.0, rect.getHeight () / 2.0);
cairo_arc (cr, 0, 0, 1, 0, 2 * M_PI);
cairo_restore (cr);
draw (drawStyle);

3. drawPolygon closes line

Here, "close" means it connects last point to first point. Unlike D2DDrawContext::drawPolygon, drawPolygon in cairocontext.cpp closes polygon.

auto& last = polygonPointList.back (); // line 318
cairo_move_to (cr, last.x, last.y);
for (auto& it : polygonPointList)
    cairo_line_to (cr, it.x, it.y);

To make drawPolygon in cairocontext.cpp to behave as same as D2DDrawContext::drawPolygon, I changed above code to the following.

auto& first = polygonPointList.front ();
cairo_move_to (cr, first.x, first.y);
for (auto it = polygonPointList.begin () + 1; it != polygonPointList.end (); ++it)
    cairo_line_to (cr, (*it).x, (*it).y);

4. Translation with ± 0.5

There are some + 0.5 and - 0.5 to slightly translate coordinate. They are causing inconsistency between Windows and Linux.

Following methods are affedted.

For example in drawLine:

cairo_move_to (cr, start.x + 0.5, start.y + 0.5); // line 266
cairo_line_to (cr, end.x + 0.5, end.y + 0.5);

These + 0.5 make location of paths to be slightly off. When I removed them, cairo put paths to correct location.

cairo_move_to (cr, start.x, start.y);
cairo_line_to (cr, end.x, end.y);
jpcima commented 4 years ago

@ryukau I think 0.5 offsets are appropriate, given that cairo's top-left pixel will be located at coordinate (0.5,0.5). I experience a problem of lines which are not displayed, but the above does not solve it. Instead black lines would appear as half-opacity grey, which suggests lines are drawn exactly on a mid-pixel coordinate, with one of the pixel lines being clipped.

I am lead to believe the problem has to do with the clipper.

Here it's a label frame rendered in Windows, using vanilla vstgui Capture du 2020-09-01 20-38-31 Same in linux, top and left line not seen Capture du 2020-09-01 20-39-45

ryukau commented 4 years ago

@jpcima Thanks for pointing out. I didn't know the coordinate convension in cairo. For my reference, it's documented in here.

However, ±0.5 is adding a offset relative to other 2D primitives. See the image below.

translate

With ±0.5 translation, the vertical line is slightly on the right relative to circle and arc.

I guess a solution to this is adding ±0.5 to everything.

jpcima commented 4 years ago

If this might help, I also discovered a bit ago a problem occurring with the flag -ffast-math. If this compiler flag is set globally in the project, a wrong computation can happen in CRect::makeIntegral which can make a pixel coordinate off by 1.

It's not an overall solution to all coordinate problems, but one worth to check.

Second to this, it's not only cairocontext.cpp but also cairopath.cpp which have ±0.5 issues. I read that context adds 0.5 to positions, path subtracts 0.5, which I think doesn't makes lots of sense. I will try to investigate the detail.

jpcima commented 4 years ago

I have made a first attempt https://github.com/sfztools/vstgui/commit/3be004ee6702e024a1fbbfddda09118703830815

This patch gets me identical drawing to D2D, although my program is not very elaborate.

to do: the paths probably.. all that I tried so far is rectangles and rounded rects. also we don't know if path is going to be a fill or stroke, so can't apply the 0.5 offset on it. (not sure if we need it)

ryukau commented 4 years ago

@jpcima The patch breaks the code using CDrawContext::Transform. On my plugins, everything except texts are translated to somewhere invisible.

I tried the change below to pixelAlign on the patch and it made drawings visible. This change aligns 1px strokes, however 2px strokes are blurred.

inline void roundPixel(const ContextHandle& handle, double x, double y)
{
    cairo_user_to_device(handle, &x, &y);
    x = std::round(x);
    y = std::round(y);
    cairo_device_to_user(handle, &x, &y);
}

CPoint pixelAlign (const ContextHandle& handle, const CPoint& point)
{
        double x = point.x;
        double y = point.y;
        roundPixel (handle, x, y);
        return CPoint(x, y);
}

CRect pixelAlign (const ContextHandle& handle, const CRect& rect)
{
        double left = rect.left;
        double top = rect.top;
        double right = rect.right;
        double bottom = rect.bottom;
        roundPixel (handle, left, top);
        roundPixel (handle, right, bottom);
        return CRect(std::round(left), std::round(top), std::round(right), std::round(bottom));
}
jpcima commented 4 years ago

This code has a problem inline void roundPixel(const ContextHandle& handle, double x, double y) change to: inline void roundPixel(const ContextHandle& handle, double& x, double& y)

You're right, this conversion has a problem. This seems working here at least, I will update the PR with this change. Thanks

jpcima commented 4 years ago

@scheffle, I have tried your fix at commit b065e1b. The fills are still misaligned.

To explain the problem I faced, here are rectangle shaped drawn in plain cairo, and source that creates it. cairo-test.cpp.gz cairo-align

As the picture shows, adding 0.5 makes strokes perfect, but then it makes fills imperfect. In the previous PR, I would work around this by adding 0.5 or not, depending if the DrawStyle is filled or not.

This is a quote from the cairo FAQ. (cf. https://www.cairographics.org/FAQ/#sharp_lines)

The reason that cairo does it this way is so that fills align nicely, at the cost that some strokes do not. It is not possible to set up sample locations in a way so that both fills and strokes with integer coordinates work nicely, so one had to be preferred over the other. One argument in favor of preferring fills is that all fills with integer coordinates align nicely this way. The best that can be done with strokes is to make all even-integer-width strokes align nicely (as they do in cairo) or to make odd-integer-width strokes align (which would then break the fill alignment).