Rounding issues due to scaled layers

[akoch-yatta]

When testing with different diagrams, we noticed some inconsistencies when drawing on the non-diagram layers. Simple Workflow was:

1.Get bounds from a figure on the diagram layer
2. Translate bounds to FEEDBACK_LAYER
3. Use translated bounds to create an overlay figure on feedback layer

In most cases it worked correctly, but there were multiple cases, where the figure on the feedback layer was rendered a bit shifted. When having a look into it, the reason was quite obvious - rounding issues caused by Translatable#performScale. You'll find a simple snippet to see the difference when using a Translatable with and without precision.

For the Snippet the output is:

Start Bounds 10/10 52/52
Bounds 17/17 92/92
Precision Bounds 17/17 91/91
--------------------------------------------------------------------------------------------------------------
Start Bounds 10/52 52/52
Bounds 17/91 92/91
Precision Bounds 17/91 91/91
--------------------------------------------------------------------------------------------------------------
Start Bounds 52/10 52/52
Bounds 91/17 91/92
Precision Bounds 91/17 91/91
--------------------------------------------------------------------------------------------------------------
Start Bounds 52/52 52/52
Bounds 91/91 91/91
Precision Bounds 91/91 91/91
--------------------------------------------------------------------------------------------------------------

Question from me is, can we do something about it? With the additional monitor scale layer these rounding issues will become more visible, because you have up to two different layers with two separate roundings involved. Looking at the call hierarchy the only callers of Translatable#performScale are IScalablePaneHelper#translateToParent and IScalablePaneHelper#translateFromParent.

import org.eclipse.draw2d.geometry.PrecisionRectangle;
import org.eclipse.draw2d.geometry.Rectangle;

public class TranslationRoundingExample {
	public static void main(String[] args) {
		Figure root = new Figure();
		root.setBounds(new Rectangle(0, 0, 1200, 1200));
		root.setBounds(new Rectangle(0, 0, 100, 100));
		ScalableFigure f1 = new ScalableLayeredPane();
		f1.setBounds(new Rectangle(0, 0, 100, 100));
		f1.setScale(1.75f);
		IFigure f2 = new Figure();
		root.add(f1);
		f1.add(f2);

		calculateAndPrint(f2, new Rectangle(10, 10, 52, 52));
		calculateAndPrint(f2, new Rectangle(10, 52, 52, 52));
		calculateAndPrint(f2, new Rectangle(52, 10, 52, 52));
		calculateAndPrint(f2, new Rectangle(52, 52, 52, 52));
	}

	private static void calculateAndPrint(IFigure f2, Rectangle startBounds) {
		Rectangle bounds = startBounds.getCopy();
		f2.translateToAbsolute(bounds);

		Rectangle precisionBounds = new PrecisionRectangle(startBounds.getCopy());
		f2.translateToAbsolute(precisionBounds);
		System.out.println(String.format("Start Bounds %s/%s %s/%s", startBounds.x(), startBounds.y(),
				startBounds.width(), startBounds.height()));
		System.out
				.println(String.format("Bounds %s/%s %s/%s", bounds.x(), bounds.y(), bounds.width(), bounds.height()));
		System.out.println(String.format("Precision Bounds %s/%s %s/%s", precisionBounds.x(), precisionBounds.y(),
				precisionBounds.width(), precisionBounds.height()));
		System.out.println(
				"--------------------------------------------------------------------------------------------------------------");
	}
}

[ptziegler]

I don't think the issue lies with the Translatable or the IScalablePaneHelper. Rather, the imprecision is caused by this beauty here:

gef-classic/org.eclipse.draw2d/src/org/eclipse/draw2d/geometry/Rectangle.java

Lines 902 to 910 in 1d83b1b

	public Rectangle scale(double scaleX, double scaleY) {
	int oldX = x;
	int oldY = y;
	x = (int) (Math.floor(x * scaleX));
	y = (int) (Math.floor(y * scaleY));
	width = (int) (Math.ceil((oldX + width) * scaleX)) - x;
	height = (int) (Math.ceil((oldY + height) * scaleY)) - y;
	return this;
	}

Or to simply your example:

a) new Rectangle(new Point(10, 10).scale(1.75), new Dimension(52, 52).scale(1.75)) produces Rectangle(17, 17, 91, 91).
b) new Rectangle(10, 10, 52, 52).scale(1.75) produces Rectangle(17, 17, 92, 92).

Even though semantically, they are the same operation ("scale the location and size by the given factor"). So I think this algorithm is simply inaccurate.

[akoch-yatta]

Yes, of course the scale methods in Rectangle, Point, Dimension and PointList are in the end where the imprecision comes from, but we face these issues now more often because of the additional layer. So what can we do here? Some ideas:

1. Just copy over everything from PrecisionXXX to XXX e.g. PrecisionRectangle to Rectangle -> would solve it, but probably not wanted?
2. Replace only specific instances, e.g. Figure#bounds with a PrecisionRectangle -> you won't cover everything, but it should help a bit
3. Rely on people adapting all consumers into a precise copy
4. ...?

[ptziegler]

As said, the algorithm in the Rectangle class is buggy and produces a wrong result for fractional values. This is something that needs to be fixed first, before considering additional steps.

I've created #830, with which at least your snippet produces the Rectangle and PrecisionRectangle end up with the same scaled bounds. I can't say whether that's enough for your use-case, but I think it would be worth to try it out.

[HeikoKlare]

Unfortunately, that change to Rectangle does not solve the issue for us. The issue is rooted in the accumulation of rounding errors, as every intermediate result in translations is rounded. This applies to the rounding after every scaling operation (previously only the diagram scale, now also the monitor scale, i.e., two roundings accumulated) and multiplied by every translate operation performed. The issue has always been there when doing multiple translate operations, but it led to visible effects far less often because only one scaled layer instead of two was involved, reducing the number of accumulated rounding errors by half.

As one example, we have code that renders a highlighting on the feedback layer, and for that it translates a point list from a host figure to the feedback layer. It looks something like this (simplified):

   PointList hostPointList = hostFigure.getPoints().getCopy();
   hostFigure.translateToAbsolute(hostPointList);
   getFeedbackLayer().translateToRelative(hostPointList);

Here, at least 4 roundings are involved and accumulated (one per scaled layer multiplied by the two translate operations). With zoom levels that are prone to imprecision when rounding, the result will be multiple pixels off from what you would expect. For example, monitor zoom 150% and diagram zoom 250% is for me usually a good combination to see quite off results.

This is how a highlighting with the current mentiond zooms and the current implementation looks for us:

And this is how it should look, which can simply be achieved by wrapping the PointList into a PrecisionPointList before passing it to the translate methods (conforming to proposal (3) from Andreas):

[azoitl]

While I admit that we have a rounding problem in GEF Classic I see in your specific case a more substantial problem in the default setup of the layers. I never understood why the handle and feedback layer is not part of the scalable layers so that I don't always have to perform this nasty transformations at all.

[HeikoKlare]

While I admit that we have a rounding problem in GEF Classic I see in your specific case a more substantial problem in the default setup of the layers. I never understood why the handle and feedback layer is not part of the scalable layers so that I don't always have to perform this nasty transformations at all.

I see that point and it probably makes sense to discuss about that in addition.
Still what we face right now is that the translate... methods are used at multiple places and they are currently prone to rounding errors (and became more prone to it with monitor scaling being involved). And the question is what we can do about it.

One approach that we tried last week could ensure that we at least restore the "old" state, i.e., that the additional scaled layer for the monitor zoom does not lead to a multiplication of roundings. To do so, we wrapped the translatables passed to the translate... methods into a precision version, so that the calculations are done on precise values, and then converted the results back at the end, such that rounding is only done there and only done once per translate... call. This is the preliminary implementation: master...vi-eclipse:gef-classic:precision-translation

However, Andreas' proposals would go even beyond that and not only restore the existing behavior but improve the general behavior, as rounding errors already happened before the monitor scale was incorporated (just far more seldom).

[azoitl]

@HeikoKlare the reason I brought up that point is that fixing the layers may be quicker and with less impact then migrating nearly everything to the precision versions. Even if the latter is the way to go.

[ptziegler]

I had a look at master...vi-eclipse:gef-classic:precision-translation and I don't like this approach at all. This exposes a lot of low-level concepts that I don't think a consumer should ever have to worry about.

I tend to agree with @azoitl that we'll likely solve a lot of these problems by moving the feedback/handle layer into the scalable pane and therefore applying the same rounding errors everywhere.

My other concern is that this forces all consumers into a double-based geometry, even though the int-based geometry might be more than sufficient (and probably also faster). Or to put it differently: The nested, scalable layers are only created on Windows. What about Linux and MacOS?

3. Rely on people adapting all consumers into a precise copy

The problem is: Who are the "people" who do this for "all" consumers? I think it's fine switching to precise variants (where appropriate) inside Draw2D. But that should not be visible to the consumer.

[HeikoKlare]

I tend to agree with @azoitl that we'll likely solve a lot of these problems by moving the feedback/handle layer into the scalable pane and therefore applying the same rounding errors everywhere.

It sounds reasonable to at least have the feedback layer scalable, but wouldn't that also be a pretty risky change as consumers may rely on that layer not being scaled? I am pretty sure that several calculations in our product will break if that layer changes it's semantics. That would definitely be the case for when making the handle layer scalable (we require it to be unscaled). So this would probably require consumer-side adaptations (which may or may not be easier than with any other approach -- hard to tell without trying out).

I had a look at master...vi-eclipse:gef-classic:precision-translation and I don't like this approach at all. This exposes a lot of low-level concepts that I don't think a consumer should ever have to worry about.

Just to be precise: I did not want to propose that as a final solution. It was just a PoC to test the effects of wrapping the translatables inside translate...() calculations into precise values, which seemed to solved several issues for us.
The background is that we have around 1150 calls to Figure#translate...(), which are now all prone to a higher degree of imprecision due to scaling and rounding two times for each call. That's why we tested if it would not be possible to resolve imprecision of these multiple roundings by those methods by wrapping the translatable object into a precision representation. For the consumer, results should then be of same precision when using multiple scaled layers as if the scale factor was accumulated into one scalable layer.
So the goal was exectly to not leave it up to the user to deal with the increased imprecision to multiple scaled layers as far as possible. The conversion methods introduced in that branch could of course be hidden as internal details, so that the change would, e.g., be kind of local to Figure and the translate...() methods in there. But in general, the proposal was about thinking into the direction of restoring the existing precision of calculation without the consumer having to bother with it. That will of course not solve preexisting imprecision that could be solved when more holistically moving to precision values.

My other concern is that this forces all consumers into a double-based geometry, even though the int-based geometry might be more than sufficient (and probably also faster). Or to put it differently: The nested, scalable layers are only created on Windows. What about Linux and MacOS?

I am not completely sure about this point. As pointed out above, using precision values would be a hidden, internal detail not visible to the consumer (in contrast to all other approaches that have one the table which will require consumers to adapt). The argument regarding performance is probably valid, but is it really relevant? Float-precision calculation as so fast nowadays that I would be hesitant to assume that it produces a reasonable negative impact on performance.

Note that I am not fully convinced and that I do not want to convince anyone that the proposal of wrapping translatables into precision version within internal calculations is the right way to go. I just want to collect and discuss arguments and better understand what alternatives we have, as the current ones seem to put a burden on consumers.

[azoitl]

@HeikoKlare I totally agree with you that we need to find the way with the least or no impact on our clients. The reason I suggested to move the layers to scaled layers is mainly as most code working the feedback or handle layer should have a translateToAbsolute , translateToRelative combination, as in your example above. If that is the case then nothing will break for clients and the rounding errors should even out.

But if clients assume certain layer structures and make use of that then we will break them. That is clear.

[azoitl]

I just noticed if moving the feedback and handle layer into a scaled layer is breaking your product @HeikoKlare then we have to also revert 1bd2883. As this one is already doing that and breaking my product.

[ptziegler]

But in general, the proposal was about thinking into the direction of restoring the existing precision of calculation without the consumer having to bother with it. That will of course not solve preexisting imprecision that could be solved when more holistically moving to precision values.

Maybe to rephrase my initial response: To me this approach feels like the inverse of what should be done. If the translateX methods receive a Rectangle, int-based arithmetic should be done. And for a PreciseRectangle, double-based arithmetic.

   PointList hostPointList = hostFigure.getPoints().getCopy();
   hostFigure.translateToAbsolute(hostPointList);
   getFeedbackLayer().translateToRelative(hostPointList);

So in your example, I think it would make more sense for hostFigure.getPoints() to return a PrecisePointList and then treat the rounding as an implementation detail of the geometric class.

I just noticed if moving the feedback and handle layer into a scaled layer is breaking your product @HeikoKlare then we have to also revert 1bd2883. As this one is already doing that and breaking my product.

Isn't this what we're trying to fix with #824?