Issue 115917 - tools: bezier curve drawing and smoothness
tools: bezier curve drawing and smoothness
Status: RESOLVED FIXED
Product: General
Classification: Code
Component: code
OOo 1.0.0
Mac All
: P3 trivial (vote)
: 4.0.0
Assigned To: Armin Le Grand
issues@framework
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2010-12-04 11:13 UTC by osnola
Modified: 2013-07-12 16:09 UTC (History)
1 user (show)

See Also:
Issue Type: PATCH
Latest Confirmation on: ---
Developer Difficulty: ---


Attachments
the proposed patch (1.40 KB, text/plain)
2010-12-04 11:14 UTC, osnola
no flags Details
the new Image280's result after applying this patch (81.91 KB, text/plain)
2010-12-04 11:17 UTC, osnola
no flags Details
Better precision in correction (3.72 KB, patch)
2012-07-27 16:36 UTC, Armin Le Grand
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this issue.
Description osnola 2010-12-04 11:13:34 UTC
To correct the smoothness of a bezier curve, the function impCorrectContinuity uses the actual point and the previous control point in order to define the position of the next 
control point, even if the vector between these two points is much smallest than the vector between the actual point and the next control point.

This can lead to some odd drawing like in http://www.openoffice.org/nonav/issues/showattachment.cgi/75177/Image280.zoom.png ( see the last messages at the end of issue: 
http://www.openoffice.org/issues/show_bug.cgi?id=115096 ).

Therefore, just a small proposition which first checks what is the longest vector and according to the result modifies the previous control point or the next control point.
Comment 1 osnola 2010-12-04 11:14:14 UTC
Created attachment 75236 [details]
the proposed patch
Comment 2 osnola 2010-12-04 11:17:37 UTC
Created attachment 75237 [details]
the new Image280's result after applying this patch
Comment 3 carsten.driesner 2010-12-06 08:43:42 UTC
cd->aw: Please take over. I am not familiar with the basegfx library.
Comment 4 Armin Le Grand 2012-07-27 15:30:03 UTC
ALG: Good catch. impCorrectContinuity only corrects the numerical error in the conversion from integer to double precision polygon, thus only errors in the range of 0.0 <= error <= sqrt(2.0) can happen when the one vector is offset on integer on +-1 in X and/or y.
Choosing the longer (originally integer) vector bets on the fact that it might have been snapped better to integer than the shorter one. An even better correction would be to take the mediated direction for C1 correction and the mediated control points for C2 correction.
Checking how to do that...
Comment 5 Armin Le Grand 2012-07-27 15:38:33 UTC
ALG: Thought again. For C1, the longest vector will have the best preservation of the original direction, thus the patch is very correct. For C2 it might be best to take the mediated result as vectors.
Comment 6 Armin Le Grand 2012-07-27 16:25:18 UTC
ALG: And again: For C1, mediating direction weighted by length may be even better, this gives vector addition to get the common direction.
This leads to unification of calculating the common direction for C1 and C2 cases for correction. Proposed code:

            // extract the point and vectors
			const basegfx::B2DPoint aPoint(roPolygon.getB2DPoint(nIndex));
            const basegfx::B2DVector aNext(roPolygon.getNextControlPoint(nIndex) - aPoint);
            const basegfx::B2DVector aPrev(aPoint - roPolygon.getPrevControlPoint(nIndex));

            // calculate common direction vector, normalize
            basegfx::B2DVector aDirection(aNext + aPrev);
            aDirection.normalize();

			if(POLY_SMOOTH == nCFlag)
			{
                // C1: use common direction vector, preserve individual lengths
				roPolygon.setNextControlPoint(nIndex, basegfx::B2DPoint(aPoint + (aDirection * aNext.getLength())));
				roPolygon.setPrevControlPoint(nIndex, basegfx::B2DPoint(aPoint - (aDirection * aPrev.getLength())));
			}
			else // POLY_SYMMTR
			{
                // C2: get mediated length. Taking half of the unnormalized direction would be
                // an approximation, but not correct. Create common vector in direction next.
                const double fMedLength((aNext.getLength() + aPrev.getLength()) * 0.5);
                const basegfx::B2DVector aCommonNext(aDirection * fMedLength);

                // use common vector
				roPolygon.setNextControlPoint(nIndex, basegfx::B2DPoint(aPoint + aCommonNext));
				roPolygon.setPrevControlPoint(nIndex, basegfx::B2DPoint(aPoint - aCommonNext));
			}
Comment 7 Armin Le Grand 2012-07-27 16:36:16 UTC
Created attachment 78761 [details]
Better precision in correction

ALG: OOps, adding as comment was not too good, adding as proposed. patch.

@osnola: What is your opinion? Comments welcome (and thans for digging this out).
Comment 8 osnola 2012-07-28 08:11:24 UTC
Hello ALG,
yes, using the mediating direction seems a better idea, ie. if a vector length is
much bigger than the other one, the result will almost correspond to my patch ;
if not, the previous conversion errors may sometimes cancel and sometimes not 
( but as we do not have enough informations to do a better correction, this seems
the best that we can do ).

    osnola.
Comment 9 SVN Robot 2012-07-30 10:41:08 UTC
"alg" committed SVN revision 1367054 into trunk:
#115917# Better conversion of C1 and C2 bezier curve pointsPatch by: osnolaRe...
Comment 10 Armin Le Grand 2012-07-30 10:42:05 UTC
ALG: Hi osnola, thanks for your comments. I will add the changes as described. Comitted as revision 1367054.
Comment 11 Armin Le Grand 2012-07-30 10:54:12 UTC
ALG: Hi osnola, one more question: Do you still have the original steps/files to reproduce the error? If so, could you please attach them to the task? Thanks!
Comment 12 Armin Le Grand 2012-07-30 10:54:35 UTC
ALG: Corrected state
Comment 13 osnola 2012-07-30 11:19:20 UTC
(In reply to comment #11)
> ALG: Hi osnola, one more question: Do you still have the original
> steps/files to reproduce the error? If so, could you please attach them to
> the task? Thanks!

Hello ALG,
- if you download https://issues.apache.org/ooo/attachment.cgi?id=75160 ( in https://issues.apache.org/ooo/show_bug.cgi?id=115096 ), you will obtain two files : Image7239.pict and Image280.pict,
- then you need to open a text document and insert these images in these document.

Normally, you need to obtain an ellipse around the vertices B and J and two arcs of circle around O ( as in Image7239.pict.old.png and in Image280.pict.old.png ) ; if this bug is triggered, you will obtain two nasty ellipses and a nasty small arc of circle as in Image7239.pict.new.png and in Image280.pict.new.png .

Note: 
- the name are files are weird, because in https://issues.apache.org/ooo/attachment.cgi?id=75160 , I first present a patch where I created the arcs/circles by hand. After we modify it to use a function which is already present in OpenOffice ( and which indirectly triggered this bug ).
- as I have a very old version of OpenOffice, I am downloading the 3.4.0 version to check that these two pictures continue to trigger these bugs or not ( but it downloads very very slowly :-~ ).
Comment 14 osnola 2012-07-30 12:41:25 UTC
Just a small message to confirm that the two previous pictures allow to reproduce the error with OpenOffice 3.4.0...
Comment 15 Armin Le Grand 2012-07-31 12:26:04 UTC
ALG: Thanks, got and checked.