Archived

This topic is now archived and is closed to further replies.

Moe

Help needed with linking....

Recommended Posts

I seem to be having a small bit of trouble getting the files of my project to fit in with eachother well. I have a main d3d.h and d3d.cpp for my wrapper (for Direct 3D), a di.h and di.cpp (for my Direct Input wrapper), and a timer.cpp which needs to be accessed in the di.h and di.cpp. Anyway, I was wondering if anyone would be so kind as to help me out. I have bundled up the source code into a .zip file which should be accesible here. All you need to do is download it, see what I mean, and try to figure out how to fix it. Thanks in advance. Edited by - Moe on August 8, 2001 12:13:21 PM

Share this post


Link to post
Share on other sites
I got it to work fine and display a nice black window, and I only had to add 9 lines.

At the top of each header, add

    
#ifndef _D3D_H_ //or whatever the header is called


#define _D3D_H_

...at the end of the header

#endif



If you do that in all your headers, you can include stuff from one header to the other, although you should keep #includes out of the header file as much as possible.

There are some more things you could fix up, but I didn't know if you wanted a fix or a code review.

Edited by - Big B on August 8, 2001 7:14:15 PM

Share this post


Link to post
Share on other sites
Gamedev code reviews would be a really cool thing to start. I've only ever had 2, but in both I learned a lot. It would help newbies out and give some of the pros(not me) a chance to flex their coding skills.

Well Moe, here it goes, and DON'T TAKE ANYTHING PERSONALLY (its unprofessional):

General Comments

  • Your use of comments is good. They give a good sense of whats going on and keep it from looking like just a big ball of code.
  • Real OOPers might lambast you for having everything public. I won't because thats what my project looks like.
  • You should use const instead of #define. Its type safe and makes your code look more C++ish.
  • Don't use #pragma once, since #ifndef/#endif will take care of all your worries.
  • Overall, a good design. Its not populated with any game specific code yet, but it should be easy to use this "engine" for pretty much anything, with some additions naturally.


All line numbers are before any additions to the code.

gamestate.h

  • 13-22 - Use an enum instead of all those defines. That way you don't have to keep track of the numbers and the GAMESTATE would be its own type in a way.
  • 28 - #pragma twice? The first one isn't necessary, so the second one really isn't.
  • 30,31 - Don't need the includes in this file. Put the includes in the gamestate.cpp and use a forward declaration to declare class D3D. That way if di.h or d3d.h changes, it wouldn't change gamestate.h and everything that includes it. It will cut down on compile times if/when you have more source files that include gamestate.h
  • 56-60 - Specify the return type. By default it is an int, but that might not be so apparent to others and may complain on other compilers. Its just a good habit to get into.


di.h

  • 15 - Use const (see general comments)
  • 28 - is hWnd ever initialized? Looking through di.cpp I don't think it is. Maybe have a constructor to set it to 0 and initialize everything to NULL or 0.
  • 31,32,35,38,39,42,45 - Specify return type, same reason.


d3d.h

  • 10 - move include to d3d.cpp. Its just better to have the includes in the source (forget why).
  • 12 - mmsystem.h included twice? Just incase, right
  • 15,16 - use const.
  • 30,31,47-50 - specify return type.


gamestate.cpp

  • 63 - could be used in a destructor. That way your d3d::D3Dshutdown will be called no matter what. Its one of the advantages of OOP. You might not be able to check if everything cleaned up properly, but since your shutting down, its not your problem anymore.


d3d.cpp

  • 26,28-30,32,33 - use false instead of 0. Its a bool so treat it like one (besides, false is a keyword so it adds colour to your code)
  • 194 - could use a destructor. Same reasons.
  • 244,286 - return 2 ? Instead of 2 a constant should be used in case you decide to use 3. And why 2?


di.cpp

  • It's all good!


prototype.cpp

  • 49 - use bool instead of "BOOL" and false instead of "FALSE". Its looks more C++ish and adds colour to your code.






Whew, all done. Whether you take any of this advice or not is up to you, but I believe it would be to your advantage.

I'll let you look at my project when I have a build I am happy with, and you can tear it apart to your hearts content.

Happy Coding

P.S. - remember not to take anything personally

EDIT - dang unordered lists!


Edited by - Big B on August 8, 2001 11:40:05 PM

Share this post


Link to post
Share on other sites
Don''t worry, I won''t take it personally. Thats got to be the best review of code I have ever seen. I really appreciate it. Its always nice to see people''s comments on my code. I will probably make most of those changes.

Oh, and you seem like a pretty professional coder/programmer yourself.

Share this post


Link to post
Share on other sites
I have some comments to your code..

I just looked it over very quickly.

I think you should try and make the various components more independant of each other. For example your input code.

You pass your gamestate to your input code and make decisions based on that gamestate.
That will make it harder to reuse your input code another time.

And don´t call your objects something like "gs". I know gs = gamestate but when your project grows big and has a lot of objects called "gs" and "fp" and "gt" then it might get confusing. And you only call gs in your winmain code so there is really no need to make the name so short since you don´t have to type it so many times...

Well thats all...See ya...



-- Look at you hacker. A Pathetic creature of meat and bone, panting and sweating as you run through my corridors. How can you challenge a perfect, immortal machine?--

"Yeah, I''ve seen people w/ ''so-called'' lives. They are petty and thoughtless." - Nazrix

Share this post


Link to post
Share on other sites
Of all the stupid things:

I can''t get the enum thing to work. I have something like:
  
//the gamestate types

enum STATE {
INIT, //initialization

INTRO, //show intro movie/script

MENU, //menu state

RENDER, //main rendering state

INGAMEMENU, //in game menu state - eg pause, inventory...

SHUTDOWN //shutdown

};

in gamestate.h. Now I can''t get it to work with di.h and di.cpp. The function prototype looks like this:
  
InputHandler(STATE &Gamestate, TIMER &tmr);

What am I doing wrong? I have tried everything but the stupid thing just won''t work. Anyone?

Share this post


Link to post
Share on other sites
Also, how do I do that forward declaration that you described for gamestate.h? I haven''t had much experience with this type of thing.

Share this post


Link to post
Share on other sites
Hi Moe,

I haven''t looked at your code, and I''m not too sure about the advice I''m about to give you, but it would probably be worth your while to look it up:

I think that the forward declaration is done with the "extern" keyword, where you declare the function in all of your files with "extern" in front it. This tells the compiler that the implementation of the function is "external" to the file, so it skips over it (of course, you''ll get an error if the function does not exist in your project at all).

Naturally, the better way to do this is to put all your global function declarations and global objects/variables in a file called globals.h. Type them all with "extern" in front. Include this file in all .cpp files as one of your standard header files. Actually implement the functions and objects in a globals.cpp file (without "extern"s ).

This technique might work for your enum problem too, but I''m REALLY not sure if you can or should do it (I''ve not had much experience with enums yet!)

If I''ve misinterpreted the words "forward declaration" then sorry for wasting your time!!! Otherwise I hope this helps you out!

Gareth

Share this post


Link to post
Share on other sites
Well, I have been trying to keep the number of global variables down (so far I only have 2 - winmain and winproc). My last re-incarnation of this program was done that way but things got way to messy with 50 some global variables.

Thanks for the advice though.

Share this post


Link to post
Share on other sites
I inserted the enum into gamestate.h and lo and behold, it didn''t work:

Compiling...
di.cpp
d:\prototype\prototype\di.h(35) : error C2061: syntax error : identifier ''TIMER''
d:\prototype\prototype\di.cpp(124) : error C2511: ''InputHandler'' : overloaded member function ''int (int &,class TIMER &)'' not found in ''DI''
d:\prototype\prototype\di.h(19) : see declaration of ''DI''

If this looks familiar, I''ve got the answer.

Here''s how you do a forward declaration

  
class TIMER;


A class without a declaration or implementation is just like a function prototype. It has been declared as being somewhere and thats all the compiler cares about. Put that before your definition of class DI in di.h and your program should compile.

Oh, and global variables suxor

Share this post


Link to post
Share on other sites
I am still having problems with it. For some stupid reason it performs an illegal operation whenever I try to close it. At least I got it to compile.

I don't suppose you could e-mail me back the modified source? (rcjbvermilion@hotmail.com) Its really making me mad.

EDIT: I found out what was causing the problem, but its real odd. I made a destructor for the D3D class, which calls the D3DShutdown function. Now when I use the destructor (or leave its code normal, not commented) it performs an illegal operation. But, when I comment the code in the destructor out, it works fine. I have no idea why its doing that, since the code for the D3DShutdown() should handle everything if its already been released or not. Any ideas on why it would mess up?

Oh, I am also having one other wierd problem. I have a very small memory leak after I run in fullscreen mode (and fullscreen mode only). Its always 2304 bytes and appears to be 2 things. I looked through my fullscreen code and I couldn't find anything that I'm not releasing. Any ideas on this?

Edited by - Moe on August 11, 2001 6:59:40 PM

Share this post


Link to post
Share on other sites
I have been adding more code to it that has to do with vertex buffers. The wierd thing is that it won''t release the vertex buffer when I call GAMESTATE::Shutdown(). Anyone?

Anyone?

Share this post


Link to post
Share on other sites
Post your code on your website again. That way there''s fresh code to work with thats exactly in the state in which you are having trouble.

I can''t guarantee a fast response. My time is split between work, girlfriend, and Counter-strike. There''s just not enough time in the week.

Share this post


Link to post
Share on other sites
Ok, I really appreciate the help you have given me, along with all you other gamedev members.

The code should be available here.

I admit, I get carried away playing counterstrike as well. Its soo fun, and it actually doesn''t lag that bad on my poor 33.6 modem (its really 56k but it can''t connect as fast because of old phone lines).

Share this post


Link to post
Share on other sites
Ok, I have been playing around with a few things. I have some problem with the GAMESTATE destructor. For some reason when I call gamestate.GameStateShutdown(), and I have the destructor so it calls the same function, I get an illegal operation. Why would it not work? When I release something I do check if it is first, so that can't be the problem. Also, when I comment out the line in the destructor that calls the shutdown function, the function doesn't get called at all even though I am specifically calling it in prototype.cpp at the very end of my program. There is something fishy going on there and I would like to get that figured out.

EDIT - I must be tired... I can't even spell anymore.

Edited by - Moe on August 14, 2001 10:06:35 PM

Share this post


Link to post
Share on other sites
Ok, I have done more playing around with the code. I found out that for some reason the gamestate.GameStateShutdown() function is not being called. I put a message box just before and one in the function, and niether appeared. There is also something funny going on with the destructor for GAMESTATE. When I call the shutdown function in the destructor, it doesn''t get called. I set it up so that a messagebox function should be called when the shutdown function is called, and it never appears even though I should be calling it in the destructor. It also doesn''t get called when I specifically call it. I would like to know what the heck is causing this thing to get messed up. It also performs an illegal operation when I go to close the program if I have the shutdown function in both the destructor and and the end of the program. That shouldn''t happen since I do test if the things are null first.

I don''t really care whatever calls the shutdown function, but as long as it gets called. I would prefer to call it myself and get this whole mess fixed so its called both times just in case I somehow don''t call it. I really just want to stop this memory leak before things get worse.

Anyone?

Share this post


Link to post
Share on other sites
The link from 3 posts ago doesn't work. It gives the redirection message and links to your home site. Just put a link there to your code and the date it was last modified.

What are you using to detect memory leaks?

EDIT - bad speeling

Edited by - Big B on August 15, 2001 5:40:18 PM

Share this post


Link to post
Share on other sites
Ok, the address for the latest code should be:
www20.brinkster.com/tyrannica/memleak.zip. I will edit the page anyway.

Share this post


Link to post
Share on other sites
Whenever something takes a long time to debug, its usually something stupid.

In D3D.cpp change this

    
if(lpd3ddevice != NULL)
lpd3ddevice->Release();

if(lpd3d != NULL)
lpd3d->Release();



to this

  
if(lpd3ddevice != NULL)
{
lpd3ddevice->Release();
lpd3ddevice = NULL;
}

if(lpd3d != NULL)
{
lpd3d->Release();
lpd3d = NULL;
}


The problem was that you were calling lpd3ddevice->Release() twice. The first time it would work, whereas the second time it would crash. Did I mention I have never used Direct3D?

Don't feel bad, I've had my share of stupid bugs that have cost companies a lot of money with me scratching my head.

Email me if you have any other problems.

EDIT-fix source tags

Edited by - Big B on August 19, 2001 8:51:57 PM

Share this post


Link to post
Share on other sites
This kind of error is why a lot of people use a ''SAFE_RELEASE'' macro, which is usually something like:

#define SAFE_RELEASE (x) { if (x) { (x)->Release(); x = NULL; }}

This tends to stop you making these sorts of mistakes. Although, you probably also need to look into why your code tries to release something twice, rather than just patching it up so it doesn''t break.

Share this post


Link to post
Share on other sites
Thanks for that. I will have to try it out as soon as I get home. Sorry for not getting back sooner, my internet has been down since thursday.

Also, the reason for the shutdown code being called twice was that I had a destructor that would call it in case it wasn''t called already. I also had to fix up a few other things in my program (like the main loop wasn''t being called).

So did that also fix the memory leak? (All I did to find it was crank the DX8 debug level up to the max, then debug it.).

Share this post


Link to post
Share on other sites
It appears that there might be something wrong with that SAFE_RELEASE macro, or the way I am using it:
  
SAFE_RELEASE(lpd3ddevice);
SAFE_RELEASE(lpd3d);

Now for some reason it gives me these errors:
  
error C2065: ''x'' : undeclared identifier
error C2143: syntax error : missing '';'' before ''{''
error C2227: left of ''->Release'' must point to class/struct/union
error C2143: syntax error : missing '';'' before ''{''
error C2227: left of ''->Release'' must point to class/struct/union

Let me guess, there is a semicolon missing somewhere...

Share this post


Link to post
Share on other sites