Rounding issues due to scaled layers
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(
"--------------------------------------------------------------------------------------------------------------");
}
}
I don't think the issue lies with the Translatable or the IScalablePaneHelper. Rather, the imprecision is caused by this beauty here:
https://github.com/eclipse-gef/gef-classic/blob/1d83b1bd13c69c71ebd8edb461bea1b565dc07de/org.eclipse.draw2d/src/org/eclipse/draw2d/geometry/Rectangle.java#L902-L910
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.
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:
- Just copy over everything from PrecisionXXX to XXX e.g. PrecisionRectangle to Rectangle -> would solve it, but probably not wanted?
- Replace only specific instances, e.g. Figure#bounds with a PrecisionRectangle -> you won't cover everything, but it should help a bit
- Rely on people adapting all consumers into a precise copy
- ...?
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.
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):
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.
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: https://github.com/eclipse-gef/gef-classic/compare/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).
@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.
I had a look at https://github.com/eclipse-gef/gef-classic/compare/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?
- 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.
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.
@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.
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 https://github.com/eclipse-gef/gef-classic/commit/1bd28834343bce2bf73cbf66d9cb2accc54382f0. As this one is already doing that and breaking my product.
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 https://github.com/eclipse-gef/gef-classic/pull/824?
If the
translateXmethods receive aRectangle, int-based arithmetic should be done. And for aPreciseRectangle, double-based arithmetic.
I understand the idea and maybe that claims for adapting consumers or adapting the providers of values that are passed into the translate... methods by consumers, as you pointed to with hostFigure.getPoints().
But I am not sure I a fully agree with that expectation as I don't think that fits to the implicit expectations of existing consumers. Let's take an example: I have a Point (15, 15) that I want to translate to parent (i.e., the scales of the scaled layers are applied). Diagram zoom and monitor zoom are both 125%.
Then the calculations are as follows:
- Current: Math.floor(Math.floor(15 * 1.25) * 1.25) = Math.floor(Math.floor(18.75) * 1.25) = Math.floor(22.5) = 22
- My expectation: Math.floor(15 * 1.25 * 1.25) = Math.floor(23.43) = 23
Why do I expect that? The translation calculation and the involved arithmetic operations should be completely transparent to me. I do not want to have my calculation results depend on how scalings are spread across layers. Since I pass an int-based value, the result will also be (applied to) an int-based value, but I do not want the rounded result to depend on how often scales are applied and rounded. At least, that's what probably most consumers currently expect, as effectively rounding was only done once (because of just the diagram scale applied), such that the rounded result with basing an int-based translatable or a precise one will be the same.
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.
Sorry for maybe being a bit imprecise here: the problem would arise if moving the feedback and handle layer to a scaled layer including the diagram zoom, as current consumers expect those layers to be independent of the diagram zoom. That's the part that will break. The mentioned commit of course moved those layers to the scaled layers including the monitor zoom, as handle and feedback layer of course need to include the monitor zoom to be compatible with the existing behavior. Otherwise, all consumers would need to implement the monitor scaling adaptation in their calculations.
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 https://github.com/eclipse-gef/gef-classic/pull/824?
Yes it is. I was a bit in a witty mode yesterday. Sorry for that. That was unprofessional.
Sorry for maybe being a bit imprecise here: the problem would arise if moving the feedback and handle layer to a scaled layer including the diagram zoom, as current consumers expect those layers to be independent of the diagram zoom. That's the part that will break. The mentioned commit of course moved those layers to the scaled layers including the monitor zoom, as handle and feedback layer of course need to include the monitor zoom to be compatible with the existing behavior. Otherwise, all consumers would need to implement the monitor scaling adaptation in their calculations.
The code that I have seen is handling the differences in diagram zoom by using the translatetoAbsolute / translatetoRelative pair. With that moving the layers would not make a difference. But I may be wrong.
The code that I have seen is handling the differences in diagram zoom by using the translatetoAbsolute / translatetoRelative pair. With that moving the layers would not make a difference. But I may be wrong.
Of course translations would be correct, but the visual appearance of the layers would be different. As an example: we draw popup menus on the handle layer which are supposed to have UI scaling (not diagram scaling). Integrating diagram zoom into the handle layer would break that behavior. We may accept that, as we can of course add a custom layer on which we draw that stuff instead, but it's definitely something to consider as there may be other similar consumers that need to adapt accordingly. And for those usage scenarios (any layer incorporating monitor zoom but not diagram zoom), you will still need the translate methods and need to deal with the accumulated rounding errors (or make sure to pass double-based types).
Regardless of which approach is the best, I've created https://github.com/eclipse-gef/gef-classic/pull/840. From my understanding, this issue can be closed once those tests succeed, correct?
@HeikoKlare thx for your patience with me. I was to focused on positions and forgot about the size of elements to be drawn.
Replace only specific instances, e.g. Figure#bounds with a PrecisionRectangle -> you won't cover everything, but it should help a bit
I've played around with this for a bit and I think this is the least invasive approach that should cover most use-cases. The PointList is a little bit tricky, primarily because the field can be set directly.
https://github.com/eclipse-gef/gef-classic/blob/deaf818116c1b5a9ee97cce689d44b9ca7ea3834/org.eclipse.draw2d/src/org/eclipse/draw2d/AbstractPointListShape.java#L190-L194
But changing the Rectangle to a PrecisionRectangle is relatively safe.
Just for completeness I wanted to document here that Using everywhere PrecisionRectangle will have a performance impact. Based on the discussion in #830 I looked more thoroughly on PrecisionRectangle. Any operation on PrecisionRectangle is in the best case at lest 2-3 times slower then the normal Rectangle. And this only if we are fully reworking every operation in PrecisionRectangle to avoid unnecessary double changes. The reason for that is that any change of x, y, w, h. requires to also update the integer versions. If we follow what we found out in #830 for all changes in x and y we need to also update w, and h. If we are not optimizing some of the methods would update w and h twice. Then the performance is even worse.
I have no clue what difference this makes on a modern machine. But we have diagrams with several 1000 figures, maybe even 100k).
Also it should be noted that PrecisionRectangle needs three time the memory of a normal Rectangle.
The only chance we have to reduce that is to introduce API breaking changes. I would have two in mind:
- Introduce interfaces for all geometry classes
- make the x, y, width, and height field in
Rectangleprivate and use the getters and setters everywhere.
The second we should at least for GEF Classic code do. Otherwise PrecisionRectangle could have inconsistent data.
Any operation on PrecisionRectangle is in the best case at lest 2-3 times slower then the normal Rectangle. And this only if we are fully reworking every operation in PrecisionRectangle to avoid unnecessary double changes. The reason for that is that any change of x, y, w, h. requires to also update the integer versions.
From my experience, a linear increase in complexity is usually a rounding error in terms of performance. And if calls to getBounds() are the bottleneck, it's usually more beneficial to figure out why it needs to be called that often. I know that I used a similar argument before. Nonetheless do I think that this shouldn't be the deciding factor if there are real benefits to switching to precise geometry.
If there are noticeable differences in applications, that's obviously a different story...
That said, PrecisionRectangles don't have to be used everywhere. If you have something like:
Rectangle r = fig.getBounds().getCopy();
layer.translateToAbsolute(r);
layer.translateToRelative(r);
The rounding errors should already disappear if fig uses a PrecisionRectangle, even if all of its parents still use Rectangles. You could obviously also use something like this:
Rectangle r = new PrecisionRectangle(fig.getBounds());
layer.translateToAbsolute(r);
layer.translateToRelative(r);
Or you could make it an opt-in functionality, where some editors use Rectangles, others use PrecisionRectangles.
The only chance we have to reduce that is to introduce API breaking changes. I would have two in mind:
- Introduce interfaces for all geometry classes
- make the x, y, width, and height field in Rectangle private and use the getters and setters everywhere.
I fear that this would be slightly less devastating than the SWT team removing e.g. the x and y fields of org.eclipse.swt.graphics.Point. I don't think that's a realistic option.
The second we should at least for GEF Classic code do. Otherwise PrecisionRectangle could have inconsistent data.
But that's already the case right now, isn't it? And even then, the PrecisionRectangle tries to synchronize the data whenever possible. The few situations where you get inconsistent data is if you were to e.g. set x and then read preciseX. But preciseX (and alike) have already been deprecated since 3.7 and will be internalized eventually. I'm not sure if this is a practical use-case.
Please don't get me wrong. I'm all in for switching to the precise versions where possible. I just wanted to point out some point that I noticed that we need to consider and take care. And what I always preach to my students I also need to do my self:
before optimizing measure!
The only chance we have to reduce that is to introduce API breaking changes. I would have two in mind:
- Introduce interfaces for all geometry classes
- make the x, y, width, and height field in Rectangle private and use the getters and setters everywhere.
I fear that this would be slightly less devastating than the SWT team removing e.g. the x and y fields of org.eclipse.swt.graphics.Point. I don't think that's a realistic option.
I'm not sure if I expressed myself correctly: I always meant on draw2d side.
The second we should at least for GEF Classic code do. Otherwise PrecisionRectangle could have inconsistent data.But that's already the case right now, isn't it? And even then, the PrecisionRectangle tries to synchronize the data whenever possible. The few situations where you get inconsistent data is if you were to e.g. set x and then read preciseX. But preciseX (and alike) have already been deprecated since 3.7 and will be internalized eventually. I'm not sure if this is a practical use-case.
I'm also not sure but I know that in my code I'm nearly always writing directly to the fields of the none precise versions. I find the code for synchronizing the ints back to the precise version a bit questionable. Especially when we implement the different rounding as discussed in #840 we will need to touch this code as well.
But currently I don't see a different solution.
I'm not sure if I expressed myself correctly: I always meant on draw2d side.
I know, but I just wanted to give an example that would be just as devastating. The Draw2D geometries are too widely used, which makes their API effectively untouchable. Users will not adapt their code if suddenly, they're no longer able to update those fields directly.
I'm also not sure but I know that in my code I'm nearly always writing directly to the fields of the none precise versions. I find the code for synchronizing the ints back to the precise version a bit questionable. Especially when we implement the different rounding as discussed in https://github.com/eclipse-gef/gef-classic/pull/840 we will need to touch this code as well.
Maybe to clarify: From an outside perspective, both a Rectangle and a PrecisionRectangle always contain consistent data. The double-based coordinates in the PrecisionRectangle should be treated as internal values that are only used to keep track of the rounding error and the user should only ever need to look at the int-based coordinates.
In cases where the fields have completely different values (e.g. after updating the fields directly), everything will be synchronized once a method is called.
I'm not sure if I expressed myself correctly: I always meant on draw2d side.
I know, but I just wanted to give an example that would be just as devastating. The Draw2D geometries are too widely used, which makes their API effectively untouchable. Users will not adapt their code if suddenly, they're no longer able to update those fields directly.
Ah now I get it. Yes you are totally right.
I'm also not sure but I know that in my code I'm nearly always writing directly to the fields of the none precise versions. I find the code for synchronizing the ints back to the precise version a bit questionable. Especially when we implement the different rounding as discussed in #840 we will need to touch this code as well.
Maybe to clarify: From an outside perspective, both a
Rectangleand aPrecisionRectanglealways contain consistent data. The double-based coordinates in thePrecisionRectangleshould be treated as internal values that are only used to keep track of the rounding error and the user should only ever need to look at the int-based coordinates.
Thx for this point of view. I think I was to focused on the floating point values being the main source of truth.