Compile issues on Windows

Everything about development and the OpenMW source code.
Locked
User avatar
lgromanowski
Site Admin
Posts: 1193
Joined: 05 Aug 2011, 22:21
Location: Wroclaw, Poland
Contact:

Compile issues on Windows

Post by lgromanowski »

athile wrote: I'm still ramping up on the development side, but there appear to be a couple problems in the current code which are likely specific to Windows and Visual Studio 2008:

(1) With the default CMake settings for VS2008, "near" and "far" cannot be used as identifiers in the OpenMW code (anyone who ever wrote 16-bit DOS code might recognize these "keywords" :)). I'm sure there's a setting somewhere to avoid this, but it seems more practical for maximum compatibility to simply pretend near and far are reserved words and rename them in the OpenMW code. Specifically, in node.hpp rename NiCamera::Camera "near" and "far" to "nearDistance" and "farDistance"?

(2) RC_NONE is a GDI raster capabilites #define in one of the GDI windows headers (http://msdn.microsoft.com/en-us/library/aa447821.aspx). The Nif::RecordType::RC_NONE causes a conflict here. How about renaming the enum to Nif::RecordType::RC_NiNone?

(3) "alphaTest" can be used (passed to createMaterial()) without being initialized in ogre_nif_loader.cpp. I realize the OpenMW code never actually uses alphaTest in the case that alphaFlags == -1, but still it generates a run-time warning in Visual Studio. Initializing alphaTest to zero would hide this run-time debug warning. (As a side issue, I see createMaterial() takes a float argument, but alphaTest is a ubyte...)

Eventually, I'll try to provide patches for such things myself - but I'm still familiarizing myself with git and the rest. Thanks.
athile wrote: I've put together a patch of changes that does two things:
1) Gets the code building on Windows again
2) Adds code to check the standard Morrowind install directories (Steam or CD install) if the specified data directory does not have Morrowind.esm in it.

What's the procedure for submitting code changes? Does it need to be reviewed? Do I create some new branch on GitHub? Again, apologies if this is info I should know...I'm new to the project and to Git.


Also...since the Forums seem to reject files ending the extensions ".patch", ".txt", or seemingly anything other than an image, here's the full text of the patch:


Code: Select all

From b08d2670cd33fde624844acc9442576e4e96a18b Mon Sep 17 00:00:00 2001
From: athile <[email protected]>
Date: Fri, 11 Jun 2010 20:36:50 -0700
Subject: [PATCH] Adds a function to try to automatically find the Morrowind data directory as well as provides some Windows compile fixes.

---
 game/main.cpp               |   55 +++++++++++++++++++++++++++++++++++++-----
 nif/nif_file.cpp            |    2 +-
 nif/node.hpp                |    7 ++++-
 nif/record.hpp              |    4 +-
 nifogre/ogre_nif_loader.cpp |   12 ++++----
 tools/fileops.cpp           |   11 ++++++++
 tools/fileops.hpp           |    7 +++++
 7 files changed, 81 insertions(+), 17 deletions(-)

diff --git a/game/main.cpp b/game/main.cpp
index f66fed2..8ae0807 100644
--- a/game/main.cpp
+++ b/game/main.cpp
@@ -16,13 +16,10 @@
 
 using namespace std;
 
-void maintest (std::string dataDir)
+void maintest (const std::string& dataDir)
 {
   assert (!dataDir.empty());
-  
-  if (dataDir[dataDir.size()-1]!='/' && dataDir[dataDir.size()-1]!='\\')
-    dataDir += "/";
-
+ 
   const char* esmFile = "Morrowind.esm";
   const char* bsaFile = "Morrowind.bsa";
 
@@ -77,6 +74,49 @@ void maintest (std::string dataDir)
   cout << "\nThat's all for now!\n";
 }
 
+
+
+/**
+    This method will do a simple search in standard locations to find
+    the Morrowind data files if they are not where expected.
+
+    This is a convenience to allow OpenMW to "just work" as much as 
+    possible for users and new developers.
+ */
+static std::string findDataDirectory (std::string dataDir)
+{
+    const std::string kEsmFile = "Morrowind.esm";
+    appendDirectorySeparatorIfNeeded(dataDir);
+
+    // The currently specified directory seems fine: exit out
+    if (isFile(dataDir + kEsmFile))
+        return dataDir;
+
+    // Try the Windows "Program Files" directory.  For simplicity this code
+    // is compiled on all platforms - on non-Windows platforms, the first
+    // if-check will likely fail and move on.
+    //
+    const char* progFilesPtr = getenv("ProgramFiles");
+    if (progFilesPtr)
+    {
+        std::string progFiles = progFilesPtr;
+        appendDirectorySeparatorIfNeeded(progFiles);
+
+        // Check the Bethesda Softworks install
+        std::string path;
+        path = progFiles + "Bethesda Softworks/Morrowind/Data Files/";
+        if (isFile(path + kEsmFile))
+            return path;
+
+        // Check for a Steam install
+        path = progFiles + "Steam/steamapps/common/morrowind/Data Files/";
+        if (isFile(path + kEsmFile))
+            return path;
+    }
+
+    return dataDir;
+}
+
 int main(int argc, char**argv)
 {
   try
@@ -107,8 +147,9 @@ int main(int argc, char**argv)
       std::cout << desc << std::endl;
     }
     else
-    {          
-      maintest (variables["data"].as<std::string>());
+    {   
+        const std::string dataDir = findDataDirectory(variables["data"].as<std::string>());
+        maintest(dataDir);
     }  
   }
   catch(exception &e)
diff --git a/nif/nif_file.cpp b/nif/nif_file.cpp
index 40e923f..a55daf5 100644
--- a/nif/nif_file.cpp
+++ b/nif/nif_file.cpp
@@ -156,7 +156,7 @@ void NIFFile::parse()
         fail("Unknown record type " + rec.toString());
 
       assert(r != NULL);
-      assert(r->recType != RC_NONE);
+      assert(r->recType != RC_NiNothing);
       r->recName = rec;
       records[i] = r;
       r->read(this);
diff --git a/nif/node.hpp b/nif/node.hpp
index 381dd7c..8b5770e 100644
--- a/nif/node.hpp
+++ b/nif/node.hpp
@@ -139,7 +139,12 @@ struct NiCamera : Node
   struct Camera
   {
     // Camera frustrum
-    float left, right, top, bottom, near, far;
+    float left;
+    float right;
+    float top;
+    float bottom;
+    float nearDist;
+    float farDist;
 
     // Viewport
     float vleft, vright, vtop, vbottom;
diff --git a/nif/record.hpp b/nif/record.hpp
index 3e68489..5a18ed3 100644
--- a/nif/record.hpp
+++ b/nif/record.hpp
@@ -31,7 +31,7 @@ class NIFFile;
 
 enum RecordType
 {
-  RC_NONE = 0,
+  RC_NiNothing = 0,
   RC_NiNode,
   RC_NiTriShape,
   RC_NiRotatingParticles,
@@ -89,7 +89,7 @@ struct Record
   int recType;
   SString recName;
 
-  Record() : recType(RC_NONE) {}
+  Record() : recType(RC_NiNothing) {}
 
   /// Parses the record from file
   virtual void read(NIFFile *nif) = 0;
diff --git a/nifogre/ogre_nif_loader.cpp b/nifogre/ogre_nif_loader.cpp
index 3225030..0957c0e 100644
--- a/nifogre/ogre_nif_loader.cpp
+++ b/nifogre/ogre_nif_loader.cpp
@@ -178,8 +178,8 @@ static void createMaterial(const String &name,
 static String getUniqueName(const String &input)
 {
   static int addon = 0;
-  static char buf[8];
-  snprintf(buf,8,"_%d", addon++);
+  static char buf[64];
+  sprintf(buf, "_%d", addon++);
 
   // Don't overflow the buffer
   if(addon > 999999) addon = 0;
@@ -248,8 +248,8 @@ static void createOgreMesh(Mesh *mesh, NiTriShape *shape, const String &material
     {
       const float *colors = data->colors.ptr;
       RenderSystem* rs = Root::getSingleton().getRenderSystem();
-      RGBA colorsRGB[numVerts];
-      RGBA *pColour = colorsRGB;
+      std::vector<RGBA> colorsRGB(numVerts);
+      RGBA *pColour = &colorsRGB.front();
       for(int i=0; i<numVerts; i++)
    {
      rs->convertColourValue(ColourValue(colors[0],colors[1],colors[2],
@@ -260,7 +260,7 @@ static void createOgreMesh(Mesh *mesh, NiTriShape *shape, const String &material
       vbuf = HardwareBufferManager::getSingleton().createVertexBuffer(
           VertexElement::getTypeSize(VET_COLOUR),
      numVerts, HardwareBuffer::HBU_STATIC_WRITE_ONLY);
-      vbuf->writeData(0, vbuf->getSizeInBytes(), colorsRGB, true);
+      vbuf->writeData(0, vbuf->getSizeInBytes(), &colorsRGB.front(), true);
       bind->setBinding(nextBuf++, vbuf);
     }
 
@@ -420,7 +420,7 @@ static void handleNiTriShape(Mesh *mesh, NiTriShape *shape, int flags)
 
       // Alpha modifiers
       int alphaFlags = -1;
-      ubyte alphaTest;
+      ubyte alphaTest = 0.0f;
       if(a)
         {
           alphaFlags = a->flags;
diff --git a/tools/fileops.cpp b/tools/fileops.cpp
index bb85983..e58968a 100644
--- a/tools/fileops.cpp
+++ b/tools/fileops.cpp
@@ -6,3 +6,14 @@ bool isFile(const char *name)
   boost::filesystem::path cfg_file_path(name);
   return boost::filesystem::exists(cfg_file_path);
 }
+
+
+void appendDirectorySeparatorIfNeeded (std::string& path)
+{
+    if (!path.empty())
+    {
+        const char lastCharacter = path[path.size() - 1];
+        if (lastCharacter !='/' && lastCharacter !='\\')
+            path += "/";
+    }
+}
\ No newline at end of file
diff --git a/tools/fileops.hpp b/tools/fileops.hpp
index 6a5ad7c..37a1459 100644
--- a/tools/fileops.hpp
+++ b/tools/fileops.hpp
@@ -1,7 +1,14 @@
 #ifndef __FILEOPS_H_
 #define __FILEOPS_H_
 
+#include <string>
+
 /// Check if a given path is an existing file (not a directory)
 bool isFile(const char *name);
+inline bool isFile(const std::string& s) { return isFile(s.c_str());}
+
+/// Adds a directory separator character (e.g. "/") to the end of the path if
+/// there is not already one 
+void appendDirectorySeparatorIfNeeded (std::string& path);
 
 #endif
-- 
1.7.0.4
[/size]
Zini wrote:
(1) With the default CMake settings for VS2008, "near" and "far" cannot be used as identifiers in the OpenMW code (anyone who ever wrote 16-bit DOS code might recognize these "keywords" :)). I'm sure there's a setting somewhere to avoid this, but it seems more practical for maximum compatibility to simply pretend near and far are reserved words and rename them in the OpenMW code. Specifically, in node.hpp rename NiCamera::Camera "near" and "far" to "nearDistance" and "farDistance"?
Are you serious? Is this crap still in use? Can't MSVC be configured into a more standard behaviour? I don't think Compiler vendors are allowed to add random keywords outside the reserved range (if they want to provide a standard-conformant compiler). I know it is only a minor issue, but this makes me rage.
(2) RC_NONE is a GDI raster capabilites #define in one of the GDI windows headers (http://msdn.microsoft.com/en-us/library/aa447821.aspx). The Nif::RecordType::RC_NONE causes a conflict here. How about renaming the enum to Nif::RecordType::RC_NiNone?
Rotten Microsoft defines. Die Gates, die!
I agree though, that these enums should be renamed. It is bad practice to use all-uppercase enums exactly for this reason. The most simple soluation would be to change this partcular one into RC_None, but RC_NiNone works too.
(3) "alphaTest" can be used (passed to createMaterial()) without being initialized in ogre_nif_loader.cpp. I realize the OpenMW code never actually uses alphaTest in the case that alphaFlags == -1, but still it generates a run-time warning in Visual Studio. Initializing alphaTest to zero would hide this run-time debug warning. (As a side issue, I see createMaterial() takes a float argument, but alphaTest is a ubyte...)
What do you mean by runtime warning? I could imagine, that this causes a compiletime warning. Actually there are many locations in the code, that could cause a compiletime warning. We might consider switching on some more warnings (the default warning level at least on gcc is pretty low).
Zini wrote: The easiest way to provide a patch is to fork the project on github and then send a pull request. github has ample documenation about this procedure:

http://help.github.com/forking/

I looked at your patch and there are several things I don't really agree with. Also, it is a good idea, to split unrelated changes into separate commits.

1. findDataDirectory

- You shouldn't rely on the variable ProgramFiles not existing on other systems. Please #ifdef it out.

- There is no guarantee, that there is a Morrowind.esm file in the given data directory. The user might want to point it to a completely different directory outside of the Morrowind installation. In this case your code, would override the user setting and point it to Morrowind anyway.
If you want to use any automatic directory finding function, it should
do so only if no data directory is configured via command line and openmw.cfg.

2. appendDirectorySeparatorIfNeeded

Is the handling of / and \ really needed? I was under the impression, that even on Windows / would be accepted here.
And if it is not, boost filesystem (which we use anyway) offers some nifty functions to handle it without the use of custom code.
And I think boost has an isFIle function too (used in your findDataDirectory function).
nicolay wrote:
athile wrote:I've put together a patch of changes that does two things:
1) Gets the code building on Windows again
2) Adds code to check the standard Morrowind install directories (Steam or CD install) if the specified data directory does not have Morrowind.esm in it.
Great, thanks for the heads up. I've applied the parts related to 1), but I'll hold back on the rest until I've reviewed it a bit more. I agree that we should check common locations (might check for the 'official' Morrowind registry entry too?), but the code should be Windows-only. Also note that I've applied changes by hand so my code won't match your code.
What's the procedure for submitting code changes? Does it need to be reviewed? Do I create some new branch on GitHub? Again, apologies if this is info I should know...I'm new to the project and to Git.
No apologies needed, I only learned this stuff recently myself. In fact I had an idea of making a (short) how-to-set-up-OpenMW-with-GitHub page on the Wiki, linking to the GitHub tutorial for more details.

The way it works is you go to GitHub, fork the project (very important that you don't just reupload the entire code as a separate project). That gives you your private online repo that you can modify anyway you like. I can then review your changes directly on the net, pull in all your commits or even cherry-pick only the ones I like. Git is a bit confusing at first if you're used to SVN/CVS style version control, but once you understand how it actually works (which is very simple - even simpler than how SVN works) - it is incredibly intuitive. Ask away if there's any more questions, maybe we can make a FAQ out of it :)
nicolay wrote:
nicolay wrote:Ask away if there's any more questions, maybe we can make a FAQ out of it :)
.. aaand done.
athile wrote: Zini, to answer your questions:
Are you serious? Is this crap still in use? Can't MSVC be configured into a more standard behaviour? I don't think Compiler vendors are allowed to add random keywords outside the reserved range (if they want to provide a standard-conformant compiler). I know it is only a minor issue, but this makes me rage.
I'm speculating here - so forgive me if I'm wrong. Segmented memory models do still exist. I believe the modern use case would be embedded devices. I suspect that there's someone out there is still using the near/far keywords (Are they using VS2008? Who knows?). There's also the issue of syntactically parsing the keyword correctly in legacy code even if it's treated a no-op. That may not matter to you or me, but it might to a decades old company with chunks of old code originally written back in the 16-bit OS days.

I do agreed with you, though - it'd be nice if these "keywords" were off by default and you had to go out of your way to turn on support for them.
What do you mean by runtime warning?
I mean do mean runtime warning: http://msdn.microsoft.com/en-us/library/8wtf2dfz.aspx. Apparently CMake has the /RTCu option enabled by default for debug builds. These runtime checks can be annoying as they sometimes generate false positives (which can be worked around easily enough when it happens), but it's probably good to have them enabled.
Locked