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'tbe 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?
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
- "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
I've still no idea what "Orig" means.