Draw a line using Bresenhams

[George]

I cannot understand how to fix my code so that it will work. It needs
to draw a series of lines in different colors and save it to a ppm
file. Could someone tell me what I have done wrong. Thanks a lot.

#include <stdio.h>
#include <stdlib.h>
#include <assert.h>


struct Pixel {
unsigned char R, G, B; // Red, Green, Blue
};


class ColorImage {
Pixel *pPixel;
int xRes, yRes;
public:
ColorImage();
~ColorImage();
void init(int xSize, int ySize);
void clear(Pixel background);
Pixel readPixel(int x, int y);
void writePixel(int x, int y, Pixel p);
void outputPPM(char *filename);
void DrawLine(int x1, int y1, int x2, int y2, Pixel p);
bool Inside(int x0, int x1, int x2, int x3, int y0, int y1, int y2,
int y3);
void DrawTriangle(int x1, int x2, int x3, int y1, int y2, int y3,
Pixel p);
};


ColorImage::ColorImage()
{
pPixel = 0;
}


ColorImage::~ColorImage()
{
if (pPixel) delete[] pPixel;
pPixel = 0;
}


void ColorImage::init(int xSize, int ySize)
{
Pixel p = {0,0,0};
xRes = xSize;
yRes = ySize;
pPixel = new Pixel[xSize*ySize];
clear(p);
}


void ColorImage::clear(Pixel background)
{
int i;

if (! pPixel) return;
for (i=0; i<xRes*yRes; i++) pPixel = background;

}


Pixel ColorImage::readPixel(int x, int y)
{
assert(pPixel); // die if image not initialized
return pPixel[x + y*yRes];
}


void ColorImage::writePixel(int x, int y, Pixel p)
{
assert(pPixel); // die if image not initialized
pPixel[x + y*yRes] = p;
}


void ColorImage:utputPPM(char *filename)
{
FILE *outFile = fopen(filename, "wb");

assert(outFile); // die if file can't be opened

fprintf(outFile, "P3 %d %d 255\n", xRes, yRes);
fwrite(pPixel, 1, 3*xRes*yRes, outFile );

fclose(outFile);
}

void ColorImage:rawLine(int x1, int y1, int x2, int y2, Pixel p)
{
int dx, dy, d, incE, incNE, x, y;

dx = x2 - x1;
dy = y2 - y1;
d = 2*dy - dx;
incE = 2*dy;
incNE = 2*(dy - dx);
y = y1;
for (x=x1; x<=x2; x++) {
writePixel(x, y, p);
if (d>0) {
d = d + incNE;
y = y + 1;
} else {
d = d + incE;
}
}
}

// A test program that generates varying shades of reds.
int main(int argc, char* argv[])
{
ColorImage image;
Pixel p={0,0,0};
Pixel red={1,0,0};
Pixel blue={0,0,1};
Pixel green={0,1,0};
Pixel white={1,1,1};
Pixel yellow={1,1,0};
Pixel purple={1,0,1};

image.init(512, 512);

image.DrawLine(299,300,299,400,white);

image.outputPPM("output.ppm");
return 0;
}

 

[mlimber]

I cannot understand how to fix my code so that it will work. It needs
to draw a series of lines in different colors and save it to a ppm
file. Could someone tell me what I have done wrong. Thanks a lot.

What exactly is going wrong? We're not here to do your homework for
you, but we might give some hints.

#include <stdio.h>
#include <stdlib.h>
#include <assert.h>

Prefer:
#include <cstdio>
#include <cstdlib>
#include <cassert>

or better:
#include <iostream>
#include <fstream>

struct Pixel {
unsigned char R, G, B; // Red, Green, Blue
};


class ColorImage {
Pixel *pPixel;
int xRes, yRes;
public:
ColorImage();
~ColorImage();
void init(int xSize, int ySize);
void clear(Pixel background);
Pixel readPixel(int x, int y);
void writePixel(int x, int y, Pixel p);
void outputPPM(char *filename);
void DrawLine(int x1, int y1, int x2, int y2, Pixel p);
bool Inside(int x0, int x1, int x2, int x3, int y0, int y1, int y2,
int y3);
void DrawTriangle(int x1, int x2, int x3, int y1, int y2, int y3,
Pixel p);
};


ColorImage::ColorImage()
{
pPixel = 0;
}

First of all, you should prefer initialization lists (see
http://www.parashift.com/c++-faq-lite/ctors.html#faq-10.6):

ColorImage::ColorImage() : pPixel(0) {}

Second of all, you should prefer std::vector to statically or
dynamically allocated arrays (see
http://www.parashift.com/c++-faq-lite/containers.html#faq-34.1).

ColorImage::~ColorImage()
{
if (pPixel) delete[] pPixel;
pPixel = 0;

There is no need for the if statement since it is ok to delete null
(see
http://www.parashift.com/c++-faq-lite/freestore-mgmt.html#faq-16.8).
Also, since you are destroying the object here, there's no point to
setting pPixel back to zero. Did I mention you should prefer
std::vector?

}


void ColorImage::init(int xSize, int ySize)
{
Pixel p = {0,0,0};
xRes = xSize;
yRes = ySize;
pPixel = new Pixel[xSize*ySize];
clear(p);
}


void ColorImage::clear(Pixel background)
{
int i;

if (! pPixel) return;
for (i=0; i<xRes*yRes; i++) pPixel = background;

}


Pixel ColorImage::readPixel(int x, int y)
{
assert(pPixel); // die if image not initialized
return pPixel[x + y*yRes];
}


void ColorImage::writePixel(int x, int y, Pixel p)
{
assert(pPixel); // die if image not initialized
pPixel[x + y*yRes] = p;
}


void ColorImage:utputPPM(char *filename)
{
FILE *outFile = fopen(filename, "wb");

assert(outFile); // die if file can't be opened

fprintf(outFile, "P3 %d %d 255\n", xRes, yRes);
fwrite(pPixel, 1, 3*xRes*yRes, outFile );

fclose(outFile);
}

Prefer std::fstream to FILE operations. Your output here may or may not
be what you want. It depends on your desired file format, of course,
but the fprintf of text (including the \n, which will automagically
become \r\n) to a FILE opened in binary mode seems a bit wacky.

void ColorImage:rawLine(int x1, int y1, int x2, int y2, Pixel p)
{
int dx, dy, d, incE, incNE, x, y;

Declare the variables as you use them, not at the top. Yours is not C++
style. Oh, and make everything you can const (including methods like
outputPPM).

dx = x2 - x1;
dy = y2 - y1;
d = 2*dy - dx;
incE = 2*dy;
incNE = 2*(dy - dx);
y = y1;
for (x=x1; x<=x2; x++) {
writePixel(x, y, p);
if (d>0) {
d = d + incNE;
y = y + 1;
} else {
d = d + incE;
}
}
}

// A test program that generates varying shades of reds.
int main(int argc, char* argv[])

// No need for unused arguments
int main()

{
ColorImage image;
Pixel p={0,0,0};
Pixel red={1,0,0};
Pixel blue={0,0,1};
Pixel green={0,1,0};
Pixel white={1,1,1};
Pixel yellow={1,1,0};
Pixel purple={1,0,1};

image.init(512, 512);

image.DrawLine(299,300,299,400,white);

image.outputPPM("output.ppm");
return 0;
}

One last time, prefer std::vector.

Cheers! --M

 

[Ivan Vecerina]

:I cannot understand how to fix my code so that it will work. It needs
: to draw a series of lines in different colors and save it to a ppm
: file. Could someone tell me what I have done wrong. Thanks a lot.

First of all, you should create test cases and narrow down the problem
without your program.
For example, start by drawing a short horizontal line. If it worked,
try something slightly more complicated, etc.

If the line-drawing algorithms is itself the problem, a better place
to ask your question would be comp.graphics.algorithms, for example.
A majority of people there use C or C++ - while a majority of
participants to comp.lang.c... have no interest in core graphic
algorithms.


Ivan

 

[Howard Hinnant]

#include <stdio.h>
#include <stdlib.h>
#include <assert.h>

Prefer:
#include <cstdio>
#include <cstdlib>
#include <cassert>

or better:
#include <iostream>
#include <fstream>
#include <cassert>[/QUOTE]

Actually if C I/O is going to be used, the <*.h> are currently more

First of all, you should prefer initialization lists (see
http://www.parashift.com/c++-faq-lite/ctors.html#faq-10.6):

Or just dump the default ctor entirely since its only use appears to be
to allow the client to forget to call init. ;-)

ColorImage::ColorImage() : pPixel(0) {}

Second of all, you should prefer std::vector to statically or
dynamically allocated arrays (see
http://www.parashift.com/c++-faq-lite/containers.html#faq-34.1).

<nods vigorously> Use of vector here would also automagically make the
image copy constructor and copy assignment stop crashing (if actually
used).

void ColorImage::init(int xSize, int ySize)
{
Pixel p = {0,0,0};
xRes = xSize;
yRes = ySize;
pPixel = new Pixel[xSize*ySize];
clear(p);
}

This could have been a perfectly good constructor.

Just make it a class invariant that pPixel is always initialized.

Prefer std::fstream to FILE operations. Your output here may or may not
be what you want. It depends on your desired file format, of course,
but the fprintf of text (including the \n, which will automagically
become \r\n) to a FILE opened in binary mode seems a bit wacky.

Unless you really want to read the file with some other native
application, binary might make a lot of sense (if used consistently).
I've lost count of the number of times I've been bitten by newline
translation. Insisting newlines output as '\n' has a certain amount of
sanity if you want your file format to be cross platform.

If I could I would travel back in time 20 years and argue that binary
should be the default but there could be an optional text mode (newline
translation mode). :-\

One last time, prefer std::vector.

<nods vigorously one last time!>

-Howard

 

[mlimber]

[snip]

Prefer:
#include <cstdio>
#include <cstdlib>
#include <cassert>

Actually if C I/O is going to be used, the <*.h> are currently more
portable than the <c*> (unfortunately).

I'm not sure where. All the C++ compilers I have access to (VC++6 and
up, g++ 3.4 and up, and even Texas Instruments C++ which doesn't even
ship with an STL) all support the standard <c*> headers. They're
required for a conformant library, and because of namespacing, I'd only
go with the .h files if my code were in pure C.

[snip]

Just make it a class invariant that pPixel is always initialized.

[snip]

On the other hand, it's not a bad idea to document your assumptions via
asserts. They disappear in release mode, and they'll protect the code
if/when someone else modifies it in the future and forgets to
initialize pPixel.

Cheers! --M

 

[Howard Hinnant]

[snip]

#include <stdio.h>
#include <stdlib.h>
#include <assert.h>

Prefer:
#include <cstdio>
#include <cstdlib>
#include <cassert>

Actually if C I/O is going to be used, the <*.h> are currently more
portable than the <c*> (unfortunately).

I'm not sure where. All the C++ compilers I have access to (VC++6 and
up, g++ 3.4 and up, and even Texas Instruments C++ which doesn't even
ship with an STL) all support the standard <c*> headers. They're
required for a conformant library, and because of namespacing, I'd only
go with the .h files if my code were in pure C.

Try this:

#include <cstdio>

int main()
{
printf("Hello World!\n");
}

It should fail to compile: Unknown identifier printf.

On most, but not all, compilers I'm aware of, it compiles when it
shouldn't. This has two bad consequences:

1. The global namespace is polluted with names it shouldn't be, so you
can't (for example) introduce your own printf without conflict.

2. Code that tries to be namespace correct, includes the <c*>, but
accidently fails to prefix C identifiers with std::, isn't caught as an
error. Such code is ill-formed, commonly runs, but will eventually fail
if ported to a sufficiently conforming compiler.

Conclusion: By today's standard, combined with today's implementations,
it is more portable to admit that the C identifiers are going to appear
in the global namespace, and explicitly make it so with the <*.h> C
headers.

That is, the above example won't portably compile, even though it does
compile on most implementations (it shouldn't on a conforming
implementation). However the following rewrite is both standard, and
universally compiled:

#include <stdio.h>

int main()
{
printf("Hello World!\n");
}

I don't like it (I was responsible for one of the first commercial
implementations to put the C functions into namespace std, and my own
advice really chaps my butt) but it is reality.

[snip]

Just make it a class invariant that pPixel is always initialized.

[snip]

On the other hand, it's not a bad idea to document your assumptions via
asserts. They disappear in release mode, and they'll protect the code
if/when someone else modifies it in the future and forgets to
initialize pPixel.

Sure, I agree. Just dump the default constructor and turn the init
function into a constructor. This will make assert firing far less
likely. If the default constructor must be there for other reasons,
then you can have it set to some reasonable resolution (possibly even
0x0) and rename init to reset_resolution.

Doing so reduces the assert fire to an internal image bug as opposed to
a client misuse of image. Any time you can create a class invariant
that gets rid of a precondition that a client must heed, you've greatly
simplified your interface and made it much more robust in the process.

E.g. imagine how error prone it would be if clients had to ensure that
vector::size() < vector::capacity() every time they called push_back
(say by calling reserve()).

-Howard