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

Not using ScaledGraphics class degrades quality at higher zoom #637

Closed
Phillipus opened this issue Jun 1, 2020 · 57 comments
Closed

Not using ScaledGraphics class degrades quality at higher zoom #637

Phillipus opened this issue Jun 1, 2020 · 57 comments

Comments

@Phillipus
Copy link
Member

Phillipus commented Jun 1, 2020

In order to fix #621 we removed the use of the org.eclipse.draw2d.ScaledGraphics class. However, looking closer at some zoomed diagrams there are now new issues:

  1. The outer rectangle of a box is misaligned on right and bottom sides
  2. Fonts are not smooth
  3. Other artefacts
  4. Fonts loaded via Display#loadFont() are not scaled and rendered correctly (see Issues using additional fonts in user fonts folders (SVG/Loading/Rendering) #628)
@Phillipus
Copy link
Member Author

With ScaledGraphics at 800% zoom:

Box_SG

arrow_SG

Without ScaledGraphics at 800% zoom:

Box_NSG

arrow_NSG

@Phillipus Phillipus changed the title Not using ScaledGraphics class degraded quality at higher zoom Not using ScaledGraphics class degrades quality at higher zoom Jun 1, 2020
@Phillipus
Copy link
Member Author

Here's an example of a font zoomed in at 800%:

With ScaledGraphics at 800% zoom:

font_SG

Without ScaledGraphics at 800% zoom:

font_NSG

@Phillipus
Copy link
Member Author

Phillipus commented Jun 1, 2020

I'm beginning to think that Draw2d uses ScaledGraphics for a good reason.

What we could do is copy ScaledGraphics and have our own version that we could hack and use that one (we already do that with the Thumbnail class). Caveat - I don't know what needs changing in ScaledGraphics to fix the original font clipping issue.

@Phillipus
Copy link
Member Author

Another solution is to re-instate ScaledGraphics and use the workaround for TextFlow as mentioned here:

https://www.eclipse.org/forums/index.php?t=msg&th=154575&goto=487928&#msg_487928

fTextFlow = new TextFlow() {
    @Override
    protected void paintFigure(Graphics g) {
        g.setClip(getBounds().getCopy().resize((int)(g.getAbsoluteScale()*3), 0));
        super.paintFigure(g);
    }
};

@jbsarrodie
Copy link
Member

Another solution is to re-instate ScaledGraphics and use the workaround for TextFlow

No because it doesn't solves the issue (rendering is also ugly and text is still truncated in several cases)

  • The outer rectangle of a box is misaligned on right and bottom sides

That's simply because we don't set a borderwidth, ScaledGraphics was introducing an issue that Archi adressed in drawing figures by substracting one pixel out of dimensions, but in fact the real fix is to shrink dimension by one in every directions and draw a 1px border.

  • Fonts are not smooth

Are we sure that the underlying graphics is configured as "advanced" and uses antialias ?

FWIW, If I have to choose, I prefer having a non smooth font at 800% than a smooth but truncated one.

  • Other artefacts

For relationships, that's in fact exactly the same in SVG output (at least on Linux), so this makes me think that the real issue lies in the figure drawing code, not the graphics itself (it was just invisible when using ScaledGraphics, but I guess for bad reasons)

I have to look at this one in detail, but this might be acceptable in the short/mid terme to explain that sideloading fonts is an experimental figure with known limitations which should be hopefuly solved in future release.

@Phillipus
Copy link
Member Author

That's simply because we don't set a borderwidth, ScaledGraphics was introducing an issue that Archi adressed in drawing figures by substracting one pixel out of dimensions, but in fact the real fix is to shrink dimension by one in every directions and draw a 1px border.

If we edit RectangleFigureDelegate to do that, it still causes misalignment when exporting to SVG.

Are we sure that the underlying graphics is configured as "advanced" and uses antialias ?

Draw2D is doing the drawing on the underlying graphics for figures, not us. But yes they are on. Also, for fonts, I think ScaledGraphics is better at scaling fonts than SWTGraphics.

think that the real issue lies in the figure drawing code

I tried different things for connection Figures, but it still looks the same.

@Phillipus
Copy link
Member Author

I don't know what to do about this any more. I tried to improve the figures by adjusting the bounds of the each figure but can never get a solution.

Here's an Application Component as it is now (no ScaledGraphics class):

view_now

And here's the SVG export:

svg_no_changes

Here it is with some x, y, width, height adjustments made in the figure to compensate:

view_adjusted

But here's the SVG export:

svg_with_changes

@Phillipus
Copy link
Member Author

For comparison, here's using ScaledGraphics:

View:

old_view

SVG:

old_svg

@Phillipus
Copy link
Member Author

It seems that ScaledGraphics has some advantages but is getting something wrong when rendering scaled fonts. I think it is getting the distance between characters too wide so the overall width is too wide and this is then being clipped.

@Phillipus
Copy link
Member Author

Here's an experiment setting the figure's line width to 1:

view2

But here's the SVG export:

svg2

@Phillipus
Copy link
Member Author

Here's the same experiment setting the line width to 1 but using ScaledGraphics:

View:

view_scaled

SVG:

svg_scaled

@Phillipus
Copy link
Member Author

Phillipus commented Jun 5, 2020

I wondered whether Modelio uses ScaledGraphics so I looked at their source code. They have a class called ScalableFreeformLayeredPane2 which extends ScalableFreeformLayeredPane and they also have the same workaround as we are now using in our ExtendedScalableFreeformLayeredPane:

    protected void paintClientArea(Graphics graphics) {
        if (getChildren().isEmpty()) {
            return;
        } else if (getScale() == 1.0) {
            super.paintClientArea(graphics);
        } else {
            boolean optimizeClip = getBorder() == null || getBorder().isOpaque();
            if (!optimizeClip) {
                graphics.clipRect(getBounds().getShrinked(getInsets()));
            }
            
            graphics.scale(getScale());
        
            graphics.pushState();
            paintChildren(graphics);
            graphics.popState();
            graphics.restoreState();
        }
    }

And they also have a comment in that code:

/**
 * Same as {@link ScalableFreeformLayeredPane} but don't use a ScaledGraphics
 * to paint the area, this class sucks at zooming texts.
 */

See https://github.com/ModelioOpenSource/Modelio/blob/master/modelio/app/app.diagram.elements/src/org/modelio/diagram/elements/core/figures/freeform/ScalableFreeformLayeredPane2.java

@Phillipus
Copy link
Member Author

Phillipus commented Jun 5, 2020

Modelio have a class ZoomableLineBorder which sort of does what we have tried in our experiments - shrinking the bounds. Also note the use of getWidth() % 2 which reduces width and height only for odd widths.

    public void paint(IFigure figure, Graphics graphics, Insets insets) {
        AbstractBorder.tempRect.setBounds(getPaintRectangle(figure, insets));
        if (getWidth() % 2 != 0) {
            AbstractBorder.tempRect.width--;
            AbstractBorder.tempRect.height--;
        }
        AbstractBorder.tempRect.shrink(getWidth() / 2, getWidth() / 2);
        
        ZoomDrawer.setLineWidth(graphics, getWidth());
        graphics.setLineStyle(getStyle());
        
        if (getColor() != null) {
            graphics.setForegroundColor(getColor());
        }
        graphics.drawRectangle(AbstractBorder.tempRect);
    }

@Phillipus
Copy link
Member Author

Aha! I remembered something in ExtendedGraphicsToGraphics2DAdaptor:

/**
 * JB fix - decrease width and height by 1 pixel
 */
@Override
public void fillRectangle(int x, int y, int width, int height) {
    Rectangle2D rect = new Rectangle2D.Float(x + transX, y + transY, width - 1, height - 1);

    checkState();
    getGraphics2D().setPaint(getColor(getSWTGraphics().getBackgroundColor()));
    getGraphics2D().fill(rect);
}

/**
 * JB fix - decrease width and height by 1 pixel
 */
@Override
public void fillRoundRectangle(Rectangle rect, int arcWidth, int arcHeight) {
    RoundRectangle2D roundRect = new RoundRectangle2D.Float(rect.x + transX, rect.y + transY, rect.width - 1, rect.height - 1, arcWidth,
            arcHeight);

    checkState();
    getGraphics2D().setPaint(getColor(getSWTGraphics().getBackgroundColor()));
    getGraphics2D().fill(roundRect);
}

These are not needed now.

@Phillipus
Copy link
Member Author

Phillipus commented Jun 5, 2020

But there are other issues now we are not using ScaledGraphics such as the icon drawing:

co

ar

That is like that even at 100%

@Phillipus
Copy link
Member Author

It looks like most of the Figures will have to be re-done.

@Phillipus
Copy link
Member Author

I've created a branch figure-fixes with a few fixes for the Figures. There's more to be done.

@Phillipus
Copy link
Member Author

Here's an example of a font zoomed in at 800%:

With ScaledGraphics at 800% zoom:

font_SG

Without ScaledGraphics at 800% zoom:

font_NSG

I just checked the quality of zoomed fonts in Modelio. Same thing, as they also don't use ScaledGraphics

@Phillipus
Copy link
Member Author

I've done some more fixes for some Figures in the figure-fixes branch.

For simple shapes like rectangles and rounded rectangles this was simply a case of shortening the width and height of a rectangle by 1 pixel both for the fill and for the outline (when we used ScaledGraphics we only did that for the outline).

However, I can't fix where we've used a PointList to draw polygons.

For example, AbstractMotivationFigure uses the same PointList for the fill and for the outline, and yet they are drawn differently:

assessment

The ScaledGraphics class has been tuned to draw graphics differently depending on the current scale factor and seems to do a better job. SWTGraphics doesn't do this.

@Phillipus
Copy link
Member Author

It seems that a certain other Eclipse-based tool also has some problems when drawing at higher scale factors:

m_assessment

Ouch.

@jbsarrodie
Copy link
Member

For example, AbstractMotivationFigure uses the same PointList for the fill and for the outline, and yet they are drawn differently:

In most of your attempts and test, borderwidth is not set, this is mostly why there are issues when zooming. In fact without a bordersidth set, there are several issues because the physical linewith on the screen is always 1 pixel and never zoomed while it should be.

So the only way I see to fix this is:

  • set a 1px border on figure
  • shrink the "drawing" dimensions by 1 unit (in all directions). This is the trick to fix the clipping which occures on the border itself
  • Check figure drawing code to remove any other dimensions adjustments which were created only to accomodate the fact that borderwidth was not set
  • When OK, check SVG related code to remove any other compensation factors introduced

And for an easier way to manage code, I would make sure that we have a private method to draw the figure which does is strictly based on a Rectangle (no shrinking), and then have the public draw method creating a Rectangle based on getting the figure dimensions shrinked by 1 and then calling the private method. This should then allow us to easily change the borderwith by using another value instead of 1 for shrinking (this value being related to the wanted borderwidth).

I did some experiments some weeks ago (which led me to the "BTW I find a way to fix the borderwidth issue" comment) and I was able to adjust borderwidth without impact on zoom (but I did test only on Business Actor at that time).

I'm a bit overloaded atm so can't test or contribute unfortunately :-(

@jbsarrodie
Copy link
Member

I just checked the quality of zoomed fonts in Modelio. Same thing, as they also don't use ScaledGraphics

IMHO, such anti-aliasing issues are not a big deal. I think it is preferable (and hardly noticeable at zoom factors less than 200%) than having truncated text at most zoom factors.

@Phillipus
Copy link
Member Author

Shrinking the bounds by 1 pixel will fix the clipping but there is a gap around the figure which causes a gap in the connection (I've shown the selection boxes here to show the overall shrinkage)

shrunk

@jbsarrodie
Copy link
Member

I didn't had this when I did test.

Is it visible even at 100% ?

So you experience clipping if you don't shrink ?

@Phillipus
Copy link
Member Author

Is it visible even at 100% ?

Not really, but it is in SVG.

@Phillipus
Copy link
Member Author

Phillipus commented Jun 17, 2020

I've made progress on the Figures. I want to get us to where we were before removing ScaledGraphics. And once that is done we can think about setting line widths later.

  1. Remove the minus 1 pixel width and height adjustments in ExtendedGraphicsToGraphics2DAdaptor
  2. Make adjustments for width and height by minus 1 pixel in some figures
  3. Use org.eclipse.swt.graphics.Path for drawing fills instead of org.eclipse.draw2d.geometry.PointList.

The key is to use org.eclipse.swt.graphics.Path for drawing where possible.

Here's an example of AbstractMotivationFigure before these changes. This uses Graphics.fillPolygon(points):

before

And this one uses Graphics.fillPath(path):

after

I wrote a utility method to convert the co-ordinates of org.eclipse.draw2d.geometry.PointList into a org.eclipse.swt.graphics.Path.

By doing this we can draw with more accuracy.

All of this is in the figure-fixes branch.

I'll release another beta.

@Phillipus
Copy link
Member Author

Phillipus commented Jun 17, 2020

Damn.

I've just tested on Mac/Linux and there is a difference in how lines are drawn on Mac/Linux now that we are not using ScaledGraphics. On Mac/Linux all lines have a width of 1 and look like this:

Screenshot 2020-06-17 at 17 37 06

When we used ScaledGraphics it looked like this:

Screenshot 2020-06-17 at 17 48 53

This means that Mac/Linux users will see an uneven outline in the figures.

(Edited to include Linux)

@jbsarrodie
Copy link
Member

Does this happen only when zooming? Can you do a screenshot of a figure at 100%, 200% and 300%.

When we used ScaledGraphics it looked like this:

This as no borderwidth. Do you means that even with a borderwidth defined, ScaledGraphics was drawing this ?

For me what you get is the "usual" issue: when drawn, the border is centered on the defined coordinate, thus it spans accross clipping area.

Let's say you have a figure from (10,10) to (20,20). When drawing a 3px border, it ends up being drawn (thats an example and not perfectly accurate information) from (9,9) to (21,21) because there are 3px to draw the inner part of the box will be (11,11) to (19,19).

TBH, it seems that draw2d always put the bottom & right borders inside the clipping area while the top & left borders are "on" the limit of the clipping area. So we have to see when (which borders) to remove what (my guess is floor(borderwidth/2)+1) on which OS (in case it is OS dependant).

@Phillipus
Copy link
Member Author

I have found a method to draw a rectangle with a specified line width that doesn't require any shrinking. Again, it means that we must do all drawing using org.eclipse.swt.graphics.Path

bounds.width--;
bounds.height--;

float lineWidth = 1;
float offSet = lineWidth / 2;
graphics.setLineWidthFloat(lineWidth);

Path path = new Path(null);
path.moveTo(bounds.x + offSet, bounds.y + offSet);
path.lineTo(bounds.x + bounds.width - offSet, bounds.y + offSet);
path.lineTo(bounds.x + bounds.width - offSet, bounds.y + bounds.height - offSet);
path.lineTo(bounds.x + offSet, bounds.y + bounds.height - offSet);
path.lineTo(bounds.x + offSet, bounds.y);
graphics.drawPath(path);
path.dispose();
  1. We reduce width and height by 1 pixel
  2. We set the line width to 1 (or 2 or 3)
  3. We divide the line width by 2 to get a pixel offset
  4. We draw a rectangle using the bounds x, y, width and height BUT add and subtract the offSet for x and y

In the case of a line width of 1, the offset is 0.5. This works for all line widths. And we don't need to use bounds.shrink()

@Phillipus
Copy link
Member Author

Phillipus commented Jun 17, 2020

I've committed the changes to RectangleFigureDelegate in the figure-fixes branch.

Try it for rectangle figures. It works on Windows and Mac.

@Phillipus
Copy link
Member Author

Phillipus commented Jun 17, 2020

So it seems to me that if we are going to dispense with using ScaledGraphics and Mac is setting the line width to 1 whether we like it or not, we must now find a way to draw all figures with a line width of 1 on all platforms and no longer use the "magic" line width that we have been using.

Drawing operations that use org.eclipse.swt.graphics.Path have a greater accuracy than those that take integers such as graphics.drawRectangle(). Mixing the two drawing methods lead to misalignments,

@Phillipus
Copy link
Member Author

Phillipus commented Jun 17, 2020

However, I don't think it will be possible to do this for figures other than rectangles. For example, there is no equivalent Path method of graphics.drawRoundRectangle() (this uses integers not float). It does have addArc() methods but these would have to be calculated.

@Phillipus
Copy link
Member Author

Fuck! I may have found the solution.... (look at the SWTGraphics source code...)

@jbsarrodie
Copy link
Member

However, I don't think it will be possible to do this for figures other than rectangles. For example, there is no equivalent Path method of graphics.drawRoundRectangle() (this uses integers not float). It does have addArc() methods but these would have to be calculated.

Where there's Math & Geometry there's a way... with me ;-)

look at the SWTGraphics source code...

But where ?????????

@Phillipus
Copy link
Member Author

But where ?????????

public void translate(float dx, float dy)

@jbsarrodie
Copy link
Member

public void translate(float dx, float dy)

Seems like you want to translate(offset, offset) or something similar ?

@Phillipus
Copy link
Member Author

Seems like you want to translate(offset, offset) or something similar ?

Yes. The problem is that in most cases we want x and y to be 0.5 pixel indented. With translate we can do this.

Check out rectangle and rounded rectangle figures in the latest commit in figure-fixes branch ;-)

@Phillipus
Copy link
Member Author

So far, this seems to be working. So hopefully we'll be able to achieve our goal of setting line width...

@Phillipus
Copy link
Member Author

Phillipus commented Jun 18, 2020

Cancel all of those last three messages (which I have deleted) and slap me round the head. I had hard coded a "1" for line width somewhere...

@jbsarrodie
Copy link
Member

image

@Phillipus
Copy link
Member Author

Right then.

I've re-written the code for all of the Figures (yes, it was a pain in the butt). All figures are now drawn with a line width of 1. It also works when setting the line width to 2 or 3 (any more than that looks silly.)

I've tested all of them at different zoom levels and exporting to SVG.

Let's test this and hope there are no more issues...

All in the figure-fixes branch.

@jbsarrodie
Copy link
Member

Great. I'll test tonight !

@Phillipus
Copy link
Member Author

Phillipus commented Jun 18, 2020

Bear in mind a few small caveats. These are things that we are going to have to accept as a small compromise. The thing is, though, as we are no longer using ScaledGraphics then we have to draw all figures with an actual line width because Mac/Linux are not using the "magic" line width (defaults to 1 and looks terrible).

So our choices are:

  1. Go back to using ScaledGraphics and try and fix the text clipping issue
  2. Don't use ScaledGraphics and keep the "magic" line width but it looks terrible on Mac
  3. Don't use ScaledGraphics and set default line width to 1 and use Graphics#translate() to compensate

Caveat:

  • Graphics#translate() is called when setting the line width. This will ensure outlines are placed correctly but it means the fill colour is indented (a bit like calling Bounds#shrink). You won't see this as the outline is drawn on top of the fill, but you will see it if you set the outline opacity to zero. Tough luck on that. I tried to draw the fill before calling Graphics#translate() for the outline but it causes over-spill on some figures and when exporting to SVG.

Here's the code that does it:

bounds.width -= lineWidth;
bounds.height -= lineWidth;
graphics.setLineWidthFloat(lineWidth);
graphics.translate(lineWidth / 2, lineWidth / 2);

@jbsarrodie
Copy link
Member

Even without having tested this latest version, I choose option 3 as it is for me what should have been the right behavior from the begining if we didn't had to cope with eclipse/ScaledGraphics issues.

@Phillipus
Copy link
Member Author

I agree. I've tested it and it's OK. Given the nature of Draw2d we will always have to compromise somewhere. I think we do a pretty good job at making the best of what we have to deal with.

A small reminder of how it could be done... ;-)

m_assessment

@Phillipus
Copy link
Member Author

as it is for me what should have been the right behavior from the begining

It only took 10 years... ;-)

@Phillipus
Copy link
Member Author

So our choices are:

  1. Go back to using ScaledGraphics and try and fix the text clipping issue
  2. Don't use ScaledGraphics and keep the "magic" line width but it looks terrible on Mac
  3. Don't use ScaledGraphics and set default line width to 1 and use Graphics#translate() to compensate

Actually, this is a 4th choice:

  1. Go back to using ScaledGraphics and fix the text clipping issue and set the default line width to 1 using Graphics#translate()

I only mention it to point out that the line width fix using Graphics#translate() works both with and without ScaledGraphics.

@jbsarrodie
Copy link
Member

Actually, this is a 4th choice:

Yes... but no ;-) because it is (almost) impossible to fix the font clipping issue.

I only mention it to point out that the line width fix using Graphics#translate() works both with and without ScaledGraphics.

That's good new though.

@Phillipus
Copy link
Member Author

(I'm not advocating for some of these choices, I just want to record them here so we know why we made these decisions)

BTW - you can actually set line widths to floats like 1.5 and 2.5 and it works.

Suggest this:

  1. You test this
  2. We issue a new beta
  3. If it is all OK we could add a line width section to the Properties view (would need to decide what are reasonable limits, a line width greater than 3 or 4 is not a good look)

@Phillipus
Copy link
Member Author

Phillipus commented Jun 19, 2020

That's good new though.

Everything we do should work with or without ScaledGraphics...at 100% scale. ScaledGraphics is only used by GEF if scaling/zoom != 1.0:

	protected void paintClientArea(Graphics graphics) {
		if (getChildren().isEmpty())
			return;
		if (scale == 1.0) {
			super.paintClientArea(graphics);
		} else {
			ScaledGraphics g = new ScaledGraphics(graphics);

@jbsarrodie
Copy link
Member

Suggest this:

  1. You test this

I've just tested and everything seems ok. Good !

@Phillipus
Copy link
Member Author

Phillipus commented Jun 19, 2020

You can even set the line width to 0 and we are back to how it was before. Of course, line width of 0 doesn't work on Mac or Linux so it would have to be Windows only...

(Edited to include Linux)

@jbsarrodie
Copy link
Member

Some comments on Archi 4.7 beta 7 on this aspect.

I can still see some small drawing issues (here at 400%):
image

  • Arrowhead is a bit truncated on the left and is not fully filled
  • Rounded connection is not so rounded

I can also see some slight differences between 100% and other zoom factors (I guess this is because in this case we don't use translate() and other tricks.

All in all, there is nothing requesting any change in the short term. This is really acceptable for 4.7 final. I'll try to look at them for 4.7.1 or 4.8.

@Phillipus
Copy link
Member Author

Arrowhead is a bit truncated on the left and is not fully filled

It was truncated before. The fill problem is because it is drawing an outline and a fill and they do not line up.

I created a new class com.archimatetool.editor.diagram.figures.connections.PathDrawnPolygonDecoration that helps with AggregationConnectionFigure and CompositionConnectionFigure but for the other connections I could not resolve them. I dug deep into the code and I think there are issues to do with clipping in one of the super classes of org.eclipse.draw2d.PolylineConnection

But connections need further investigation as you say.

@Phillipus
Copy link
Member Author

Let's close this and open individual issues for each individual case.

@Phillipus
Copy link
Member Author

A follow up to this.

Not using scaled graphics in Archi 4.7 has made an improvement to connections that have dots/dashes when exporting to an image at scales greater than 100%, In Archi 4.6 the dots/dashes didn't scale when exporting at higher scales.

Here's an image export at 200% from Archi 4.6:

Default View

And here's the same image export at 200% from Archi 4.7:

Default View2

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

No branches or pull requests

2 participants