Phase 1B

ICE/Assets/CMakeLists.txt

@@ -8,6 +8,7 @@ add_library(${PROJECT_NAME} STATIC)
 target_sources(${PROJECT_NAME} PRIVATE
         src/AssetBank.cpp
 		src/AssetPath.cpp
+		src/stb_image_impl.cpp
         src/Shader.cpp
         src/Model.cpp
         src/Mesh.cpp

ICE/Assets/include/Texture.h

@@ -62,7 +62,10 @@ class Texture : public Asset {
 class Texture2D : public Texture {
    public:
     Texture2D(const std::string& path);
-    Texture2D(void* data, int width, int height, TextureFormat fmt);
+    // take_ownership: when true, this texture owns `data` and frees it (with
+    // stbi_image_free, i.e. free) on destruction. Use for stb- or malloc-allocated
+    // buffers the caller hands off; leave false for buffers owned elsewhere.
+    Texture2D(void* data, int width, int height, TextureFormat fmt, bool take_ownership = false);
 
     virtual AssetType getType() const override { return AssetType::ETex2D; }
     virtual std::string getTypeName() const override { return "Texture2D"; }

ICE/Assets/src/Texture.cpp

@@ -16,8 +16,9 @@ Texture2D::Texture2D(const std::string& path) {
         m_format = TextureFormat::None;
     }
 }
-Texture2D::Texture2D(void* data, int width, int height, TextureFormat fmt) {
+Texture2D::Texture2D(void* data, int width, int height, TextureFormat fmt, bool take_ownership) {
     data_ = data;
+    m_owns_data = take_ownership;
     m_width = width;
     m_height = height;
     m_format = fmt;

ICE/Assets/src/stb_image_impl.cpp

@@ -0,0 +1,5 @@
+// The single translation unit that compiles the stb_image implementation for the whole
+// engine. Every other file includes <stb/stb_image.h> as a declaration-only header, so
+// the implementation must be defined here exactly once to avoid duplicate symbols.
+#define STB_IMAGE_IMPLEMENTATION
+#include <stb/stb_image.h>

ICE/Assets/test/AssetBankTest.cpp

@@ -1,7 +1,6 @@
 //
 // Created by Thomas Ibanez on 24.02.21.
 //
-#define STB_IMAGE_IMPLEMENTATION
 #include <gtest/gtest.h>
 
 #include "AssetBank.h"

ICE/Components/include/Component.h

@@ -60,7 +60,20 @@ class ComponentArray : public IComponentArray {
         size--;
     }
 
-    T* getData(Entity entity) { return &(componentArray[entityToIndexMap[entity]]); }
+    // Returns nullptr (instead of silently aliasing another entity's slot) when the
+    // entity has no component of this type. operator[] on the index map used to insert
+    // index 0 for a missing key, handing back entity #0's component -- a silent-corruption
+    // bug. Callers that require the component should use getData and check the result.
+    T* tryGetData(Entity entity) {
+        auto it = entityToIndexMap.find(entity);
+        return it == entityToIndexMap.end() ? nullptr : &(componentArray[it->second]);
+    }
+
+    T* getData(Entity entity) {
+        T* data = tryGetData(entity);
+        assert(data != nullptr && "getData: entity has no component of this type.");
+        return data;
+    }
 
     void entityDestroyed(Entity entity) override {
         if (entityToIndexMap.find(entity) != entityToIndexMap.end()) {
@@ -83,7 +96,7 @@ class ComponentArray : public IComponentArray {
     std::unordered_map<size_t, Entity> indexToEntityMap;
 
     // Total size of valid entries in the array.
-    size_t size;
+    size_t size = 0;
 };
 
 class ComponentManager {
@@ -128,6 +141,17 @@ class ComponentManager {
         return getComponentArray<T>()->getData(entity);
     }
 
+    // Returns nullptr if the type is not registered or the entity has no such component,
+    // instead of asserting/throwing. For callers that legitimately probe for a component.
+    template<typename T>
+    T* tryGetComponent(Entity entity) {
+        auto it = componentArrays.find(typeid(T));
+        if (it == componentArrays.end()) {
+            return nullptr;
+        }
+        return std::static_pointer_cast<ComponentArray<T>>(it->second)->tryGetData(entity);
+    }
+
     void entityDestroyed(Entity entity) {
         // Notify each component array that an entity has been destroyed
         // If it has a component for that entity, it will remove it
@@ -151,8 +175,9 @@ class ComponentManager {
     // Convenience function to get the statically casted pointer to the ComponentArray of type T.
     template<typename T>
     std::shared_ptr<ComponentArray<T>> getComponentArray() {
-        auto const& type = typeid(T);
-        return std::static_pointer_cast<ComponentArray<T>>(componentArrays[type]);
+        // .at() throws std::out_of_range for an unregistered component type instead of
+        // operator[] inserting a null shared_ptr that later null-derefs.
+        return std::static_pointer_cast<ComponentArray<T>>(componentArrays.at(typeid(T)));
     }
 };
 }  // namespace ICE

ICE/Core/src/ICEEngine.cpp

@@ -1,5 +1,3 @@
-#define STB_IMAGE_IMPLEMENTATION
-
 #include "ICEEngine.h"
 
 #include <EngineConfig.h>

ICE/Graphics/include/RenderCommand.h

@@ -31,10 +31,11 @@ struct RenderCommand {
     // Bone matrices - pointer to avoid copying large map
     const std::unordered_map<int, Eigen::Matrix4f>* bones = nullptr;
 
-    // Render state - packed into bitfield to save space
-    bool faceCulling : 1;
-    bool depthTest : 1;
-    bool is_instanced : 1;
+    // Render state - packed into bitfield to save space. Default-initialized so commands
+    // (e.g. the skybox) that don't set them don't read indeterminate values.
+    bool faceCulling : 1 = true;
+    bool depthTest : 1 = true;
+    bool is_instanced : 1 = false;
 
     // Instancing support
     const std::vector<InstanceData>* instance_data = nullptr;

ICE/Graphics/src/ForwardRenderer.cpp

@@ -6,6 +6,8 @@
 
 #include <GL/gl3w.h>
 #include <ICEMath.h>
+
+#include <algorithm>
 #include <LightComponent.h>
 #include <Logger.h>
 #include <RenderComponent.h>
@@ -42,9 +44,12 @@ void ForwardRenderer::prepareFrame(Camera& camera) {
     m_camera_ubo->putData(&camera_ubo_data, sizeof(CameraUBO));
 
     SceneLightsUBO light_ubo_data;
-    light_ubo_data.light_count = m_lights.size();
+    // Clamp to the UBO's fixed capacity: lights[] is MAX_LIGHTS long, so writing more
+    // would overflow the stack-allocated struct.
+    const size_t light_count = std::min(m_lights.size(), static_cast<size_t>(MAX_LIGHTS));
+    light_ubo_data.light_count = static_cast<int>(light_count);
     light_ubo_data.ambient_light = Eigen::Vector4f(0.1f, 0.1f, 0.1f, 1.0f);
-    for (int i = 0; i < m_lights.size(); i++) {
+    for (size_t i = 0; i < light_count; i++) {
         auto light = m_lights[i];
         light_ubo_data.lights[i].position = light.position;
         light_ubo_data.lights[i].rotation = light.rotation;

ICE/GraphicsAPI/OpenGL/src/OpenGLShader.cpp

@@ -137,6 +137,8 @@ GLenum OpenGLShader::stageToGLStage(ShaderStage stage) {
         case ShaderStage::Compute:
             return GL_COMPUTE_SHADER;
     }
+    Logger::Log(Logger::FATAL, "Graphics", "Unknown shader stage %d", static_cast<int>(stage));
+    return GL_VERTEX_SHADER;
 }
 
 }  // namespace ICE

ICE/IO/src/MeshLoader.cpp

@@ -5,6 +5,7 @@
 #include "MeshLoader.h"
 
 #include <AssetBank.h>
+#include <Logger.h>
 #include <Material.h>
 #include <assert.h>
 #include <assimp/postprocess.h>
@@ -23,7 +24,8 @@ std::shared_ptr<Mesh> MeshLoader::load(const std::vector<std::filesystem::path>
                                              aiProcess_FlipUVs | aiProcess_ValidateDataStructure | aiProcess_SortByPType | aiProcess_GenSmoothNormals
                                                  | aiProcess_CalcTangentSpace | aiProcess_Triangulate | aiProcess_PreTransformVertices);
 
-    if (scene->mNumMeshes < 1) {
+    if (scene == nullptr || scene->mNumMeshes < 1) {
+        Logger::Log(Logger::ERROR, "IO", "Could not load mesh '%s': %s", file[0].string().c_str(), importer.GetErrorString());
         return nullptr;
     }
     MeshData data;

ICE/IO/src/ModelLoader.cpp

@@ -5,23 +5,35 @@
 #include "ModelLoader.h"
 
 #include <AssetBank.h>
+#include <Logger.h>
 #include <Material.h>
 #include <assimp/postprocess.h>
 #include <assimp/scene.h>
 
+#include <cstdlib>
+#include <cstring>
+
 #include <assimp/Importer.hpp>
 #include <cassert>
 #include <iostream>
 
 namespace ICE {
 std::shared_ptr<Model> ModelLoader::load(const std::vector<std::filesystem::path> &file) {
+    if (file.empty()) {
+        return nullptr;
+    }
     Assimp::Importer importer;
 
     const aiScene *scene =
         importer.ReadFile(file[0].string(),
                           aiProcess_OptimizeGraph | aiProcess_FlipUVs | aiProcess_ValidateDataStructure | aiProcess_SortByPType
                               | aiProcess_GenSmoothNormals | aiProcess_CalcTangentSpace | aiProcess_Triangulate | aiProcess_LimitBoneWeights);
 
+    if (scene == nullptr || scene->mRootNode == nullptr) {
+        Logger::Log(Logger::ERROR, "IO", "Could not load model '%s': %s", file[0].string().c_str(), importer.GetErrorString());
+        return nullptr;
+    }
+
     std::vector<AssetUID> meshes;
     std::vector<AssetUID> materials;
     std::vector<Model::Node> nodes;
@@ -204,15 +216,23 @@ AssetUID ModelLoader::extractTexture(const aiMaterial *material, const std::stri
             void *data2 = nullptr;
             int width = texture->mWidth;
             int height = texture->mHeight;
-            int channels = 3;
+            int channels = 4;
             if (height == 0) {
-                //Compressed memory, use stbi to load
+                // Compressed in memory: decode with stbi into an owned RGBA buffer.
                 data2 = stbi_load_from_memory(data, texture->mWidth, &width, &height, &channels, 4);
                 channels = 4;
             } else {
-                data2 = data;
+                // Uncompressed aiTexel (BGRA/RGBA8888) data lives in the aiScene, which is
+                // freed when this Importer is destroyed -- and the GPU upload happens later.
+                // Copy it into an owned buffer so the Texture2D remains valid.
+                size_t byte_size = static_cast<size_t>(width) * static_cast<size_t>(height) * 4;
+                data2 = malloc(byte_size);
+                if (data2 != nullptr) {
+                    memcpy(data2, data, byte_size);
+                }
             }
-            auto texture_ice = std::make_shared<Texture2D>(data2, width, height, getTextureFormat(type, channels));
+            // take_ownership=true: both branches produce a free-compatible (stbi/malloc) buffer.
+            auto texture_ice = std::make_shared<Texture2D>(data2, width, height, getTextureFormat(type, channels), true);
             if (tex_id = ref_bank.getUID(AssetPath::WithTypePrefix<Texture2D>(tex_path)); tex_id != 0) {
                 ref_bank.removeAsset(AssetPath::WithTypePrefix<Texture2D>(tex_path));
                 ref_bank.addAssetWithSpecificUID(AssetPath::WithTypePrefix<Texture2D>(tex_path), texture_ice, tex_id);

ICE/IO/test/ModelLoaderTest.cpp

@@ -1,5 +1,3 @@
-#define STB_IMAGE_IMPLEMENTATION
-
 #include <AssetBank.h>
 #include <gtest/gtest.h>
 

ICE/Math/src/AABB.cpp

@@ -9,6 +9,11 @@ namespace ICE {
 AABB::AABB(const Eigen::Vector3f &min, const Eigen::Vector3f &max) : AABB(std::vector<Eigen::Vector3f>{min, max}) {
 }
 AABB::AABB(const std::vector<Eigen::Vector3f> &points) {
+    if (points.empty()) {
+        min = max = Eigen::Vector3f::Zero();
+        precomputeCenterAndExtent();
+        return;
+    }
     min = points[0];
     max = points[0];
     for (const auto &v : points) {

ICE/Platform/FileUtils.cpp

@@ -24,6 +24,10 @@ const std::string FileUtils::openFolderDialog() {
 
 const std::string FileUtils::readFile(const std::string &path) {
     std::ifstream ifs(path);
+    if (!ifs.is_open()) {
+        Logger::Log(Logger::ERROR, "Platform", "Could not open file for reading: %s", path.c_str());
+        return std::string();
+    }
     std::string content((std::istreambuf_iterator<char>(ifs)), (std::istreambuf_iterator<char>()));
     return content;
 }

ICE/Platform/Win32/dialog.cpp

@@ -80,20 +80,26 @@ const std::string open_native_folder_dialog() {
 		// Get the folder name
 		::IShellItem* shellItem(NULL);
 		result = fileDialog->GetResult(&shellItem);
-		if (!SUCCEEDED(result))
+		if (!SUCCEEDED(result) || shellItem == NULL)
 		{
-			shellItem->Release();
+			// GetResult failed: shellItem is NULL, so do not call Release on it.
 			goto end;
 		}
 		wchar_t* path = NULL;
 		result = shellItem->GetDisplayName(SIGDN_DESKTOPABSOLUTEPARSING, &path);
-		if (!SUCCEEDED(result))
+		if (SUCCEEDED(result) && path != NULL)
 		{
-			shellItem->Release();
-			goto end;
+			// Convert UTF-16 -> UTF-8 properly (std::string(ws.begin(), ws.end())
+			// truncated each wide char and mangled any non-ASCII path).
+			int bytes = WideCharToMultiByte(CP_UTF8, 0, path, -1, NULL, 0, NULL, NULL);
+			if (bytes > 1)
+			{
+				std::string utf8(bytes - 1, '\0');
+				WideCharToMultiByte(CP_UTF8, 0, path, -1, utf8.data(), bytes, NULL, NULL);
+				str = std::move(utf8);
+			}
+			CoTaskMemFree(path);
 		}
-		std::wstring ws(path);
-		str = std::string(ws.begin(), ws.end());
 		shellItem->Release();
 	}
 end:

ICE/Scene/src/Scene.cpp

@@ -117,6 +117,9 @@ void Scene::addEntity(Entity e, const std::string &alias, Entity parent) {
 }
 
 void Scene::removeEntity(Entity e) {
+    if (!hasEntity(e)) {
+        return;
+    }
     registry->removeEntity(e);
     aliases.erase(e);
     m_graph->removeEntity(e);

ICE/System/include/Registry.h

@@ -53,6 +53,10 @@ class Registry {
 
     void removeEntity(Entity e) {
         auto it = std::find(entities.begin(), entities.end(), e);
+        if (it == entities.end()) {
+            // Not a live entity: erase(end()) would be undefined behavior.
+            return;
+        }
         entities.erase(it);
         componentManager.entityDestroyed(e);
         entityManager.releaseEntity(e);
@@ -71,6 +75,12 @@ class Registry {
         return componentManager.getComponent<T>(e);
     }
 
+    // Returns nullptr instead of asserting when the entity has no component of type T.
+    template<typename T>
+    T *tryGetComponent(Entity e) {
+        return componentManager.tryGetComponent<T>(e);
+    }
+
     template<typename T>
     void addComponent(Entity e, T component) {
         componentManager.addComponent<T>(e, component);

ICE/System/src/AnimationSystem.cpp

@@ -194,6 +194,11 @@ Eigen::Vector3f AnimationSystem::interpolateScale(double timeInTicks, const Bone
 }
 
 Eigen::Quaternionf AnimationSystem::interpolateRotation(double time, const BoneAnimation& track) {
+    // Empty track: nothing to interpolate. Without this guard, rotations.size() - 1 wraps
+    // to SIZE_MAX and findKeyIndex reads out of bounds.
+    if (track.rotations.empty()) {
+        return Eigen::Quaternionf::Identity();
+    }
     if (track.rotations.size() == 1) {
         return track.rotations[0].rotation;
     }
@@ -205,6 +210,9 @@ Eigen::Quaternionf AnimationSystem::interpolateRotation(double time, const BoneA
     const auto& nextKey = track.rotations[nextIndex];
 
     double totalTime = nextKey.timeStamp - startKey.timeStamp;
+    if (totalTime == 0.0) {
+        return startKey.rotation;
+    }
     double factor = (time - startKey.timeStamp) / totalTime;
 
     Eigen::Quaternionf finalQuat = startKey.rotation.slerp((float) factor, nextKey.rotation);

ICE/System/src/RenderSystem.cpp

@@ -84,12 +84,16 @@ void RenderSystem::update(double delta) {
         if (m_registry->entityHasComponent<SkinningComponent>(e)) {
             const auto &skinning = m_gpu_bank->getMeshSkinningData(rc->mesh);
             auto skeleton_entity = m_registry->getComponent<SkinningComponent>(e)->skeleton_entity;
-            auto pose = m_registry->getComponent<SkeletonPoseComponent>(skeleton_entity);
-            for (const auto &[id, ibm] : skinning.inverseBindMatrices) {
-                bone_matrices.try_emplace(id, pose->bone_transform[id] * ibm);
+            // The skeleton entity may be stale/invalid; probe instead of asserting so a
+            // bad reference skips skinning for this frame rather than dereferencing null.
+            auto pose = m_registry->tryGetComponent<SkeletonPoseComponent>(skeleton_entity);
+            auto skel_transform = m_registry->tryGetComponent<TransformComponent>(skeleton_entity);
+            if (pose && skel_transform) {
+                for (const auto &[id, ibm] : skinning.inverseBindMatrices) {
+                    bone_matrices.try_emplace(id, pose->bone_transform[id] * ibm);
+                }
+                model_mat = skel_transform->getWorldMatrix();
             }
-
-            model_mat = m_registry->getComponent<TransformComponent>(skeleton_entity)->getWorldMatrix();
         }
 
         std::unordered_map<AssetUID, std::shared_ptr<GPUTexture>> texs;