CLEANUP: mesh_gfx

Development directions, tasks, and features being actively implemented or pursued by the development team.
Post Reply
chuck_starchaser
Elite
Elite
Posts: 8014
Joined: Fri Sep 05, 2003 4:03 am
Location: Montreal
Contact:

CLEANUP: mesh_gfx

Post by chuck_starchaser »

Just to get the ball rolling I decided to start doing some cleanup, or, well, just writing notes
for now. Why mesh_gfx? Because Klauss recommended I take a look at it in the context of my
feature suggestion to make ai ship motions smoother.
http://vegastrike.sourceforge.net/forum ... 27#p114527

File: /src/gfx/mesh_gfx.cpp
I'm looking at the version in Safemode's branch, as it seems to be the latest:
http://vegastrike.svn.sourceforge.net/v ... ortby=file
Notes:
  • Couldn't

    Code: Select all

    #ifdef _MSC_VER
       26 //Undefine those nasty min/max macros
       27 //MS must hate the STL
       28 #undef min
       29 #undef max
       30 #endif
    be in a header file of its own (e.g. minmaxblues.h)? I'm sure this us not the only cpp file that needs to do this?
  • "class Exception : public std::exception { ..."... Same idea: How about putting it in a "VSexception.h"?
  • class OrigMeshContainer { ... Needs comment about the purpose of the class; not obvious from its name
  • float d; bad name. Initialized to a 'float d' passed in ctor. Line 95, 152, etc, use d in mysterious ways.
  • int program; No idea what this could mean. If it's a vertex program ID, perhaps it should read int vert_prog_ID, or better yet, have a classs vert_prog_id_t { int idnum; public: explicit vert_prog_id_t(int); ... and only operators that make sense, like ==.
  • What does "unsigned int passno:14" do? Never seen this kind of colon notation. Looks like a different language.
  • OrigMeshContainer(){ orig=NULL; }; There should be a bool operator!() const { return orig==NULL; }
  • At end of ctor, there are two assert lines, but no logging of errors for the release version
  • Operator<() is pretty complex routine, but there are no comments to explain the causes for so much complexity. There are four unexplained, acronymically named macros. Most worrisome is the fact that the very meaning of "less than" for an "orig mesh container", whatever that's supposed to be, is nowhere stated.
  • Comments needed to explain the mening and semantic relevance of ...TextureFrames..., ...TextureTime..., etc.
  • Similarly, Terxture * Mesh::TempGetTexture(...) needs comment explaining purpose.. --"Temporary get texture"? What could that possibly mean?
  • Lines 277 to 292 are a bunch of superfluous createThisOrThat() functions that call new ThisOrThat() with the same arguments. Destroy.
  • Line 306 in Mesh::~Mesh() there's a 'delete vlist'. Wanted to make sure it was allocated by the ctor, but can't find the ctor. Ctor and dtor should be together, whether inlined or not. But then the mystery deepened: Where the hell is mesh_gfx.h? Anyways, I think most files are too big, and that there's wisdom in the one class <==> one file tradition. Even when classes are private to a cpp file it's a good idea to have them as separate files in a sub-folder. So, if I see a screwball_t and wonder which include it comes from, I'll probably find #include "screwball.h". But if files declare more than one type on average, I'd probably have to grep it.
  • Mesh::Draw(), like all the rest of the engine AND shaders, is unprofitably complicated by cloaking issues. Cloaking is not used, was never used, and will never be used. If it was ever used it would break everything, as only half the code dealing with cloaking is implemented (see Mesh::DrawNow(), right below, which says "//fixme: cloaking not delt with...") and even what's implemented is not tested (and would never pass any testing, since you can have a few transparencies in a scene, but you can't draw EVERYTHING in alpha blend mode; would be terribly inefficient).
  • Line 430 it says, "//Making it static avoids frequent reallocations - although may be troublesome for thread safety but... WTH... nothing is thread safe in VS. Also: Be careful with reentrancy... right now, this section is not reentrant; static vector <int> specialfxlight; ... hopefully most such statements are or will soon be no longer true. We should remove static, make the function reentrant, and then we can tackle allocation concerns by using a per-thread block allocator (avoiding mutexes).
  • Line 434: unsigned int i; for ( i=0;i<LocalFX.size();i++) ... there must be a way to find and replace ints and unsigneds by size_t's globally... Well, at least, whenever the name is "i" you know size_t was meant :D
Enough 4 now.
I've still no idea what "Orig" means.
safemode
Developer
Developer
Posts: 2150
Joined: Mon Apr 23, 2007 1:17 am
Location: Pennsylvania
Contact:

Re: CLEANUP: mesh_gfx

Post by safemode »

well, i believe with the original mesh, we may be caching the mesh file itself, unprocessed. But i'm just guessing, i haven't played in it since i first ported opcode into VS.

anyways, it may mean nothing these days. You have to remember, a lot of the more basic code in VS originates back when VS could load data not originally written for VS. So we support various modes of things that nobody these days uses with VS.

But yea, i hate the static crap everywhere and global vars and mashing up all the classes together and sticking inline definitions of one class function into a source file dealing with a totally different class. That static issue is why i created the options class. There's probably still a few cases in the code where the options class is not used and the variables are still being read directly but they'll all fall to my options class eventually.

If i'm alive on the day when every source and every header file in VS only includes (meaning the #include directive too) the headers and code that it actually requires instead of blindly including another header and hoping that header includes anything you missed ....that'll be a good day.
Ed Sweetman endorses this message.
klauss
Elite
Elite
Posts: 7243
Joined: Mon Apr 18, 2005 2:40 pm
Location: LS87, Buenos Aires, República Argentina

Re: CLEANUP: mesh_gfx

Post by klauss »

chuck_starchaser wrote:"class Exception : public std::exception { ..."... Same idea: How about putting it in a "VSexception.h"?
There is one such class... perhaps it should be reused...
I guess it's the problem of having patch after patch after patch.
chuck_starchaser wrote:class OrigMeshContainer { ... Needs comment about the purpose of the class; not obvious from its name
I guess it wouldn't hurt. However, I'd rather vote for a big comment at the start of the .cpp explaining the whole thing, because there are dozens of classes and functions interacting there.
chuck_starchaser wrote:What does "unsigned int passno:14" do? Never seen this kind of colon notation. Looks like a different language.
Those are C bitfields
chuck_starchaser wrote:OrigMeshContainer(){ orig=NULL; }; There should be a bool operator!() const { return orig==NULL; }
Why?
chuck_starchaser wrote:At end of ctor, there are two assert lines, but no logging of errors for the release version
mesh_gfx is a critical path... I wouldn't log there. Besides, the assert does the logging on failure already. And the release version is supposed to skip the check (more so being in a critical path)
chuck_starchaser wrote:Operator<() is pretty complex routine, but there are no comments to explain the causes for so much complexity. There are four unexplained, acronymically named macros. Most worrisome is the fact that the very meaning of "less than" for an "orig mesh container", whatever that's supposed to be, is nowhere stated.
Perhaps because the mechanism involving an "OrigMeshContainer" isn't explained.
It defines a sort order (a vector of OrigMeshContainers is sorted when rendering each frame).
chuck_starchaser wrote:Comments needed to explain the mening and semantic relevance of ...TextureFrames..., ...TextureTime..., etc.
Textures may be animated. As such, they have a current time, number of frames, etc...
chuck_starchaser wrote:Line 306 in Mesh::~Mesh() there's a 'delete vlist'. Wanted to make sure it was allocated by the ctor, but can't find the ctor. Ctor and dtor should be together, whether inlined or not.
Good point. Still, delete doesn't explode if vlist is NULL, so if the ctor sets it to NULL it's enough.
chuck_starchaser wrote:Mesh::Draw(), like all the rest of the engine AND shaders, is unprofitably complicated by cloaking issues. Cloaking is not used, was never used, and will never be used. If it was ever used it would break everything, as only half the code dealing with cloaking is implemented (see Mesh::DrawNow(), right below, which says "//fixme: cloaking not delt with...") and even what's implemented is not tested (and would never pass any testing, since you can have a few transparencies in a scene, but you can't draw EVERYTHING in alpha blend mode; would be terribly inefficient).
It's complicaaaated. DrawNow is only used for immediate rendering, and that happens in very few places. In fact, I can't remember one place where it's used (at one time bases used it, but since DrawNow is an imcomplete implementation that was problematic and was changed).
Perhaps DrawNow should be eradicated.
chuck_starchaser wrote:Line 430 it says, "//Making it static avoids frequent reallocations - although may be troublesome for thread safety but... WTH... nothing is thread safe in VS. Also: Be careful with reentrancy... right now, this section is not reentrant; static vector <int> specialfxlight; ... hopefully most such statements are or will soon be no longer true. We should remove static, make the function reentrant, and then we can tackle allocation concerns by using a per-thread block allocator (avoiding mutexes).
No graphics engine is multithreaded, since OpenGL isn't. Well, OpenGL is thread-safe I think, but blocks just like malloc and other thread-safe but not multi-threaded APIs.
Oíd mortales, el grito sagrado...
Call me "Menes, lord of Cats"
Wing Commander Universe
chuck_starchaser
Elite
Elite
Posts: 8014
Joined: Fri Sep 05, 2003 4:03 am
Location: Montreal
Contact:

Re: CLEANUP: mesh_gfx

Post by chuck_starchaser »

klauss wrote:
chuck_starchaser wrote:"class Exception : public std::exception { ..."... Same idea: How about putting it in a "VSexception.h"?
There is one such class... perhaps it should be reused...
I guess it's the problem of having patch after patch after patch.
Alright, so; let's do it. Where would that be?
chuck_starchaser wrote:class OrigMeshContainer { ... Needs comment about the purpose of the class; not obvious from its name
I guess it wouldn't hurt. However, I'd rather vote for a big comment at the start of the .cpp explaining the whole thing, because there are dozens of classes and functions interacting there.
That would be great. Could you write a little blurb? Right here. I'll flesh it out, if I can, and commit it. may seem like nothing compared to the mountain of uncommented code, but evere journey begins with the first step.
chuck_starchaser wrote:What does "unsigned int passno:14" do? Never seen this kind of colon notation. Looks like a different language.
Those are C bitfields
LOL. I can't remember seeing them before, but maybe it's all the morphine...
chuck_starchaser wrote:OrigMeshContainer(){ orig=NULL; }; There should be a bool operator!() const { return orig==NULL; }
Why?
Never mind; just a personal preference; I like clients to be able to test for initialization with !, with classes that could be uninitialized.

And for the rest, ok. But what's d?
Post Reply