diff --git a/ICE/Assets/CMakeLists.txt b/ICE/Assets/CMakeLists.txt index 3800b05f..a87d41a8 100644 --- a/ICE/Assets/CMakeLists.txt +++ b/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 diff --git a/ICE/Assets/include/Texture.h b/ICE/Assets/include/Texture.h index a43d83c6..146edbff 100644 --- a/ICE/Assets/include/Texture.h +++ b/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"; } diff --git a/ICE/Assets/src/Texture.cpp b/ICE/Assets/src/Texture.cpp index 5a832260..d478954d 100644 --- a/ICE/Assets/src/Texture.cpp +++ b/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; diff --git a/ICE/Assets/src/stb_image_impl.cpp b/ICE/Assets/src/stb_image_impl.cpp new file mode 100644 index 00000000..c7f87a0e --- /dev/null +++ b/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 as a declaration-only header, so +// the implementation must be defined here exactly once to avoid duplicate symbols. +#define STB_IMAGE_IMPLEMENTATION +#include diff --git a/ICE/Assets/test/AssetBankTest.cpp b/ICE/Assets/test/AssetBankTest.cpp index 83d47857..63e3df41 100644 --- a/ICE/Assets/test/AssetBankTest.cpp +++ b/ICE/Assets/test/AssetBankTest.cpp @@ -1,7 +1,6 @@ // // Created by Thomas Ibanez on 24.02.21. // -#define STB_IMAGE_IMPLEMENTATION #include #include "AssetBank.h" diff --git a/ICE/Components/include/Component.h b/ICE/Components/include/Component.h index f60fae36..36b6bc5e 100644 --- a/ICE/Components/include/Component.h +++ b/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 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()->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 + T* tryGetComponent(Entity entity) { + auto it = componentArrays.find(typeid(T)); + if (it == componentArrays.end()) { + return nullptr; + } + return std::static_pointer_cast>(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 std::shared_ptr> getComponentArray() { - auto const& type = typeid(T); - return std::static_pointer_cast>(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>(componentArrays.at(typeid(T))); } }; } // namespace ICE diff --git a/ICE/Core/src/ICEEngine.cpp b/ICE/Core/src/ICEEngine.cpp index 1d54da41..cd84d6c9 100644 --- a/ICE/Core/src/ICEEngine.cpp +++ b/ICE/Core/src/ICEEngine.cpp @@ -1,5 +1,3 @@ -#define STB_IMAGE_IMPLEMENTATION - #include "ICEEngine.h" #include diff --git a/ICE/Graphics/include/RenderCommand.h b/ICE/Graphics/include/RenderCommand.h index b6ff5843..ecf7cfc3 100644 --- a/ICE/Graphics/include/RenderCommand.h +++ b/ICE/Graphics/include/RenderCommand.h @@ -31,10 +31,11 @@ struct RenderCommand { // Bone matrices - pointer to avoid copying large map const std::unordered_map* 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* instance_data = nullptr; diff --git a/ICE/Graphics/src/ForwardRenderer.cpp b/ICE/Graphics/src/ForwardRenderer.cpp index acba0b67..f0cbc09e 100644 --- a/ICE/Graphics/src/ForwardRenderer.cpp +++ b/ICE/Graphics/src/ForwardRenderer.cpp @@ -6,6 +6,8 @@ #include #include + +#include #include #include #include @@ -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(MAX_LIGHTS)); + light_ubo_data.light_count = static_cast(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; diff --git a/ICE/GraphicsAPI/OpenGL/src/OpenGLShader.cpp b/ICE/GraphicsAPI/OpenGL/src/OpenGLShader.cpp index 679bcf1d..24b6c876 100644 --- a/ICE/GraphicsAPI/OpenGL/src/OpenGLShader.cpp +++ b/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(stage)); + return GL_VERTEX_SHADER; } } // namespace ICE \ No newline at end of file diff --git a/ICE/IO/src/MeshLoader.cpp b/ICE/IO/src/MeshLoader.cpp index f338bb7d..9a04d47d 100644 --- a/ICE/IO/src/MeshLoader.cpp +++ b/ICE/IO/src/MeshLoader.cpp @@ -5,6 +5,7 @@ #include "MeshLoader.h" #include +#include #include #include #include @@ -23,7 +24,8 @@ std::shared_ptr MeshLoader::load(const std::vector 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; diff --git a/ICE/IO/src/ModelLoader.cpp b/ICE/IO/src/ModelLoader.cpp index cf453a5b..ac2005ff 100644 --- a/ICE/IO/src/ModelLoader.cpp +++ b/ICE/IO/src/ModelLoader.cpp @@ -5,16 +5,23 @@ #include "ModelLoader.h" #include +#include #include #include #include +#include +#include + #include #include #include namespace ICE { std::shared_ptr ModelLoader::load(const std::vector &file) { + if (file.empty()) { + return nullptr; + } Assimp::Importer importer; const aiScene *scene = @@ -22,6 +29,11 @@ std::shared_ptr ModelLoader::load(const std::vectormRootNode == nullptr) { + Logger::Log(Logger::ERROR, "IO", "Could not load model '%s': %s", file[0].string().c_str(), importer.GetErrorString()); + return nullptr; + } + std::vector meshes; std::vector materials; std::vector 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(width) * static_cast(height) * 4; + data2 = malloc(byte_size); + if (data2 != nullptr) { + memcpy(data2, data, byte_size); + } } - auto texture_ice = std::make_shared(data2, width, height, getTextureFormat(type, channels)); + // take_ownership=true: both branches produce a free-compatible (stbi/malloc) buffer. + auto texture_ice = std::make_shared(data2, width, height, getTextureFormat(type, channels), true); if (tex_id = ref_bank.getUID(AssetPath::WithTypePrefix(tex_path)); tex_id != 0) { ref_bank.removeAsset(AssetPath::WithTypePrefix(tex_path)); ref_bank.addAssetWithSpecificUID(AssetPath::WithTypePrefix(tex_path), texture_ice, tex_id); diff --git a/ICE/IO/test/ModelLoaderTest.cpp b/ICE/IO/test/ModelLoaderTest.cpp index db406ab0..8ecef875 100644 --- a/ICE/IO/test/ModelLoaderTest.cpp +++ b/ICE/IO/test/ModelLoaderTest.cpp @@ -1,5 +1,3 @@ -#define STB_IMAGE_IMPLEMENTATION - #include #include diff --git a/ICE/Math/src/AABB.cpp b/ICE/Math/src/AABB.cpp index 3a9cc960..81040a04 100644 --- a/ICE/Math/src/AABB.cpp +++ b/ICE/Math/src/AABB.cpp @@ -9,6 +9,11 @@ namespace ICE { AABB::AABB(const Eigen::Vector3f &min, const Eigen::Vector3f &max) : AABB(std::vector{min, max}) { } AABB::AABB(const std::vector &points) { + if (points.empty()) { + min = max = Eigen::Vector3f::Zero(); + precomputeCenterAndExtent(); + return; + } min = points[0]; max = points[0]; for (const auto &v : points) { diff --git a/ICE/Platform/FileUtils.cpp b/ICE/Platform/FileUtils.cpp index 0144d606..24268781 100644 --- a/ICE/Platform/FileUtils.cpp +++ b/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(ifs)), (std::istreambuf_iterator())); return content; } diff --git a/ICE/Platform/Win32/dialog.cpp b/ICE/Platform/Win32/dialog.cpp index 58370e98..8e2cb877 100644 --- a/ICE/Platform/Win32/dialog.cpp +++ b/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: diff --git a/ICE/Scene/src/Scene.cpp b/ICE/Scene/src/Scene.cpp index 2a0b0912..63172fde 100644 --- a/ICE/Scene/src/Scene.cpp +++ b/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); diff --git a/ICE/System/include/Registry.h b/ICE/System/include/Registry.h index 4d923d7e..00f361a9 100644 --- a/ICE/System/include/Registry.h +++ b/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(e); } + // Returns nullptr instead of asserting when the entity has no component of type T. + template + T *tryGetComponent(Entity e) { + return componentManager.tryGetComponent(e); + } + template void addComponent(Entity e, T component) { componentManager.addComponent(e, component); diff --git a/ICE/System/src/AnimationSystem.cpp b/ICE/System/src/AnimationSystem.cpp index 3c109e4e..24d69ebb 100644 --- a/ICE/System/src/AnimationSystem.cpp +++ b/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); diff --git a/ICE/System/src/RenderSystem.cpp b/ICE/System/src/RenderSystem.cpp index 98584812..2bc83e28 100644 --- a/ICE/System/src/RenderSystem.cpp +++ b/ICE/System/src/RenderSystem.cpp @@ -84,12 +84,16 @@ void RenderSystem::update(double delta) { if (m_registry->entityHasComponent(e)) { const auto &skinning = m_gpu_bank->getMeshSkinningData(rc->mesh); auto skeleton_entity = m_registry->getComponent(e)->skeleton_entity; - auto pose = m_registry->getComponent(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(skeleton_entity); + auto skel_transform = m_registry->tryGetComponent(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(skeleton_entity)->getWorldMatrix(); } std::unordered_map> texs;