Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I just started using Perlin noise a few days ago, and the results look quite good. But it takes over 3 seconds to calculate and draw a 1024x1024 bitmap of said noise. I use an array of 1024x1024 to store int32s ranging from -1 to 1.

Please note that the variable tileSize equals 1024 / resolutieX. This is in case I want to use a lower resolution Perlin noise.

 private void button1_Click(object sender, EventArgs e)
    {
        sw.Start();
        LibNoise.Perlin perlinMap = new LibNoise.Perlin();
        perlinMap.Lacunarity = lacunarity + 0.01d;
        perlinMap.NoiseQuality = LibNoise.NoiseQuality.High;
        perlinMap.OctaveCount = octaveCount;
        perlinMap.Persistence = persistence;
        perlinMap.Frequency = frequency;
        perlinMap.Seed = 1024;

        if (radioButton1.Checked)
            perlinMap.NoiseQuality = LibNoise.NoiseQuality.Low;
        else if (radioButton2.Checked)
            perlinMap.NoiseQuality = LibNoise.NoiseQuality.Standard;
        else if (radioButton3.Checked)
            perlinMap.NoiseQuality = LibNoise.NoiseQuality.High;

        double sample = trackBar6.Value * 10;

        double[,] perlinArray = new double[resolutieX, resolutieY];
        for (int x = 0; x < resolutieX; x++)
        {
            for (int y = 0; y < resolutieY; y++)
            {
                perlinArray[x, y] = perlinMap.GetValue(x / sample, y / sample, 1d);
            }
        }
        draw(perlinArray);
        textBox12.Text = sw.ElapsedMilliseconds.ToString() + "ms";
        sw.Reset();
    }



public void draw(double[,] array)
    {
        Bitmap afbeelding = new Bitmap(1024, 1024);
        Color color;
        int tileSize = 1024 / resolutieY;
        for (int y = 1; y < resolutieY; y++)
        {
            for (int x = 1; x < resolutieX; x++)
            {
                if (array[x, y] <= -0.2)
                    color = Color.DarkBlue;
                if (array[x, y] <= 0)
                    color = Color.DarkBlue;
                else if (array[x, y] <= 0.1)
                    color = Color.Blue;
                else if (array[x, y] <= 0.2)
                    color = Color.Beige;
                else if (array[x, y] <= 0.22)
                    color = Color.LightGreen;
                else if (array[x, y] <= 0.40)
                    color = Color.Green;
                else if (array[x, y] <= 0.75)
                    color = Color.DarkGreen;
                else if (array[x, y] <= 0.8)
                    color = Color.LightSlateGray;
                else if (array[x, y] <= 0.9)
                    color = Color.Gray;
                else if (array[x, y] <= 1)
                    color = Color.DarkSlateGray;
                else
                    color = Color.DarkSlateGray;

                //  color = Color.FromArgb(255);

                for (int i = 0; i < tileSize; i++)
                {
                    for (int j = 0; j < tileSize; j++)
                    {
                        afbeelding.SetPixel(((x - 1) * tileSize) + i, ((y - 1) * tileSize) + j, color);
                    }
                }

            }
        }

        pictureBox1.Image = afbeelding;
    }

Should I start using Simplex noise? Or am I just missing something obvious here?

share|improve this question
1  
The code you posted isn't a perlin noise implementation; it just converts some values in array to colors and fills an image with them. If you want us to review the actual noise function, you need to post it. –  Colonel Thirty Two Jun 12 at 14:39
1  
You're using a perlin noise library so we can't do anything about that. Have you profiled if it is your draw function or the perlin noise generation that takes time? –  Emily L. Jun 12 at 14:44
2  
I am not a C# expert by any means, but I have done this sort of thing many times in Windows applications. Setting a pixel can be relatively expensive, so its possible that's your problem. If so, consider setting the entire bitmap's pixels in a single write. There should be a way to write to the bitmap data directly. –  GrandmasterB Jun 12 at 16:07

2 Answers 2

Your check

if (array[x, y] <= -0.2)
    color = Color.DarkBlue;

serves no purpose, if your number is indeed lessthan or equal -.2 then it will also hit the next check if it is lessthan or equal to 0.


A prettier way to write if-else blocks like yours would have been to use the ternary operator...

color = array[x, y] <= 0.0  ? Color.DarkBlue       :
        array[x, y] <= 0.1  ? Color.Blue           :
        array[x, y] <= 0.2  ? Color.Beige          :
        array[x, y] <= 0.22 ? Color.LightGreen     :
        array[x, y] <= 0.40 ? Color.Green          :
        array[x, y] <= 0.75 ? Color.DarkGreen      :
        array[x, y] <= 0.8  ? Color.LightSlateGray :
        array[x, y] <= 0.9  ? Color.Gray           :
        array[x, y] <= 1.0  ? Color.DarkSlateGray  :
                              Color.DarkSlateGray  ;

Your method draw is a public method, and should be PascalCase not camelCase


Your worst case scenario for the if-else chain is that you have to hit every single one of those IFs. If you structured those in binary search design, ie. start from the middle and eliminate half of the spectrum at a time and work your way out, worst case scenario is that you have to hit half of your IFs. The code for this may look ugly, but it would help.


The most inefficient thing about your code though, and the reason it is taking so long to process is most likely your quadruple nested for-loop

for (int y = 1; y < resolutieY; y++)
{
    for (int x = 1; x < resolutieX; x++)
    {
        //color calculations

        for (int i = 0; i < tileSize; i++)
        {
            for (int j = 0; j < tileSize; j++)
            {
                afbeelding.SetPixel(((x - 1) * tileSize) + i, ((y - 1) * tileSize) + j, color);
            }
        }
    }
}

I would recommend instead, to create the 2d array of colors in that first double for-loop, then, loop over that Color[,] and set the pixels.

If possible, instead just create a max resolution version of your image, and rely on the PictureBox's ability to scale the image for you.

share|improve this answer
    
As resolutieY * tileSize == 1024 (ans assumedly the same applies for resolutieX) the quadruple loop (although it looks heavy) only loops over each pixel exactly once and is not the culprit in this case as you have to visit every pixel once any way. –  Emily L. Jun 12 at 14:25
1  
Also, your ternary block will still incur a lot of branch miss predictions. Structuring the if-else blocks as a binary tree would reduce the if checks from 9 to 4 which is an improvement but a LUT would be faster still. –  Emily L. Jun 12 at 14:28
    
The ternary was a style recommendation, the binary search was the functionality recommondation –  BenVlodgi Jun 12 at 14:30
    
And I said nothing to contradict that. Also, color is only generated once for each tile so creating a smaller image and then up scaling as you suggest will actually create more work load as you need to at least iterate over the image once more and possible perform interpolation. –  Emily L. Jun 12 at 14:35
    
My suggestion is to not scale the image, create a 1:1 copy of the double[,] to image, and leave it up to the PictureBox to handle scaling. I may have not made that clear though. –  BenVlodgi Jun 12 at 14:37

Since noise is essentially random, your large block of if-else chains is causing severe CPU stalls due to branch miss predictions.

You should create a look up table (LUT) and quantize array[x,y] to an integer which indexes into the LUT. This should get rid of many of the miss predictions.

Also note that ((x - 1) * tileSize) + i is invariant under the inner loop and you can move that calculation to the outer loop, although your compiler should already do that if it is optimizing.

Edit: Also, your code doesn't look like perlin noise to me but I could be wrong.

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.