Animation of java.awt.Polygons

Question

I made a class capable of moving Polygons around on a Polygon in a mostly smooth manner. Disclaimer: this was part of a school project (thus the use of Polygon rather than Image), but this code is entirely outside of the scope of the assignment, and the assignment has already been graded.

The project was a Bejeweled clone, so the Animator is capable of the two animations needed for that clone: it can take two ColoredPolygons (a simple wrapper around a Color and a Polygon) and swap their position, or a List of ColoredPolygons and translate them all some dx/dy. A built in assumption in this animation is that only one animation will be active at a time, and that the area covered by the animation only contains the Polygons that were passed in.

package cad97.bejeweled.animation;

import javax.swing.*;
import java.awt.*;
import java.awt.geom.Area;
import java.util.List;

/**
 * Animate Polygons around on a Graphics object.
 *
 * @author Christopher Durham
 * @date 11/25/2016
 */
public final class Animator {
    public static final Animator INSTANCE = new Animator();

    private final static int ANIMATION_FRAMES = 30;
    private final static int ANIMATION_FPS = 60;
    private final static int ANIMATION_INTERVAL = 1000 / ANIMATION_FPS;

    /**
     * Convenience class bundling a {@link Color} and a {@link Polygon}
     */
    public static class ColoredPolygon {
        final Color color;
        final Polygon polygon;

        public ColoredPolygon(Color color, Polygon polygon) {
            this.color = color;
            this.polygon = polygon;
        }
    }

    /**
     * Animate the swap of two Polygons on a Graphics object.
     * <p>
     * The parameter Polygons are mutated during the execution of this method.
     * <p>
     * A redraw of the animated area in the {@code after} callback is suggested,
     * as the final frame of this animation is not guaranteed to be fully finished.
     * (E.g. the frame before a fully finished position.)
     *
     * @param bgColor the background Color used to erase frames
     * @param first   the first ColoredPolygon being switched
     * @param second  the second ColoredPolygon being switched
     * @param g       the Graphics object to draw on
     * @param after   callback run on UI thread after animation is finished
     */
    public void translateSwap(final Color bgColor, final ColoredPolygon first, final ColoredPolygon second,
                              final Graphics g, final Runnable after) {
        new SwingWorker<Void, Void>() {
            @Override
            protected Void doInBackground() throws Exception {
                final Rectangle b1 = first.polygon.getBounds();
                final Rectangle b2 = second.polygon.getBounds();
                final int deltaX = (b1.x - b2.x) / ANIMATION_FRAMES;
                final int deltaY = (b1.y - b2.y) / ANIMATION_FRAMES;
                final Rectangle animationBounds = b1.union(b2);

                for (int i = 0; i < ANIMATION_FRAMES; i++) {
                    first.polygon.translate(-deltaX, -deltaY);
                    second.polygon.translate(deltaX, deltaY);
                    SwingUtilities.invokeAndWait(() -> {
                        g.setColor(bgColor);
                        g.fillRect(animationBounds.x, animationBounds.y, animationBounds.width, animationBounds.height);
                        g.setColor(first.color);
                        g.fillPolygon(first.polygon);
                        g.setColor(second.color);
                        g.fillPolygon(second.polygon);
                    });
                    Thread.sleep(ANIMATION_INTERVAL);
                }
                SwingUtilities.invokeLater(after);
                return null;
            }
        }.execute();
    }

    /**
     * Translate a group of Polygons a direction on a Graphics object.
     * <p>
     * The passed Polygons are mutated during the execution of this method.
     * <p>
     * A redraw of the animated area in the {@code after} callback is suggested,
     * as the final frame of this animation is not guaranteed to be fully finished.
     * (E.g. the frame before a fully finished position.)
     *
     * @param bgColor  the background Color used to erase frames
     * @param polygons a list of ColoredPolygons to translate
     * @param dx       the delta x to translate the polygons
     * @param dy       the delta y to translate the polygons
     * @param g        the Graphics object to draw on
     * @param after    callback run on UI thread after animation is finished
     */
    public void batchTranslate(final Color bgColor, final List<ColoredPolygon> polygons,
                               final int dx, final int dy, final Graphics2D g, final Runnable after) {
        new SwingWorker<Void, Void>() {
            @Override
            protected Void doInBackground() throws Exception {
                final Area animationBounds = polygons.stream().sequential()
                        .map(it -> it.polygon).map(Polygon::getBounds).peek(it -> {
                            it.grow(dx / 2, dy / 2);
                            it.translate(dx / 2, dy / 2);
                        }).map(Area::new).reduce((lhs, rhs) -> {
                            if (lhs == null) return rhs;
                            rhs.add(lhs);
                            return rhs;
                        })
                        .orElseThrow(AssertionError::new);
                final int deltaX = dx / ANIMATION_FRAMES;
                final int deltaY = dy / ANIMATION_FRAMES;

                for (int i = 0; i < ANIMATION_FRAMES; i++) {
                    polygons.forEach(it -> it.polygon.translate(deltaX, deltaY));
                    SwingUtilities.invokeAndWait(() -> {
                        g.setColor(bgColor);
                        g.fill(animationBounds);
                        polygons.forEach(it -> {
                            g.setColor(it.color);
                            g.fill(it.polygon);
                        });
                    });
                    Thread.sleep(ANIMATION_INTERVAL);
                }
                SwingUtilities.invokeLater(after);
                return null;
            }
        }.execute();
    }
}

I've never done actual animation before, so this is likely a hacky solution that goes against best practices. This is within a Swing context, so any suggestions using the Swing library and not just AWT are welcome.

Here is a minimal harness for this code on Gist. This is not part of the review, merely a way to enable ease of testing. In the creation of this harness, I noticed a bug on line 102 of this class, the fix to which is commented in the Gist.

Answer 1

Conceptual Problems

You have hard coded the animation FPS as ANIMATION_FPS = 60. This is a big no-no! You are assuming that your code will be called exactly with exactly 1000/ANIMATION_FPS = 16.7 ms intervals. But you are using Thread.sleep(long) which is:

subject to the precision and accuracy of system timers and schedulers.

Remember that on Windows the scheduling time slice is on the order of 10 ms (see here where a Vista system has 15ms). So this means that your calling accuracy on the animation is about the same order of magnitude as your time step, this is bad news! Your animation will either be jittery or time slide. This is not Windows specific, all OSes have some manner of time slice, some may have dynamic ones. They all just differ in how long these slices are but in general they are on the milli second scale. Even if the OS sleep was accurate, your way of doing it with a fixed sleep would break down if the CPU was busy and couldn't keep up with your paint in a fast enough manner causing a time dilation in your animation.

What you really should do is to for each call to your painting algorithm figure out the time since the last call and multiply this by the speed you want your figure to move/rotate whatever.

Pseudocode:

void paint(float deltaTimeSeconds){
    int xPos = oldXPos + velocityX*deltaTimeSeconds;
    drawAt(XPos);
}

If you don't do it in the way described above you may encounter problems such as your animation feeling jittery or that the animation "slides" and doesn't maintain the correct speed on the screen.

Useless threading

You are doing the paint computations in a background thread and then signalling the result to the main thread at an interval. To me this looks like unnecessary work. I believe that you will have better performance and less trouble by simply doing the drawing in the application thread.

Yes I realise that you use the background thread to trigger the draw because you can't trigger it from the UI thread. But there is a better way of doing this, simply use javax.swing.Timer.

From the documentation:

An example use is an animation object that uses a Timer as the trigger for drawing its frames.

Also interesting:

Although all Timers perform their waiting using a single, shared thread (created by the first Timer object that executes), the action event handlers for Timers execute on another thread -- the event-dispatching thread. This means that the action handlers for Timers can safely perform operations on Swing components. However, it also means that the handlers must execute quickly to keep the GUI responsive.

Example implementation in AnimationPanel (some assembly required):

long lastFrameNs;
boolean animationCompleted = false;

Timer animationTimer = new Timer(ANIMATION_INTERVAL, event -> {
    long currentFrameNs = System.currentNano();
    long deltaNs = currentFrameNs - lastFrameNs;
    lastFrameNs = currentFrameNs;
    float deltaS = deltaNs /1E9f;

    // Update polygon positions here using `deltaS` 

    // Detect when the animation finished....
    if(done){
        animationCompleted = true;
    }

    // Tell SWING that the component is dirty and to repaint it
    // Bonus: Figure out the affected `Rectangle` and pass that into 
    // repaint to avoid repainting the whole shebang. 
    repaint();


}); // Repeats by default

// Add method to trigger the animation and parameters
void animateSwap(Jewel A, Jewel B, ...){
    // Setup necessary data here...

    animationCompleted = false;
    lastFrameNs = System.currentNano();
    animationTimer.start();
}

@Override
void paint(Graphics g){
    // Draw all the jewels at their positions here...

    if(animationCompleted){
        animationTimer.stop();
        animationCompleted = false;
    }
}

At this point I feel that further review of the code is pointless from my part as the above already highlights problems that would mandate a rewrite that would make further review obsolete.

Answer 2

Use the right tool for the Job - Timer instead of SwingWorker

Using the right tool for the job will significantly simplify your task and make it less error-prone. Currently, you are using javax.swing.SwingWorker to "run" the animation. SwingWorker is not the right tool for the job. javax.swing.Timer is.

• SwingWorker is for running lengthy background tasks, such as loading big files, which are to be performed in the background and then update the UI in irregular intervals.
• Timer is for regularly triggering an action on the event thread, which is quite good for simple animations.

As a rule of thumb, calling Thread.sleep() should always make you think twice. Avoid Thread.sleep(). The problems with sleep() have already been mentioned in the other review.

You can achieve smoother animations by using Timer. When you use SwingWorker, the graphical update will happen asynchronously, even if you use invokeAndWait(), because the update request happens on a different thread than the AWT event thread. The amount of time it takes until your request to run something on the AWT event thread gets through is indeterministic.

If you use Timer, you are on the AWT event thread already, and you can calculate based on the start system time and the current system time the exact positioning for the current animation frame.

Good stuff

• Frequent use of final
• Use of Javadoc

Fix the bug in the animation

When I play the animation on my machine, some of the animation leaves trails and isn't smooth until the repaint. I did not dig into the bounds calculations, but there might be something wrong with them.

DRY-out translateSwap and batchTranslate

These two methods are very similar. You could probably extract large parts of it into a separate method. Or even cover everything in one method by passing in the difference as strategy (see strategy design pattern; you're already using it, the last argument to translateSwap and batchTranslate is an example for the strategy design pattern).

Consider double-buffering

In order to achieve smoother animations you might want to consider double-buffering. Currently you directly paint on-screen using the Graphics of the JPanel. This means that the user sees a flickering animation, first the background, then the reappearing polygons.

https://docs.oracle.com/javase/tutorial/extra/fullscreen/doublebuf.html

Maybe avoid getGraphics()

I've never needed to call getGraphics() of a component. I always managed to do whatever needs to be done in the paint() or paintComponent() methods. I find it fishy and fragile to use Graphics outside of the context in which it was provided. For example, if later you add an optional full screen mode to the application, and add an option for the user to switch between full screen mode and windowed mode, I would assume that whenever that switch happens, the Graphics object becomes invalid.

Use JLS modifier order

It's quicker to read Java source code if modifers are sorted according to their appearance in the Java Language Specification. In this case, it means it should be private static final instead of private final static for some of your constants.

Style: side effects in streams?

Streams are an element of functional programming. At the heart of functional programming is the avoidance of side effects. It feels weird seeing a side effect in peek() manipulating the bounds of the rectangle. While this is technically possible and in your situation technically correct code, it feels like a trap. I would rather use a map() returning the same object after manipulation:

• It's obvious that after map() the stream contains something else.
• It's unexpected that after peek() the stream contains something else.

Write shorter methods

Code is usually easier to understand if methods are short. I recommend no more than 3 lines per method in Java for the majority of methods.