Skip to content

Commit

Permalink
svg-dom: fix a bug copying transforms [BATIK-1361]
Browse files Browse the repository at this point in the history
Test file adapted from SVG provided by Stanimir Stamenkov @stanio in BATIK-1361.
  • Loading branch information
carlosame committed Oct 11, 2023
1 parent 62574f8 commit 6158245
Show file tree
Hide file tree
Showing 8 changed files with 118 additions and 1 deletion.
Original file line number Diff line number Diff line change
Expand Up @@ -287,7 +287,7 @@ public void assign(SVGTransform transform) {
setScale(matrix.getA(), matrix.getD());
break;
case SVGTransform.SVG_TRANSFORM_ROTATE:
if (matrix.getE() == 0.0f) {
if (matrix.getE() == 0f && matrix.getF() == 0f) {

This comment has been minimized.

Copy link
@stanio

stanio Oct 11, 2023

This is still not quite right. In apache/xmlgraphics-batik#39 I've shown matrix.e == 0 && matrix.f == 0 with non-zero cx and cy at 0/360-deg angles. The new condition will set cx and cy to zero in these cases. While the rendered result is the same at such angles, the serialized form rotate(0) vs. rotate(0 50 50) could be considered wrong. For example, animate rotate(0 50 50) → rotate(100 50 50) is different from rotate(0 0 0) → rotate(100 50 50).

This comment has been minimized.

Copy link
@stanio

stanio Oct 11, 2023

To be honest, my first fix for the issue was the same. Then I ran the rotation matrices with various cx, cy combinations from 0 to 360-deg angles (with 0.1 step) to test the e and f attributes, and noticed it was not quite right. Dunno if it could cause a defect elsewhere.

This comment has been minimized.

Copy link
@carlosame

carlosame Oct 11, 2023

Author Member

The new condition will set cx and cy to zero in these cases. While the rendered result is the same at such angles, the serialized form rotate(0) vs. rotate(0 50 50) could be considered wrong. For example, animate rotate(0 50 50) → rotate(100 50 50) is different from rotate(0 0 0) → rotate(100 50 50).

But that code is dealing with a 'snapshot' of the transform at a certain time, so the cases you mention shouldn't matter. Do you have an SVG document where it actually causes a wrong rendering?

This comment has been minimized.

Copy link
@stanio

stanio Oct 11, 2023

No, I don't have an example of wrong rendering in this case. I've just pointed out it is not strictly correct, while it appears to work.

But that code is dealing with a 'snapshot' of the transform...

In this regard, I don't know why is this condition for setting a rotate-only result, and not just copying the source angle, x, y. Anyway, I'm not arguing about the necessary solution – this one is probably good enough.

This comment has been minimized.

Copy link
@carlosame

carlosame Oct 11, 2023

Author Member

No, I don't have an example of wrong rendering in this case. I've just pointed out it is not strictly correct, while it appears to work.

It is perfectly correct.

In this regard, I don't know why is this condition for setting a rotate-only result, and not just copying the source angle, x, y.

It's all about setting angleOnly. With e and f being zero it is a plain rotation, so the check for e and f is the natural thing to do (and what my patch does).

rotate(transform.getAngle());
} else {
angleOnly = false;
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,54 @@
/*
See the NOTICE file distributed with this work for additional
information regarding copyright ownership.
Licensed under the Apache License, Version 2.0 (the "License");
you may not use this file except in compliance with the License.
You may obtain a copy of the License at
http://www.apache.org/licenses/LICENSE-2.0
Unless required by applicable law or agreed to in writing, software
distributed under the License is distributed on an "AS IS" BASIS,
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
See the License for the specific language governing permissions and
limitations under the License.
*/
package io.sf.carte.echosvg.test.svg;

import io.sf.carte.echosvg.transcoder.SVGAbstractTranscoder;
import io.sf.carte.echosvg.transcoder.image.ImageTranscoder;

/**
* Checks for regressions in rendering of a document with animations.
*
* @author For later modifications, see Git history.
* @version $Id$
*/
public class SVGAnimationRenderingAccuracyTest extends RenderingTest {

private float time;

public SVGAnimationRenderingAccuracyTest(float time) {
super();
this.time = time;
}

@Override
protected String getImageSuffix() {
return (time == 0f) ? "" : "-t" + Float.toString(time);
}

/**
* Returns the <code>ImageTranscoder</code> the Test should use
*/
@Override
ImageTranscoder getTestImageTranscoder() {
ImageTranscoder t = super.getTestImageTranscoder();
t.addTranscodingHint(SVGAbstractTranscoder.KEY_SNAPSHOT_TIME, time);
return t;
}

}
Original file line number Diff line number Diff line change
Expand Up @@ -320,6 +320,12 @@ public void testSystemColors() throws TranscoderException, IOException {
/*
* Coordinates
*/
@Test
public void testAnimRotateOrigin() throws TranscoderException, IOException {
float[] times = {0.5f, 1.5f, 1.52f, 3.5f};
testAnim("samples/tests/spec/coordinates/animRotateOrigin.svg", times);
}

@Test
public void testPercentagesAndUnits() throws TranscoderException, IOException {
test("samples/tests/spec/coordinates/percentagesAndUnits.svg");
Expand Down Expand Up @@ -1710,6 +1716,14 @@ private void testResolutionPxMM(String file, float pxToMM)
runner.runTest(0.000001f, 0.000001f);
}

private void testAnim(String file, float[] times) throws TranscoderException, IOException {
for (float time : times) {
RenderingTest runner = new SVGAnimationRenderingAccuracyTest(time);
runner.setFile(file);
runner.runTest(0.000001f, 0.000001f);
}
}

/**
* Dynamic test of the rendering of a SVG file.
*
Expand Down
49 changes: 49 additions & 0 deletions samples/tests/spec/coordinates/animRotateOrigin.svg
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.

0 comments on commit 6158245

Please sign in to comment.