Created attachment 61533 [details]
Special handling for smoothing closed lines

Created attachment 61534 [details]
Example, how a closed line looks now

Created attachment 61535 [details]
Example how it would look with the attached patch

The attached patch looks fine but uncovers a different problem. I'll attach an
example to illustrate. Load the document 'Spline.ods' and double click the
chart. With the patch applied there is an invalid index access within
ViewContactOfE3dPolygon::createViewIndependentPrimitive3DSequence that leads to
a crash at least in non pro version.

Created attachment 61927 [details]
invalid index access after double click on chart

I'll create an additional patch that prevents the invalid index access within 
ViewContactOfE3dPolygon::createViewIndependentPrimitive3DSequence.
@aw, if you are ok with the changes in that method, I will commit them to one of
my next chart CWSses.
@regina, thanks for the additional off-line analysis! :-)

Created attachment 61928 [details]
patch to prevent invalid index access

AW: It is an error if E3dPolygonObj's GetPolyNormals3D() count or
GetPolyTexture2D() count is unequal to GetPolyPolygon3D(). This should
eventually be checked when settig data/creating that object. It needs to be a
1:1 relationship; it means that each 3D polygon point is directly associated
with a normal and a texture coordinate.
It would be better to fix that error if chart creates E3dPolygonObj's where this
1:1 relationship is broken. 'Guessing' which normal is meant to be at whcih
point as in the patch is from my point of view not a legal solution.

AW: For itself: It would be better to replace

			const bool bNormals(aPolyNormals3D.count());
			const bool bTexture(aPolyTexture2D.count());

with a more safe

			const bool bNormals(aPolyNormals3D.count() && aPolyNormals3D.count() ==
aPolyPolygon3D.count());
			const bool bTexture(aPolyTexture2D.count() && aPolyTexture2D.count() ==
aPolyPolygon3D.count());

AW: Oops.. I meant 'for safety itself'...

@aw, I will take your change in addition but it is not sufficient on its own. It
still crashes as in this case the main polygons are not different in size.
Instead the sub polygons are those that are different in size. They have to be
checked each in addition. But it is a good idea to introduce the same check also
for the textures. Furthermore it seems to me that some automatism ( see method
checkClosed ) remove some points from the polygons here and there thus they run
out of sync. Given that this is the main producer of this problem I think it is
a valid and good guess to take the very first point of the polygon if one is
missing in the end. I change the patch accordingly.

Created attachment 61930 [details]
2nd version of patch to prevent invalid index access

AW: With this version of the patch, the normals and texture coordiantes will not
be used at all, since the safe bNormals and bTexture will be false when the
count is not the same. Thus, the 2nd part of the fix is not needed anymore since
the parts using the normals and/or texture polygon will only be used when counts
are all equal.
I would suggest to take the 1st part (the changed definitions of bNormals and
bTexture), leave the 2nd part out (not needed anymore) and fix the source of the
problem: Some mechanism in chart creates E3dPolygonObj's with unequal counts of
points, normals and texture coordinates. This is not a valid thing to do.
HTH!

@aw, as I have explained already aPolyNormals3D.count() equals
aPolyPolygon3D.count() in this case. That is not the problem!
The problem is that aNormals3D.count() is not equal to aCandidate3D.count().
Those polygons can be created via API and they should not crash!

Furthermore the polygons that are set by the chart do have the proper sizes.
Points are removed by automatisms by the drawingengine code.

AW: Sorry, i missed that point. The condition for this object is simply that all
three involved polygons have to have the same internal layout, in the number of
polygons and the number of sub-polygons, too.
I am sorry that the object is constructable in the current form over the API,
but the reason for this is that it is an old object and also a special one only
used internally for chart. This is also the reason that there is not more code
in the object to ensure that conditions. Nowadays i would check and decline such
data sets.

So, the code should be something like>

			const bool bNormals(aPolyNormals3D.count() && aPolyNormals3D.count() ==
aPolyPolygon3D.count());
			const bool bTexture(aPolyTexture2D.count() && aPolyTexture2D.count() ==
aPolyPolygon3D.count());

			if(bNormals || bTexture)
			{
				for(sal_uInt32 a(0L); a < aPolyPolygon3D.count(); a++)
				{
					basegfx::B3DPolygon aCandidate3D(aPolyPolygon3D.getB3DPolygon(a));
					basegfx::B3DPolygon aNormals3D;
					basegfx::B2DPolygon aTexture2D;
					bool bNormalsSub(bNormals);
					bool bTextureSub(bTexture);
						
					if(bNormalsSub)
					{	
						aNormals3D = aPolyNormals3D.getB3DPolygon(a);
						bNormalsSub = aNormals3D.count() == aCandidate3D.count();
					}
					
					if(bTextureSub)
					{	
						aTexture2D = aPolyTexture2D.getB2DPolygon(a);
						bTextureSub = aTexture2D.count() == aCandidate3D.count();
					}

					for(sal_uInt32 b(0L); b < aCandidate3D.count(); b++)
					{
						if(bNormalsSub)
						{
							aCandidate3D.setNormal(b, aNormals3D.getB3DPoint(b));
						}
						
						if(bTextureSub)
						{
							aCandidate3D.setTextureCoordinate(b, aTexture2D.getB2DPoint(b));
						}
					}

					aPolyPolygon3D.setB3DPolygon(a, aCandidate3D);
				}
			}

Still, the error is in the data provider in the chart. The whole design of
E3dPolygonObj should make clear that it|s purpose was to have exactly the same
layout for all polygons and that the polygons for texture and normals are only
helper polygons for data transport. Today, a basegfx::B3DPolyPolygon which is
capable of holding all that data would be used. Since it is not, it IS necessary
to construct one from the old data distribution in three single polygons.

You may write me an issue to update the implementation of E3dPolygonObj to
fulfill all modern needs. I just can't say when i will be able to do this. We
should also update the API documentation (if this internal object is documented
at all) that normal and texture coordiante PolyPolygons will only be accepted
when their layout is equal to the base poygon (the one with the points).

AW:
> Points are removed by automatisms by the drawingengine code

?!? Please describe this closer. This should not happen since the API should
internally use the new basegfx polygons. If they still use the tools Polygons,
this may lead to a conversion error; When 1st and last points are equal in the
old polygon, it was a needed compromize to migrate to the new polygons (the old
ones had no closed state, but equal 1st and last points). To force the same for
all polygons, it should work to use the same conventions e.g. for the normal
polygon (1st and last normal the same, when points do this).

This is how points are removed by automatisms by the drawingengine code:
The normals polygon is set via the API with correct sizes. This leads to the call
Svx3DPolygonObject::setPropertyValueImpl - case
OWN_ATTR_3D_VALUE_NORMALSPOLYGON3D. Here the method
PolyPolygonShape3D_to_B3dPolyPolygon is called which itself calls
basegfx::tools::checkClosed(aNewPolygon);
-> polygons are out of sync.

AW: Loking at svx/source/unodraw/unoshape.cxx: All three cases
OWN_ATTR_3D_VALUE_POLYPOLYGON3D, OWN_ATTR_3D_VALUE_NORMALSPOLYGON3D and
OWN_ATTR_3D_VALUE_TEXTUREPOLYGON3D have identical implementations. If the
results are different, the source must have been different.

Are You sure the normals polygon mimics the point polygon in having an extra end
point equal to the start point when You are using a closed point polygon? Please
re-check.

I set the same amount of points to all polygon properties. Several normals
within the normals polygon seem to point into the same direction thus the
automatism removes them.

Function checkClosed(B3DPolygon& rCandidate) does remove an 'arbitrary' amount
of points depending on the concrete values of the points in the polygons.

AW: Ah, Yes! Da**ed! Normals are simply not points. Argh! As can be seen, a
compromize of older ages (transport normals as polygon, there was tooling for
this already) leads to growing problems in the future :-(

I have no quick idea how to fix this, but the error definitely happens in the
API implementation (Svx3DPolygonObject::setPropertyValueImpl). For
OWN_ATTR_3D_VALUE_NORMALSPOLYGON3D and for OWN_ATTR_3D_VALUE_TEXTUREPOLYGON3D
PolyPolygonShape3D_to_B3dPolyPolygon should not call
basegfx::tools::checkClosed; a flag at PolyPolygonShape3D_to_B3dPolyPolygon may
do that. In that case, the normal and/or texture polygon can never have less,
but maybe one more 'point' than the original (when the original was closed). The
safety decisions proposed in
ViewContactOfE3dPolygon::createViewIndependentPrimitive3DSequence would have to
be adapted to not only work on normal and/or texture data with the same, but
with the same or more (>= instead of ==) points.

We need to write a task for cleaning this up in the future. The API is simply
not good designed for the purpose here...

AW: Another idea: When there is a normals polygon, but less normals than points,
it's because double points were removed -> imply all non-existent normals to be
equal to the 1st normal (since double points are removed from the end of a polygon).
Congratulations IHA, this is exactly the fix You already proposed! You get all
my kudos :-) At least we know the reason now :-(
When we take Your fix (it's easier and better, i think), we should definitely
file a task to clean this up in the future ...

Thanks Armin! :-) I created Issue 101520 for further cleanup of the API
implementation. And will commit patch version 2 to CWS chart39. Thanks again for
the quick review and further analysis!

Fixed in CWS chart39.

@Sophie, please verify in CWS chart39. Thanks, Ingrid!
Load the attached document Spline.ods and double click the chart. Select 'Chart
Type...' in the contextmenu and click on the 'Lines only' Image to switch to 2D.
Compare the result with the attached pdf. The blue line should more look like
new_way.pdf and not like old_way.pdf.

Verified in CWS chart39 - The blue line looks like in new_way.pdf. Sophie

Verified in DEV300m54 under .deb version - Closing - Sophie