Merge pull request #83 from redruin1/mainHEADmain

Fix for `stb_image` heightmaps, using iterator incorrectly, and (I believe) some faulty logic 

8 files changed, 93 insertions, 33 deletions

diff --git a/Source/AI/navmesh.cpp b/Source/AI/navmesh.cpp
index e8ce7c94..751e3c89 100644
--- a/Source/AI/navmesh.cpp
+++ b/Source/AI/navmesh.cpp
@@ -566,15 +566,15 @@ void NavMesh::Save(const string& level_name, const Path& level_path) {
 
     string nav_path = GenerateParallelPath("Data/Levels", "Data/LevelNavmeshes", ".nav", level_path);
 
-    if (config["allow_game_dir_save"].toBool() == false) {
+    if (config["allow_game_dir_save"].toBool() == false) {  // change save location to user directory
         nav_path = AssemblePath(GetWritePath(level_path.GetModsource()), nav_path);
     }
 
-    char zip_path[kPathSize];
+    char zip_path[kPathSize];  // Path of the outermost file
     FormatString(zip_path, kPathSize, "%s.zip", nav_path.c_str());
 
-    char in_zip_file_name[kPathSize];
-    FormatString(in_zip_file_name, kPathSize, "%s.zip", level_name.c_str());
+    char in_zip_file_name[kPathSize];                                         // Name of the file inside the outermost zip file
+    FormatString(in_zip_file_name, kPathSize, "%s.nav", level_name.c_str());  // used to be .zip
 
     LOGI << "Saving nav mesh to " << zip_path << endl;
 
@@ -582,18 +582,22 @@ void NavMesh::Save(const string& level_name, const Path& level_path) {
 
     CreateParentDirs(temp_navmesh_path);
 
+    // Save the .NAV file to the Temp directory
     sample_tile_mesh_.Save(temp_navmesh_path.c_str());
 
     CreateParentDirs(zip_path);
 
+    // Copy the .NAV file from the Temp directory and save to the Zip file as 'level_name.zip' (why .zip?)
     Zip(temp_navmesh_path, string(zip_path), in_zip_file_name, _YES_OVERWRITE);
 
+    // Write the .OBJ file to the LevelNavmeshes directory
     if (!vertices_.empty() && !faces_.empty()) {
         char model_path[kPathSize];
         FormatString(model_path, kPathSize, "%s.obj", nav_path.c_str());
         WriteObj(model_path, vertices_, faces_);
     }
 
+    // Write the .XML file to the LevelNavmeshes directory
     char meta_path[kPathSize];
     FormatString(meta_path, kPathSize, "%s.xml", nav_path.c_str());
 
@@ -751,6 +755,7 @@ bool NavMesh::Load(const string& level_name, const Path& level_path) {
 
 bool NavMesh::LoadFromAbs(const Path& level_path, const char* abs_meta_path, const char* abs_model_path, const char* abs_nav_path, const char* abs_zip_path, bool has_zip) {
     {
+        // Load the XML file
         PROFILER_ZONE(g_profiler_ctx, "Loading XML file");
         TiXmlDocument doc(abs_meta_path);
         doc.LoadFile();
@@ -835,11 +840,14 @@ bool NavMesh::LoadFromAbs(const Path& level_path, const char* abs_meta_path, con
         PROFILER_ZONE(g_profiler_ctx, "handleMeshChanged");
         sample_tile_mesh_.handleMeshChanged(&geom_);
     }
+
     {
-        if (has_zip) {
+        if (has_zip) {  // Attempt to load data from the zip file
             ExpandedZipFile ezf;
             UnZip(abs_zip_path, ezf);
 
+            LOGI << "success!" << std::endl;
+
             if (ezf.GetNumEntries() == 1) {
                 const char* filename;
                 const char* data;
diff --git a/Source/Asset/Asset/image_sampler.cpp b/Source/Asset/Asset/image_sampler.cpp
index 08579fba..f8c6cec7 100644
--- a/Source/Asset/Asset/image_sampler.cpp
+++ b/Source/Asset/Asset/image_sampler.cpp
@@ -115,9 +115,11 @@ int ImageSampler::Load(const std::string& path, uint32_t load_flags) {
 
         int n = 0;
         // Tell stb_image that we want 4 components (RGBA)
-        stbi_uc* data = stbi_load(abs_path, &width_, &width_, &n, 4);
+        stbi_uc* data = stbi_load(abs_path, &width_, &height_, &n, 4);
 
-        if (data == nullptr || n != 4) {
+        // stbi will always ensure that the total output channels will match the specified (4); 'n' will contain the source images' channels
+        // So we only need to throw an error if the data doesn't exist, it's format is guaranteed
+        if (data == nullptr) {  // || n != 4
             return kLoadErrorGeneralFileError;
         }
 
@@ -139,7 +141,7 @@ const char* ImageSampler::GetLoadErrorString() {
         case 0:
             return "";
         default:
-            return "Unknown error";
+            return stbi_failure_reason();
     }
 }
 
diff --git a/Source/GUI/dimgui/imgui_impl_sdl_gl3.cpp b/Source/GUI/dimgui/imgui_impl_sdl_gl3.cpp
index a9b196b1..78bc9cbf 100644
--- a/Source/GUI/dimgui/imgui_impl_sdl_gl3.cpp
+++ b/Source/GUI/dimgui/imgui_impl_sdl_gl3.cpp
@@ -415,7 +415,8 @@ void ImGui_ImplSdlGL3_NewFrame(SDL_Window* window, bool ignore_mouse) {
     // Setup time step
     uint32_t time = SDL_GetTicks();
     double current_time = time / 1000.0;
-    io.DeltaTime = g_Time > 0.0 ? (float)(current_time - g_Time) : (float)(1.0f / 60.0f);
+    // If the time between frames is greater than 0, use that difference; otherwise default to 60fps
+    io.DeltaTime = (current_time - g_Time) > 0.0 ? (float)(current_time - g_Time) : (float)(1.0f / 60.0f);
     g_Time = current_time;
 
     if (ignore_mouse) {
diff --git a/Source/Graphics/heightmap.cpp b/Source/Graphics/heightmap.cpp
index e4119265..c7cf8683 100644
--- a/Source/Graphics/heightmap.cpp
+++ b/Source/Graphics/heightmap.cpp
@@ -92,9 +92,8 @@ bool HeightmapImage::LoadData(const std::string& rel_path, HMScale scaled) {
     FindFilePath(rel_path.c_str(), abs_path, kPathSize, kDataPaths | kModPaths, true, NULL, &modsource);
     modsource_ = modsource;
 
-    // Tell stb_image that we want 4 components (RGBA)
     int img_width = 0, img_height = 0, num_comp = 0;
-    float* data = stbi_loadf(abs_path, &img_width, &img_height, &num_comp, 0);
+    stbi_us* data = stbi_load_16(abs_path, &img_width, &img_height, &num_comp, 0);
 
     if (data == NULL) {
         FatalError("Error", "Could not load heightmap: %s", rel_path.c_str());
@@ -118,11 +117,12 @@ bool HeightmapImage::LoadData(const std::string& rel_path, HMScale scaled) {
             float x_scale = img_width / (float)width_;
             float z_scale = img_height / (float)depth_;
 
-            if (num_comp != 1) {                    // monochrome texture
+            if (num_comp == 1) {                    // monochrome texture
                 for (int z = 0; z < depth_; z++) {  // flipped
-                    float* bits = &data[((int)(z * z_scale)) * img_height];
+                    stbi_us* bits = &data[((int)(z * z_scale)) * img_height];
                     for (int x = 0; x < width_; x++) {
-                        height_data_[x + (((depth_ - 1) - z) * width_)] = (bits[(int)(x * x_scale)] * 65535.0f) / scale_factor;
+                        // Convert unsigned shorts to floats
+                        height_data_[x + (((depth_ - 1) - z) * width_)] = (float)(bits[(int)(x * x_scale)]) / scale_factor;
                     }
                 }
             } else {
diff --git a/Source/Images/stbimage_wrapper.cpp b/Source/Images/stbimage_wrapper.cpp
index 7450cfcf..a777f027 100644
--- a/Source/Images/stbimage_wrapper.cpp
+++ b/Source/Images/stbimage_wrapper.cpp
@@ -25,4 +25,4 @@
 
 #define STB_IMAGE_IMPLEMENTATION
 #define STB_IMAGE_WRITE_IMPLEMENTATION
-#include "stbimage_wrapper.h"
\ No newline at end of file
+#include "stbimage_wrapper.h"
diff --git a/Source/Internal/assetpreload.cpp b/Source/Internal/assetpreload.cpp
index 59ec4b1c..42818bd0 100644
--- a/Source/Internal/assetpreload.cpp
+++ b/Source/Internal/assetpreload.cpp
@@ -26,6 +26,7 @@
 #include <Internal/filesystem.h>
 #include <Internal/path.h>
 
+#include <Images/stbimage_wrapper.h>
 #include <XML/Parsers/levelassetspreloadparser.h>
 
 #include <cstring>
@@ -45,6 +46,11 @@ AssetPreload& AssetPreload::Instance() {
 }
 
 void AssetPreload::Initialize() {
+    // Before we load any assets (in preload or otherwise):
+    // We set stb_image to flip images vertically to load them how Overgrowth expects (positive Y)
+    stbi_set_flip_vertically_on_load(true);
+
+    // Now we can finish preloading
     Reload();
 }
 
diff --git a/Source/Internal/zip_util.cpp b/Source/Internal/zip_util.cpp
index f7dbffca..a4722931 100644
--- a/Source/Internal/zip_util.cpp
+++ b/Source/Internal/zip_util.cpp
@@ -113,10 +113,11 @@ void ZipFile::AddFile(const std::string& src_file_path,
     }
 
     {
-        FILE* file;
+        FILE* file = nullptr;
         file = FOPEN_FUNC(src_file_path.c_str(), "rb");
         if (file == NULL) {
             FatalError("Error", "Could not open file: %s", src_file_path.c_str());
+            return;
         }
 
         int size_read = 0;
@@ -188,7 +189,7 @@ struct ExpandedZipEntry {
     unsigned size;
 };
 
-ExpandedZipFile::ExpandedZipFile() : buf(NULL), filename_buf(NULL), entries(NULL) {}
+ExpandedZipFile::ExpandedZipFile() : buf(NULL), filename_buf(NULL), entries(NULL), num_entries(NULL) {}
 
 ExpandedZipFile::~ExpandedZipFile() {
     Dispose();
@@ -346,16 +347,26 @@ void UnZipFile::Extract(const std::string& in_zip_file_path, const std::string&
 }
 
 void UnZipFile::PrintInfo() {
+    int err;  // TODO: get better user errors instead of codes
     unz_global_info global_info;
-    unzGetGlobalInfo(uf, &global_info);
+    err = unzGetGlobalInfo(uf, &global_info);
+    if (err != UNZ_OK) {
+        FatalError("Unzip Error", "Unable to get global archive info (Error Code: %d)", err);
+    }
     LOGI << global_info.number_entry << " files." << std::endl;
 
-    unzGoToFirstFile(uf);
+    err = unzGoToFirstFile(uf);
+    if (err != UNZ_OK) {
+        FatalError("Unzip Error", "Unable to go to first file in archive (Error Code: %d)", err);
+    }
 
     do {
         unz_file_info file_info;
         char name[256];
-        unzGetCurrentFileInfo(uf, &file_info, name, 256, NULL, 0, NULL, 0);
+        err = unzGetCurrentFileInfo(uf, &file_info, name, 256, NULL, 0, NULL, 0);
+        if (err != UNZ_OK) {
+            FatalError("Unzip Error", "Unable to get current file info (Error Code: %d)", err);
+        }
         LOGI << "\t" << name << std::endl;
         LOGI << "\t\tdosDate: " << file_info.dosDate << std::endl;
         LOGI << "\t\tcrc: " << file_info.crc << std::endl;
@@ -367,11 +378,15 @@ void UnZipFile::PrintInfo() {
 }
 
 void UnZipFile::ExtractAll(ExpandedZipFile& expanded_zip_file) {
+    int err;  // TODO: get better user errors instead of codes
     expanded_zip_file.Dispose();
 
     {  // Allocate memory for the zip file entries
         unz_global_info global_info;
-        unzGetGlobalInfo(uf, &global_info);
+        err = unzGetGlobalInfo(uf, &global_info);
+        if (err != UNZ_OK) {
+            FatalError("Unzip Error", "Unable to get global archive info (Error Code: %d)", err);
+        }
         expanded_zip_file.ResizeEntries(global_info.number_entry);
     }
 
@@ -379,10 +394,16 @@ void UnZipFile::ExtractAll(ExpandedZipFile& expanded_zip_file) {
         unsigned total_size = 0;
         unsigned total_filename_size = 0;
 
-        unzGoToFirstFile(uf);
+        err = unzGoToFirstFile(uf);
+        if (err != UNZ_OK) {
+            FatalError("Unzip Error", "Unable to go to first file in archive (Error Code: %d)", err);
+        }
         do {
             unz_file_info file_info;
-            unzGetCurrentFileInfo(uf, &file_info, NULL, 0, NULL, 0, NULL, 0);
+            err = unzGetCurrentFileInfo(uf, &file_info, NULL, 0, NULL, 0, NULL, 0);
+            if (err != UNZ_OK) {
+                FatalError("Unzip Error", "Unable to get current file info (Error Code: %d)", err);
+            }
             total_size += file_info.uncompressed_size + 1;  // Extra room for '\0'
             total_filename_size += file_info.size_filename + 1;
         } while (unzGoToNextFile(uf) == UNZ_OK);
@@ -395,35 +416,53 @@ void UnZipFile::ExtractAll(ExpandedZipFile& expanded_zip_file) {
         unsigned entry_id = 0;
         unsigned data_offset = 0;
         unsigned filename_offset = 0;
-        ScopedBuffer scoped_buf(size_buf);
-        unzGoToFirstFile(uf);
+        ScopedBuffer scoped_buf(size_buf);  // Raw malloc temp data scoped to be less dangerous
+
+        err = unzGoToFirstFile(uf);
+        if (err != UNZ_OK) {
+            FatalError("Unzip Error", "Unable to go to first file in archive (Error Code: %d)", err);
+        }
         do {
             char filename_buf[256];
+
             unz_file_info file_info;
-            unzGetCurrentFileInfo(uf, &file_info, filename_buf, 256, NULL, 0, NULL, 0);
+            err = unzGetCurrentFileInfo(uf, &file_info, filename_buf, 256, NULL, 0, NULL, 0);
+            if (err != UNZ_OK) {
+                FatalError("Unzip Error", "Unable to get zip file info (Error code: %d)", err);
+            }
             if (file_info.size_filename > 256) {
-                FatalError("Error", "Zip file contains filename with length greater than 256");
+                FatalError("Unzip Error", "Zip file contains filename with length greater than 256");
             }
+
             expanded_zip_file.SetEntry(entry_id, filename_offset, data_offset, file_info.uncompressed_size);
             ++entry_id;
             expanded_zip_file.SetFilename(filename_offset, filename_buf, file_info.size_filename + 1);
             filename_offset += file_info.size_filename + 1;
 
-            unzOpenCurrentFile(uf);
+            err = unzOpenCurrentFile(uf);
+            if (err != UNZ_OK) {
+                FatalError("Unzip Error", "Unable to open current zipped file (Error code: %d)", err);
+            }
+
             int bytes_read = 0;
             do {
                 bytes_read = unzReadCurrentFile(uf, scoped_buf.ptr, size_buf);
                 if (bytes_read < 0) {
-                    FatalError("Error", "Error reading from UnZip file");
+                    FatalError("Unzip Error", "Error reading from current zipped file (Error code: %d)", bytes_read);
                 } else if (bytes_read > 0) {
                     expanded_zip_file.SetData(data_offset, (char*)scoped_buf.ptr, bytes_read);
                     data_offset += bytes_read;
                 }
             } while (bytes_read > 0);
+
             char zero = '\0';
             expanded_zip_file.SetData(data_offset, &zero, 1);
             ++data_offset;
-            unzCloseCurrentFile(uf);
+
+            err = unzCloseCurrentFile(uf);
+            if (err != UNZ_OK) {
+                FatalError("Unzip Error", "Unable to close current file (Error Code: %d)", err);
+            }
         } while (unzGoToNextFile(uf) == UNZ_OK);
     }
 }
diff --git a/Source/Main/scenegraph.cpp b/Source/Main/scenegraph.cpp
index 92559c4e..7724302c 100644
--- a/Source/Main/scenegraph.cpp
+++ b/Source/Main/scenegraph.cpp
@@ -1624,7 +1624,8 @@ void SceneGraph::SendMessageToAllObjects(OBJECT_MSG::Type type) {
     // passing way past the end. Likely because objects_ is sometimes changed as a result of this call.
     // This solution means that most objects get called, some might not if they are first destroyed.
     // New objects will also be called assuming they are added last to the list.
-    for (auto obj : objects_) {
+    for (unsigned int i = 0; i < objects_.size(); i++) {
+        Object* obj = objects_[i];
         if (obj) {
             obj->ReceiveObjectMessage(type);
         } else {
@@ -1634,11 +1635,14 @@ void SceneGraph::SendMessageToAllObjects(OBJECT_MSG::Type type) {
 }
 
 void SceneGraph::SendScriptMessageToAllObjects(std::string& msg) {
-    for (auto obj : objects_) {
+    // For consistency, I'm going to also make this a simple loop in case the same iterator problems affect
+    // This function as well.
+    for (unsigned int i = 0; i < objects_.size(); i++) {
+        Object* obj = objects_[i];
         if (obj) {
             obj->ReceiveObjectMessage(OBJECT_MSG::SCRIPT, &msg);
         } else {
-            LOGE << "One of the objects is NULL when trying to SendMessageToAllObjects." << std::endl;
+            LOGE << "One of the objects is NULL when trying to SendScriptMessageToAllObjects." << std::endl;
         }
     }
 }