Post by JackHRESULT CMesh::SetMeshData(char *szfilename)
You should respect const-correctness, and use 'const char *' as input
parameter.
Better, move to Unicode (or at least use TCHAR, which works in both
ANSI/MBCS and Unicode builds).
Moreover, usual Hungarian Notation for pointers to "raw C" NUL-terminated
strings is *psz*, not 'sz'; 'sz' is usually for static buffers like:
char szBuf[ 100 ];
(in fact, sizeof(pszSomething) is always 4 on 32-bit systems, i.e. the size
of a pointer; instead sizeof(szSomething) is the size in bytes of given
buffer; so it can be good to differentiate between these two cases using
different prefixes).
So, I would write:
HRESULT CMesh::SetMeshData( LPCTSTR pszFilename )
Post by Jack{
LARGE_INTEGER size;
HANDLE hFileMapping;
int cchFileName;
char szPath[256];
char *szTemp;
DWORD cchPath;
cchFileName, szTemp and cchPath seem not used in your code: I would just
remove their definitions from method body.
Moreover, use WCHAR or TCHAR, instead of obsolete char, e.g.:
TCHAR szPath[ ... ];
Post by JackCAllocateHierarchy Alloc;
// unload these functions to a dll
::GetModuleFileNameA(NULL, szPath, sizeof(szPath));
Just use the Unicode or TCHAR versions of Win32 APIs (and use _countof or
ARRAYSIZE instead of sizeof, to get proper size in *elements count*, not in
bytes):
GetModuleFileName( NULL, szPath, _countof( szPath ) );
Moreover, I would check the return value of Win32 APIs like the above one.
Post by Jackstrcat (szPath, szfilename);
Use CString class to concatenate strings, or use safe string functions like
StringCchCat, but you should avoid use of old strcat & Co.
Post by Jack//// Open .dat mesh
HANDLE h = CreateFileA(szPath, GENERIC_READ, NULL, NULL, OPEN_EXISTING,
CreateFile is just fine (get rid of the CreateFileA).
Post by JackFILE_ATTRIBUTE_NORMAL, NULL);
if (h == INVALID_HANDLE_VALUE)
{
MessageBoxA(NULL, "Couldn't open file with CreateFile()", "Error",
MB_OK);
return E_FAIL;
I think that it is better to return an HRESULT based on GetLastError() than
a generic E_FAIL.
e.g.
DWORD errorCode = GetLastError();
return HRESULT_FROM_WIN32( errorCode );
Moreover, I would prefer tracing error messages like above one using
ATLTRACE instead of MessageBox.
All that applies also to other Win32 calls you had (like CreateFileMapping
and MapViewOfFile).
Post by Jack}
GetFileSizeEx(h, &size);
I would check the return value of GetFileSizeEx, too.
Post by Jackstd::vector< BYTE > v( size.QuadPart+0x1e0-4 );
Note that there is a subtle thing here.
In case of allocation failure, std::vector throws a C++ exception (I think
std::bad_alloc).
But you are returning HRESULT's.
So, I would safely guard the vector construction using a try/catch, and in
case of std::bad_alloc exception return a proper HRESULT code like
E_OUTOFMEMORY.
Post by JackBYTE * binfile = &v[0];
//PBYTE binfile = new BYTE[size.QuadPart+0x1e0-4]; // maybe get the file
size 1st
memset (binfile, 0, size.QuadPart+0x1e4-4);
I don't like these "magic" numbers like 0x1E4 - 4.
I would define a named constant for that:
static const size_t something = 0x1E0;
Moreover, you passed 'size.QuadPart+0x1e0-4' to the constructor, but you are
memset'ting a different (bigger size).
I would just use v.size() method to get size of data in bytes (considering
that your vector instance is a vector of BYTE's).
memset( &v[0], 0, v.size() );
Post by Jack//binfile.clear();
memcpy (binfile, template_bin, 0x1e0);
//binfile.push_back(template_bin);//, 0x1e0);
binfile += 0x1e0;
hFileMapping = CreateFileMapping (h, NULL, PAGE_READONLY, 0, 0, NULL);
if (hFileMapping == 0)
{
CloseHandle(h);
MessageBoxA(NULL, "Couldn't open file mapping", "Error", MB_OK);
return E_FAIL;
}
//// Get a whole file into memory
PBYTE g_pMappedFileBase = (PBYTE) MapViewOfFile (hFileMapping,
Usually the g_ prefix is for *g*lobal variables, not for local ones.
Using g_ prefix for a local variable seems to me confusing.
Post by JackFILE_MAP_READ, 0, 0, 0);
if (g_pMappedFileBase == 0)
{
CloseHandle (hFileMapping);
CloseHandle (h);
return E_FAIL;
}
if (memcmp(g_pMappedFileBase, "hdr1", 4) == 1)
{
CloseHandle(hFileMapping);
CloseHandle(h);
MessageBoxA(NULL, "Error Loading Header", "Error", MB_OK);
return E_FAIL;
}
memcpy (binfile, g_pMappedFileBase+4, size.QuadPart-4);
binfile -= 0x1e0; // back to origin
HRESULT hr = D3DXLoadMeshHierarchyFromXInMemory((LPCVOID) binfile,
size.QuadPart + 0x1e0-4, D3DXMESH_MANAGED, m_pDevice, &Alloc,
NULL, (LPD3DXFRAME*)&(m_pFrameRoot), &m_pAnimController);
if (FAILED(hr))
{
MessageBoxA(NULL, "Can't load mesh", "Error", MB_OK);
}
You should return 'hr' here, not continuing execution to 'return S_OK;' that
follows.
Post by Jackif (binfile) {
delete binfile;
binfile = NULL;
std::vector deletes itself (RAII): the above delete is wrong.
Note that you may want to use RAII paradigma also for file handle and memory
mapping handle, i.e. you could write a simple class to wrap these handles
and the destructor calls CloseHandle for you.
In this way, you don't need to repeat CloseHandle calls in every failure
exit points in your function: destructors will automatically be called for
you, and your code is simplified.
Post by Jack}
return S_OK;
}
Giovanni