Re: Projectile OpenGL Problem
- From: Joseph M. Newcomer <newcomer@xxxxxxxxxxxx>
- Date: Mon, 19 Nov 2007 14:23:47 -0500
I see so many things wrong here that it is hard to figure out where to even begin.
Of course, it is intuitively obvious to everyone reading this what your "strange problem
is"...NOT. How are we supposed to guess what is going on here?
Lots more below...
On 7 May 2007 05:41:04 -0700, Raja <rokkamraja@xxxxxxxxx> wrote:
Hi,****
Attached is the code below for projectile motion. I am trying to
draw the graph but OpenGL is giving me some strange problem.
Can anyone please tell me the problem and solution.
Thank You,
Raja
---------------------------------------------------------------------------------------
#include <windows.h>
#include<conio.h>
I knew there was something wrong at this point. windows.h and conio.h should be mutually
exclusive.
****
#include<math.h>*****
#include <stdio.h>
#include <gl\gl.h> // Header File For The OpenGL32 Library
#include <gl\glu.h>
#include <stdlib.h>
#pragma comment( lib, "opengl32.lib" ) // Search For OpenGL32.lib
While Linking
#pragma comment( lib, "glu32.lib" ) // Search For GLu32.lib While
Linking
#pragma comment( lib, "vfw32.lib" ) // Search For VFW32.lib While
Linking
double xold=0, yold=0;
For a value that is used in one subroutine, and should be a local variable, why is this
declared global?
*****
int Paint(void);****
LONG WINAPI WndProc(HWND,UINT,WPARAM,LPARAM);//Window procedure
HGLRC SetUpOpenGL(HWND hwnd);//Initialize OpenGL
void DrawOpenGLScene(void);//Actual Drawing code
double **arry;
Other than the fact that this declaration shows a complete lack of understanding of how to
program in C, there's nothing wrong with it. See below...
****
int count=0;*****
Of course, the first question that should be asked is why so many global variables. I see
very little reason for most of them.
****
*****
int main()
What is a Windows program doing with a 'main'? This is obviously a console program.
*****
{*****
double v,a,t,r,s,c,x=0,y;
Commas in declaration lists are tasteless. One variable, one line. And if you insist on
commas, consider that every comma should be followed by a space to increase readability!
*****
int i=0;*****
//clrscr();
printf("Projectile Motion Program\n");
WHat kind of nonsense is this? A Windows program does not use printf.
*****
do {*****
printf("Enter initial velocity (floating point number): ");
scanf("%lf",&v);
}while ((v < 0) || (v > 100));
Right out of K&R FIrst Edition. Why are you writing code like this in 2007? This is
pretty awful coding to start with. scanf! Ick! Comparing a double to an integer? And
if the person types "Cat", this is perfectly acceptable! And if there are limits, why
doesn't the prompt specify the limits? And why is 0 a valid value?
*****
do {*****
printf("Enter the angle of projection (in degrees): ");
scanf("%lf",&a);
}while ((a < 5) || (a > 85));
Ditto. And why are the limits not part of the message? Why are you comparing floats to
integers?
*****
a=0.0175*a; //Converting degrees into radians****
printf("The range of the projectile is: %lf \n ",pow(v,2)*(sin(2*a))/
10);
Calling pow(v,2) seems a little weird when (v*v) would do the same computation, only
faster and less obscurely
****
*****
r=pow(v,2)*(sin(2*a))/10;
So why are you repeating the computation? Why not do it first and print r?
*****
s=v*sin(a);****
c=v*cos(a);
for(t=0;x<=r;t=t+0.5)
Where was x initialized? Oh yes, back at the unreadable declaration site.
Why not declare it here and initialize it?
*****
{*****
x=c*t;
count++;
}
printf("%d is the count for range %lf \n", count, r);
arry=(double **)malloc(sizeof(double *)*count);
Wouldn't it have been easier to compute the range without a loop? And the malloc is
nearly unreadable for reasons discussed below...
*****
for(i=0;i<count;i++)****
{
arry[i]=(double *)malloc(sizeof(double)*2);
}
Would it have not made more sense to declare a
typedef struct {
double x;
double y;
} fpoint;
and just declared an array of these?
Then you could write
fpoint * array;
or better still
std::vector<fpoint>;
If you use
fpoint * array;
you could have written
array = (fpoint*)malloc(sizeof(fpoint)*count);
and not need this unbelievably bad implementation of two-level allocation to allocate
trivial data structures!
*****
x=0;*****
for(i=0,t=0;x<=r;t=t+0.5,i++)
Put a space after every comma and a space after every semicolon. Or is unreadability of
code one of your programming goals?
*****
{*****
x=c*t;
y=s*t - 5*t*t;
arry[i][0]=x;
arry[i][1]=y;
arry[i].x = x;
arry[i].y = y;
Before tackling something as complex as OpenGL, it would help a lot to learn the basics of
C/C++ programming. Like elementary data structures.
*****
*****
}
I'm curious why all the precomputation? You should be computing this on-the-fly and you
should not need a one-dimensional array, let alone the awful two-dimensional array you
have done
*****
getch();*****
Just think of how much simpler this would have been as a pure GUI app!
*****
Paint();*****
return 0;
}
int Paint(void)
{
HINSTANCE hInstance=NULL,hPrevInstance=NULL;
Why would you need an hPrevInstance? And don't use commas in declaration lists!
*****
LPSTR lpCmdLine;*****
And what good does this do? And why is it not an LPTSTR? Where, exactly, do you use it?
*****
int nCmdShow;*****
You don't use this, why are you declaring it?
*****
static char szAppName[] = "OpenGL";*****
Why a static? Why a char? Why not const? Why does this variable exist at all????
Why is it called szAppName when it is the name of a window class?
*****
static char szTitle[]="Getting Started With Projectile Motion";*****
Why a static? Why a char? Why not const? Why does this variable exist at all????
Did you make the mistake of cutting and pasting from Petzold? Hint: Petzold's style of
programming well and truly sucks.
*****
WNDCLASS wc; // windows class sruct*****
MSG msg; // message struct
You don't need this MSG structure yet, why is it declared all the way back here?
*****
HWND hWnd; // Main window handle.*****
Ditto
*****
****
// Fill in window class structure with parameters that
// describe the main window.
wc.style =
CS_HREDRAW | CS_VREDRAW;// Class style(s).
wc.lpfnWndProc =
(WNDPROC)WndProc; // Window Procedure
wc.cbClsExtra = 0; // No per-class extra data.
wc.cbWndExtra = 0; // No per-window extra data.
wc.hInstance =
hInstance; // Owner of this class
OK, you have a variable called hInstance, and where, exactly, did you initialize it to the
instance of the current program? Where is the
hInstance = ::GetModuleHandle(NULL);
that would be required for proper initialization? Could it be that you are blindly
copying code from a real Windows app, and then trying to slam it without any thinking into
a console app?
*****
wc.hIcon = NULL; // Icon name*****
wc.hCursor =
LoadCursor(NULL, IDC_ARROW);// Cursor
wc.hbrBackground =
(HBRUSH)(COLOR_WINDOW+1);// Default color
wc.lpszMenuName = NULL; // Menu from .RC
wc.lpszClassName =
szAppName; // Name to register as
why not
wc.lpszClassName = _T(:"OpenGL");
which eliminates the need for the silly variable name which serves no useful purpose.
And why are you calling it an "app name" when it is a window class name?
******
*****
// Register the window class
RegisterClass( &wc );
And, of course, this succeeds, so you don't need to test the result to see if it did,
right?
*****
*****
// Create a main window for this application instance.
hWnd = CreateWindow(
szAppName, // app name
Actually, it is not the app name, it is the window class name, so why are you referring to
a window class name as the app name?
*****
szTitle, // Text for window title bar*****
You could have just put the string value here; you don't need to create a static variable
to hold it and then use the static variable name here!
*****
WS_OVERLAPPEDWINDOW// Window style****
// NEED THESE for OpenGL calls to work!
| WS_CLIPCHILDREN | WS_CLIPSIBLINGS,
CW_USEDEFAULT, 0, CW_USEDEFAULT, 0,
NULL, // no parent window
NULL, // Use the window class menu.
hInstance,// This instance owns this window
Which is still uninitialized.
*****
NULL // We don't use any extra data*****
);
// If window could not be created, return zero
if ( !hWnd )
{
return( 0 );
}
If you WERE doing a console app, note the convention is 0 means success, nonzero means
error....
return is not a function, and does not require its value be parenthesized.
return 1;
would have made sense. Return (0) is silly (and erroneous).
Note that you fail to clean up any of the structures you allocated, which is poor
practice.
*****
****
// Make the window visible & update its client area
ShowWindow( hWnd, 1 );// Show the window
There is no valid value for the second position of ShowWindow that is the number 1. There
are useful values like SW_SHOW, but all values in the second parameter of ShowWindow are
names that start with SW_. I have no idea what "1" means.
*****
UpdateWindow( hWnd ); // Sends WM_PAINT message****
// Enter the Windows message loop
// Get and dispatch messages until WM_QUIT
while (GetMessage(&msg, // message structure
NULL, // handle of window receiving
// the message
0, // lowest message id to examine
0)) // highest message id to examine
{
TranslateMessage( &msg ); // Translates messages
DispatchMessage( &msg ); // then dispatches
}
And where do you free up all that space you allocated?
****
return( msg.wParam );*****
}
LONG WINAPI WndProc( HWND hWnd, UINT msg,
WPARAM wParam, LPARAM lParam )
{
HDC hDC;
It is poor practice to declare variables used by handlers anywhere but in the handlers.
The correct return type is not LONG, it is LRESULT.
*****
static HGLRC hRC; // Note this is STATIC!*****
Read: Note this is incorrect
Declaring a static variable in a window handler is a mistake
*****
PAINTSTRUCT ps;*****
This goes down with the DC declaration
*****
GLdouble gldAspect;*****
GLsizei glnWidth, glnHeight;
Do not use commas in declaration lists
*****
*****
switch(msg)
{
case WM_CREATE:
// Select a pixel format and then
// create a rendering context from it.
hRC = SetUpOpenGL(hWnd);
This is part of the window state, and belongs in a struct referenced by the GWL_USERDATA
field. It should not be a static value in the window. Poor programming practice
Also, I note that you assume that SetUpOpenGL actually worked. Did it? Would it return
NULL if it failed? Did you check this? Why not?
*****
return 0;*****
case WM_SIZE:
{
HDC hDC;
*****
// Redefine the viewing volume and viewport****
// when the window size changes.
// Make the RC current since we're going to
// make an OpenGL call here...
hDC = GetDC(hWnd);
wglMakeCurrent(hDC,hRC);
// get the new size of the client window
// note that we size according to the height,
// not the smaller of the height or width.
glnWidth = (GLsizei) LOWORD (lParam);
glnHeight = (GLsizei) HIWORD (lParam);
gldAspect =
(GLdouble)glnWidth/(GLdouble)glnHeight;
// set up a projection matrix to fill the
// client window
glMatrixMode( GL_PROJECTION );
glLoadIdentity();
// a perspective-view matrix...
gluPerspective(
30.0, // Field-of-view angle
gldAspect, // Aspect ratio of view volume
1.0, // Distance to near clipping plane
10.0 ); // Distance to far clipping plane
glViewport( 0, 0, glnWidth, glnHeight );
wglMakeCurrent( NULL, NULL );
ReleaseDC( hWnd, hDC );
}
I also note that you assume that every OpenGL call has worked successfully. Do you know
this? Do you check it?
*****
*****
return 0;
case WM_PAINT:
// Draw the scene.
// Get a DC, then make the RC current and
// associate with this DC
{
HDC hDC;
PAINTSTRUCT ps;
*****
hDC = BeginPaint( hWnd, &ps );*****
wglMakeCurrent( hDC, hRC );
DrawOpenGLScene();
// we're done with the RC, so
// deselect it
// (note: This technique is not recommended!)
wglMakeCurrent( NULL, NULL );
EndPaint( hWnd, &ps );
}
And did the wglMakeCurrent work? Did you check it?
*****
return 0;*****
case WM_DESTROY:
// Clean up and terminate.
wglDeleteContext( hRC );
PostQuitMessage( 0 );
return 0;
}
// This function handles any messages that we didn't.
// (Which is most messages) It belongs to the OS.
return DefWindowProc( hWnd, msg, wParam, lParam );
}
HGLRC SetUpOpenGL( HWND hWnd )
{
static PIXELFORMATDESCRIPTOR pfd = {
sizeof (PIXELFORMATDESCRIPTOR), // strcut size
1, // Version number
PFD_DRAW_TO_WINDOW | // Flags, draw to a window,
PFD_SUPPORT_OPENGL, // use OpenGL
PFD_TYPE_RGBA, // RGBA pixel values
32, // 32-bit color
0, 0, 0, // RGB bits & shift sizes.
0, 0, 0, // Don't care about them
0, 0, // No alpha buffer info
0, 0, 0, 0, 0, // No accumulation buffer
32, // 32-bit depth buffer
0, // No stencil buffer
0, // No auxiliary buffers
PFD_MAIN_PLANE, // Layer type
0, // Reserved (must be 0)
0, // No layer mask
0, // No visible mask
0 // No damage mask
};
Why is this static?
*****
*****
int nMyPixelFormatID;
HDC hDC;
HGLRC hRC;
hDC = GetDC( hWnd );
nMyPixelFormatID = ChoosePixelFormat( hDC, &pfd );
// catch errors here.
// If nMyPixelFormat is zero, then there's
// something wrong... most likely the window's
// style bits are incorrect (in CreateWindow() )
// or OpenGl isn't installed on this machine
if(nMyPixelFormatID==0)
exit(0);
This is EXCEPTIONALLY poor style. The presence of exit() in any subroutine is
unacceptable. Why is this function not a BOOL function that returns FALSE if it fails?
Where did you call GetLastError to get the error code and display it? A program that just
exits without any message is unbelievably poor style.
*****
SetPixelFormat( hDC, nMyPixelFormatID, &pfd );****
hRC = wglCreateContext( hDC );
ReleaseDC( hWnd, hDC );
return hRC;
}
void DrawOpenGLScene()
{
glEnable( GL_DEPTH_TEST );
//
// Define the modelview transformation.
//
int i;
glMatrixMode( GL_PROJECTION );
glLoadIdentity();
// move the viewpoint out to where we can see everything
glTranslatef( 10.0f, 10.0f, 0.0f );
glBegin(GL_LINES);
glColor3d(1,0,0);
for(i=0;i < count ;i++)
{
glVertex2d(xold,yold);
glVertex2d(arry[i][0],arry[i][1]);
xold=arry[i][0];
yold=arry[i][1];
}
Using std::vector this would be so much easier; you would not need to have a global count,
for example.
And it would have been easier to write arry[i].x instead of arry[i][0].
****
*****
glEnd();
glFlush();
}
This is a truly awful example of some of the worst aspects of programming practice I've
seen in a while. It is some weird hybrid of a console app and a GUI app, which is also
very strange. And since the report says nothing at all about what is going wrong, and you
don't check any error returns to see if any operations succeeded, or query why they
failed, it is going to be very hard to diagnose.
Every call that returns a value indicating an error must be checked; every error return
must be reporting in a meaningful way. Until this is done, it is going to be impossible
to determine what has gone wrong. But a serious rewrite, quite possibly using sensible
ideas such as structures, using new, using std::vector instead of malloc, etc., would be
in order. This rates F- for coding style alone, and is very hard to read or understand in
its current form. It is not surprising it has bugs.
joe
*****
Joseph M. Newcomer [MVP]
email: newcomer@xxxxxxxxxxxx
Web: http://www.flounder.com
MVP Tips: http://www.flounder.com/mvp_tips.htm
.
- Prev by Date: Re: modeless dialog boxes and z order
- Next by Date: Re: GetFontUnicodeRanges() will not compile
- Previous by thread: Re: modeless dialog boxes and z order
- Next by thread: using CFile in DLLs
- Index(es):
Relevant Pages
|
