JavaFX Smoke Effect (Particle Generator algorithm)

Question

I wrote a "Smoke" effect that I thought I'd post for code review. It's a decorative effect for a puzzle game I'm working on--there will be four of these, animated independently, probably with a purplish tint. I'm mostly interested in feedback on the "Smoke" class, not so much the little JavaFX scene that displays it.

There are some variables (xThrow, xWobble, yThrow, yWobble) that can be used to vary how the "incense pot" smoke behaves, which will be a function of how close the player gets to a puzzle solution. I didn't include the getter or setter methods in this sample.

I think that having a single static WritableImage, and multiple ImageViews that all point to it is a good solution. But each ImageView has an opacity value that will have to be applied as part of the render. Would it be better to make 50 WritableImages each with the opacity already coded into the image data, so that the ImageView's opacity could just be the default of 1?

I was wondering about opportunities to use Streaming, for example, in the update() method. But maybe the arrays are probably too small to justify this? Even if I did try doing it (converting ImageView[] to ArrayList and using .stream() and maybe .forEach()), I'm curious about how one would reference the "next" item in an array from the context of a stream.

With each update(), X and Y values are moved one position down through the array (from end to beginning) as the smoke particle rises and disappears.

I am thinking maybe the better way to take advantage of multiprocessing will be to update these on a separate thread (the splitting up of chores being handled in the animator--there will be lots of little things being updated to divvy up), and not worry about streaming in this case.

Lastly, I haven't put up anything for code reading in a long time. So the "don't know what I don't know" factor applies--I'm curious if there are helpful suggestions of the unanticipated kind.

import java.util.Random;

import javafx.animation.AnimationTimer;
import javafx.application.Application;
import javafx.geometry.Point2D;
import javafx.scene.Group;
import javafx.scene.Scene;
import javafx.scene.image.ImageView;
import javafx.scene.image.PixelWriter;
import javafx.scene.image.WritableImage;
import javafx.scene.paint.Color;
import javafx.scene.paint.CycleMethod;
import javafx.scene.paint.LinearGradient;
import javafx.scene.paint.Stop;
import javafx.scene.shape.Ellipse;
import javafx.scene.shape.Rectangle;
import javafx.stage.Stage;

public class ShowSmoke extends Application 
{
    public final static int SCENE_WIDTH = 400;
    public final static int SCENE_HEIGHT = 300;

    public static void main(String[] args) 
    {
        launch(args);
    }

    @Override
    public void start(Stage primaryStage) throws Exception 
    {
        Group root = new Group();
        Scene scene = new Scene(root, SCENE_WIDTH, SCENE_HEIGHT);
        scene.setFill(Color.BLACK);

        Rectangle landscape = new Rectangle(0, 200, SCENE_WIDTH, SCENE_HEIGHT - 200);

        Stop[] stops = new Stop[] { 
                new Stop( 0, new Color(0.95, 0.95, 0.5, 1)),
                new Stop( 1, new Color(0.1, 0.1, 0, 1)) 
            };      
        landscape.setFill(new LinearGradient(
                0, 1, 0, 0, true, CycleMethod.NO_CYCLE, stops));

        root.getChildren().add(landscape);

        Ellipse potBot = new Ellipse();
        potBot.setCenterX(200);
        potBot.setCenterY(262);
        potBot.setRadiusX(9);
        potBot.setRadiusY(5);
        potBot.setFill(Color.BROWN);

        Ellipse potTop = new Ellipse();
        potTop.setCenterX(200);
        potTop.setCenterY(260);
        potTop.setRadiusX(6);
        potTop.setRadiusY(2);
        potTop.setFill(Color.BLACK);

        root.getChildren().addAll(potBot, potTop);

        Smoke smoke = new Smoke(180, 221, Color.WHITESMOKE);
        smoke.setOpacity(1);

        root.getChildren().add(smoke);

        primaryStage.setScene(scene);
        primaryStage.show();

        AnimationTimer gameloop = new AnimationTimer() 
        {
            @Override
            public void handle(long arg0) 
            {
                smoke.update();
            }
        };
        gameloop.start();       
    }   
}

class Smoke extends Group
{
    private static WritableImage baseImg;
    private final ImageView[] lifeCycle;

    private final int FRAMES = 50;
    private final int FALLING_FRAMES = 32;
    private final int width = 40;
    private final int height = 40;
    private final double radius;
    private final Point2D midPoint;

    private double xThrow = -2 ; 
    private int xWobble = 5;
    private double yThrow = -2.2;
    private int yWobble = 2;

    private Random random;
    private int throttle;
    private int throttleMax;

    public Smoke(int xLoc, int yLoc, Color color)
    {   
        throttleMax = 4; // 4 ==> 15 fps animation rate
        random = new Random();

        midPoint = new Point2D(width/2, height/2);
        radius = width/2 - 1;
        lifeCycle = new ImageView[50];

        double topAlpha = 0.5; 
        double fadeAmount = topAlpha/FALLING_FRAMES;
        double fadeInAmount = 
                topAlpha/((FRAMES - FALLING_FRAMES) * 3);

        double pixR = color.getRed();
        double pixG = color.getGreen();
        double pixB = color.getBlue();
        double pixA = topAlpha;

        // Create the base image. 
        // The color is constant, the alpha is a
        // computed radial gradient.
        baseImg = new WritableImage(width, height); 
        PixelWriter baseRaster = baseImg.getPixelWriter();
        double distance;
        double gradientFactor; // between 0 and 1

        for (int jj = 0; jj < height; jj++)
        {
            for (int kk = 0; kk < width; kk++)
            {
                distance = midPoint.distance(jj, kk);
                if (distance > radius) gradientFactor = 0.0;
                else gradientFactor = (radius - distance) / radius;

                pixA =  topAlpha * gradientFactor;
                baseRaster.setColor(jj, kk, new Color(pixR, pixG, pixB, pixA));
            }   
        }   

        for (int i = 0; i < FRAMES; ++i)
        {
            lifeCycle[i]= new ImageView();
            lifeCycle[i].setImage(baseImg);
            lifeCycle[i].setX(xLoc);
            lifeCycle[i].setY(yLoc);
            this.getChildren().add(lifeCycle[i]);
        }

        // alpha will rise and fall over course of lifeCycle
        for (int i = 0; i < FALLING_FRAMES; ++i)
        {
            lifeCycle[i].setOpacity((i + 1) * fadeAmount);
        }

        for (int i = FALLING_FRAMES; i < FRAMES; i++)
        {
            lifeCycle[i].setOpacity(
                    topAlpha - (i - FALLING_FRAMES + 1) * fadeInAmount);
        }
    }

    public void update() 
    { 
        // Slow the animation by skipping cycles. 60 fps is overkill.
        if (throttle-- > 0) return;
        else throttle = throttleMax;

        // Propagate location of particles, while adding 
        //     'throw' and 'wobble'
        // lifeCycle[n] is stationary (origin of stream, visually)
        int n = FRAMES - 1; 
        for (int i = 0; i < n; i++) 
        {
            lifeCycle[i].setX( lifeCycle[i+1].getX() 
                + xThrow + random.nextInt(xWobble));
            lifeCycle[i].setY( lifeCycle[i+1].getY() 
                + yThrow + random.nextInt(yWobble)); 
        }       
    }
}

Answer 1

Your constructor is one big lump of a method, which is hard to read. Extract some methods. like:

public Smoke(Point location, Color color){   
  baseImg = initBaseImage(new WritableImage(width, height), color);
  lifeCycle = initLifeCycle(new ImageView[50], baseImg, location);
  //[..]

Also note that I suggest using a Point as a location. It may seem wasteful, but I find it's more expressive than just two integers.

in your initLoop I would not use if/else without brackets like so:

 if (distance > radius) gradientFactor = 0.0;
 else gradientFactor = (radius - distance) / radius;

I would either use curly braces {} or I would use the ternary operator:

 gradientFactor = (distance > radius) ? 0.0 : (radius - distance) / radius;

Or use Math.max, as a more formal notation, to express the fact that the gradientFactor is no less than 0, which I personally find most appealing:

 gradientFactor = Math.max(0.0, (radius - distance) / radius);

I also would change your variables on the nested for loop, to express the fact, you're working on a coordinate system, and I would declare pixA inside the loop, as it is not used outside of it:

 for (int y= 0; y<height; ++y){
   for (int x= 0; x<width; ++x){
   //[..]
     double pixA = topAlpha * gradientFactor;
   }
 }

Same with "distance", "pixR", "pixG" and "pixB". The JIT-compiler will easily inline the methods and optimize the code to move these outside the loop again, as it can assume the values are not changing (the rgbValue in the Color class is just set in the constructors, it's effectively final). But even if the compiler wouldn't, the time penalty is so low, that code clarity trumps speed. If a variable's scope gets too broad, a method becomes harder to read and harder to reason about, I for example, first had to look, if the values were used after the loop to reason about what they were used for, if the loops were to make a meaningful change to them for later usage, or if they'd just use them internally. Always use a very small variable scope, if possible.

I hope my input was valuable for you. Kind regards.