Animation of java.awt.Polygons

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.
*
* The parameter Polygons are mutated during the execution of this method.
*
* 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.)
*
* @par

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.

StackExchange Code Review Q#148936, answer score: 5