Draw a line using Bresenhams

Discussion in 'C++' started by George, Sep 23, 2005.

  1. George

    George Guest

    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::eek: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::DrawLine(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;
    }
    George, Sep 23, 2005
    #1
    1. Advertising

  2. George

    mlimber Guest

    George wrote:
    > 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>
    #include <cassert>

    >
    >
    > 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::eek: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::DrawLine(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
    mlimber, Sep 23, 2005
    #2
    1. Advertising

  3. "George" <> wrote in message
    news:...
    :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
    --
    http://ivan.vecerina.com/contact/?subject=NG_POST <- email contact
    form
    Brainbench MVP for C++ <> http://www.brainbench.com
    Ivan Vecerina, Sep 23, 2005
    #3
  4. In article <>,
    "mlimber" <> wrote:

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

    >
    > Prefer:
    > #include <cstdio>
    > #include <cstdlib>
    > #include <cassert>
    >
    > or better:
    > #include <iostream>
    > #include <fstream>
    > #include <cassert>


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

    > > 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):


    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.

    > > Pixel ColorImage::readPixel(int x, int y)
    > > {
    > > assert(pPixel); // die if image not initialized


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

    > > void ColorImage::eek: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.


    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
    Howard Hinnant, Sep 23, 2005
    #4
  5. George

    mlimber Guest

    Howard Hinnant wrote:
    > In article <>,
    > "mlimber" <> wrote:
    >

    [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.

    [snip]
    > > > Pixel ColorImage::readPixel(int x, int y)
    > > > {
    > > > assert(pPixel); // die if image not initialized

    >
    > 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
    mlimber, Sep 24, 2005
    #5
  6. In article <>,
    "mlimber" <> wrote:

    > Howard Hinnant wrote:
    > > In article <>,
    > > "mlimber" <> wrote:
    > >

    > [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]
    > > > > Pixel ColorImage::readPixel(int x, int y)
    > > > > {
    > > > > assert(pPixel); // die if image not initialized

    > >
    > > 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
    Howard Hinnant, Sep 24, 2005
    #6
    1. Advertising

Want to reply to this thread or ask your own question?

It takes just 2 minutes to sign up (and it's free!). Just click the sign up button to choose a username and then you can ask your own questions on the forum.
Similar Threads
  1. =?Utf-8?B?QUNhdW50ZXI=?=

    draw line on image

    =?Utf-8?B?QUNhdW50ZXI=?=, Feb 22, 2005, in forum: ASP .Net
    Replies:
    3
    Views:
    462
    Peter Rilling
    Feb 22, 2005
  2. steve smith
    Replies:
    2
    Views:
    700
    Jim Sculley
    Jul 13, 2003
  3. Rick
    Replies:
    3
    Views:
    1,441
    Sandip Chitale
    Sep 30, 2003
  4. SPG
    Replies:
    2
    Views:
    5,345
  5. srinivas
    Replies:
    10
    Views:
    439
    Randy Webb
    Oct 27, 2005
Loading...

Share This Page