Bug 237802 - RelativeBendpoint.getLocation can create precision errors
Summary: RelativeBendpoint.getLocation can create precision errors
Status: RESOLVED FIXED
Alias: None
Product: GEF
Classification: Tools
Component: GEF-Legacy Draw2d (show other bugs)
Version: 3.4   Edit
Hardware: PC Windows XP
: P3 normal (vote)
Target Milestone: 3.4.1 (Ganymede SR1)   Edit
Assignee: Anthony Hunter CLA
QA Contact:
URL:
Whiteboard:
Keywords: contributed
Depends on:
Blocks:
 
Reported: 2008-06-19 11:32 EDT by Stéphane Lizeray CLA
Modified: 2011-05-17 21:48 EDT (History)
3 users (show)

See Also:


Attachments
fix the precision error (839 bytes, patch)
2008-06-19 11:32 EDT, Stéphane Lizeray CLA
no flags Details | Diff
Patch that contains Randy Hudson's fix. (985 bytes, patch)
2008-08-08 10:17 EDT, Romain Raugi CLA
ahunter.eclipse: iplog+
Details | Diff
Screenshots, before and after having applied the fix. (60.05 KB, image/png)
2008-08-18 09:02 EDT, Romain Raugi CLA
no flags Details
Unit test to highlight the bug. (4.07 KB, text/java)
2008-08-18 09:03 EDT, Romain Raugi CLA
ahunter.eclipse: iplog+
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Stéphane Lizeray CLA 2008-06-19 11:32:21 EDT
Created attachment 105424 [details]
fix the precision error

Build ID: I20080530-1730

Steps To Reproduce:


More information:
use double instead of float for the weight
Comment 1 Randy Hudson CLA 2008-06-19 12:10:12 EDT
API can not be changed.  Will this work instead:
return new PrecisionPoint((a1.preciseX() + d1.preciseWidth())
	* (1f - weight) + weight * (a2.preciseX() + d2.preciseWidth()),
	(a1.preciseY() + d1.preciseHeight()) * (1f - weight) + weight
	* (a2.preciseY() + d2.preciseHeight()));

Change 1f to 1.0, so that the subtraction is done using double precision.
Comment 2 Stéphane Lizeray CLA 2008-06-20 03:42:11 EDT
Well, for my use case, yes your suggestion fixes the bug.

Regarding changing the signature from float to double, the API will be backward compatible, so what is the problem ?

Because even it fixes my current problem, there is still a potential problem with GMF,

https://bugs.eclipse.org/bugs/show_bug.cgi?id=237803

org.eclipse.gmf.runtime.diagram.ui.editparts.ConnectionEditPart#refreshBendpoints

    protected void refreshBendpoints() {
        RelativeBendpoints bendpoints = (RelativeBendpoints) getEdge()
            .getBendpoints();
        List modelConstraint = bendpoints.getPoints();
        List figureConstraint = new ArrayList();
        for (int i = 0; i < modelConstraint.size(); i++) {
            org.eclipse.gmf.runtime.notation.datatype.RelativeBendpoint wbp = (org.eclipse.gmf.runtime.notation.datatype.RelativeBendpoint) modelConstraint
                .get(i);
            RelativeBendpoint rbp = new RelativeBendpoint(getConnectionFigure());
            rbp.setRelativeDimensions(new Dimension(wbp.getSourceX(), wbp
                .getSourceY()), new Dimension(wbp.getTargetX(), wbp
                .getTargetY()));
            if (modelConstraint.size() == 1) {
            	rbp.setWeight(0.5f);
            } else {
--->           	rbp.setWeight(i / ((float) modelConstraint.size() - 1));
            }
            figureConstraint.add(rbp);
        }
        getConnectionFigure().setRoutingConstraint(figureConstraint);
    }

Could you include your fix in Ganymede ? Thanks
Comment 3 Randy Hudson CLA 2008-06-20 10:05:35 EDT
> Regarding changing the signature from float to double, the API will be backward
> compatible, so what is the problem ?

How is a signature change backwards compatible?  This would break both binary and source compatibility.

> Because even it fixes my current problem, there is still a potential problem
> with GMF,

I don't see the issue with the above fix. Weighting is just a hint about whether the bendpoint should stay closer to the source or target end of the connection.  It could be a number from 1-100, and things should still work about the same to the user, even if you had over 100 bendpoints.
Comment 4 Stéphane Lizeray CLA 2008-06-20 10:32:55 EDT
> How is a signature change backwards compatible?  This would break both binary
> and source compatibility.


I was speaking about source compatibility. It will be source compatible because there is an automatic casting from float to double.

Anyway, your are right on the other part of your comment. You change will fix all the use cases.

Thanks
Comment 5 Stéphane Lizeray CLA 2008-06-20 10:37:25 EDT
Let me know if you can include your fix in Ganymede? Thx
Comment 6 Anthony Hunter CLA 2008-06-20 10:57:07 EDT
(In reply to comment #5)
> Let me know if you can include your fix in Ganymede? Thx
> 

Ganymede is closed but I think we can put this in the maintenance release.
Comment 7 Romain Raugi CLA 2008-08-08 10:17:39 EDT
Created attachment 109526 [details]
Patch that contains Randy Hudson's fix.
Comment 8 Anthony Hunter CLA 2008-08-08 11:01:49 EDT
Thanks for the patch.

Stephane, can you confirm this latest patch works for you?

Any ideas on how we can add a JUnit test that demonstrates the failure?
Comment 9 Romain Raugi CLA 2008-08-11 09:11:09 EDT
This patch works for us. I will write a JUnit test to highlight the bug.
Comment 10 Romain Raugi CLA 2008-08-18 09:02:32 EDT
Created attachment 110220 [details]
Screenshots, before and after having applied the fix.
Comment 11 Romain Raugi CLA 2008-08-18 09:03:17 EDT
Created attachment 110221 [details]
Unit test to highlight the bug.
Comment 12 Anthony Hunter CLA 2008-09-08 11:25:08 EDT
Committed to R3_4_maintenance and HEAD