feature: NVRHI #19
feature: NVRHI #19
Conversation
I have to figure out how to implement it...
sheazywi
added
enhancement
There was a problem hiding this comment.
Pull Request Overview
Integrate the NVRHI rendering abstraction into the build system and update the ImGui layer to target a newer GLSL version.
- Add NVRHI as a submodule and include path.
- Update premake scripts to include and link NVRHI for both engine and editor.
- Bump ImGui OpenGL backend shader version from 410 to 450.
Reviewed Changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| premake5.lua | Register new “Dependencies – NVRHI” group |
| StarEngine/src/StarEngine/ImGui/ImGuiLayer.cpp | Updated ImGui OpenGL GLSL version to 450 |
| StarEngine/premake5.lua | Added NVRHI include dir and linked libraries |
| StarEditor/premake5.lua | Added NVRHI include directory for the editor |
| Dependencies.lua | Added IncludeDir["nvrhi"] path |
| .gitmodules | Added NVRHI submodule entry |
Comments suppressed due to low confidence (2)
StarEditor/premake5.lua:26
- The editor project includes the NVRHI header path but does not link the NVRHI library. You should add "nvrhi" to the
linksblock so the editor can resolve its symbols.
%{IncludeDir.nvrhi},
premake5.lua:37
- [nitpick] The indentation for this
includeline doesn’t match the single-tab style used elsewhere. Align it with the other dependency entries for consistency.
include "StarEngine/vendor/nvrhi"
| @@ -67,7 +67,7 @@ namespace StarEngine { | |||
|
|
|||
| // Setup Platform/Renderer bindings | |||
| ImGui_ImplGlfw_InitForOpenGL(window, true); | |||
| ImGui_ImplOpenGL3_Init("#version 410"); | |||
| ImGui_ImplOpenGL3_Init("#version 450"); | |||
There was a problem hiding this comment.
[nitpick] Hardcoding "#version 450" enforces a minimum OpenGL 4.5 context and may break compatibility on older hardware. Consider making the GLSL version configurable or document this requirement.
| ImGui_ImplOpenGL3_Init("#version 450"); | |
| const char* glslVersion = "#version 450"; // Default GLSL version | |
| // Optionally, retrieve GLSL version dynamically or make it configurable | |
| ImGui_ImplOpenGL3_Init(glslVersion); |
Copilot uses AI. Check for mistakes.
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughThis PR overhauls the asset pipeline, serialization, and core application systems; adds threaded editor/runtime asset subsystems; introduces many new asset serializers, registry and manager types; removes large swaths of legacy OpenGL and Windows platform code; and updates build, tooling, and editor integration files. Changes
Sequence Diagram(s)sequenceDiagram
participant App as Application
participant AM as AssetManager (Editor/Runtime)
participant AS as AssetSystem (Asset Thread)
participant S as Serializer
participant FS as FileSystem
App->>AM: Request GetAsset(handle)
alt Asset already loaded
AM-->>App: Return asset
else
AM->>AS: QueueAssetLoad(metadata)
AS->>S: TryLoadData(metadata)
S->>FS: Read file / stream
FS-->>S: File data
S-->>AS: Construct Asset
AS-->>AM: Notify loaded
AM-->>App: Asset available (sync/async)
end
sequenceDiagram
participant Main as MainThread
participant Render as RenderThread
participant Events as EventQueue
participant Layers as LayerStack
loop Frame
Main->>Events: ProcessEvents()
Events->>Layers: Dispatch events (reverse)
Main->>Layers: OnUpdate(timestep)
Main->>Render: Submit render commands
Render-->>Main: Render complete / Present
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120+ minutes Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 75
🧹 Nitpick comments (71)
StarEngine/src/StarEngine/Core/Delegate.h (3)
25-26: Fix typo in comment."functiondelegate" should be "function delegate".
- /** Simple functiondelegate to bind a callback of some sort without + /** Simple function delegate to bind a callback of some sort without unnecessary allocations. Faster than std::function.
171-175: Consider renaming to BindLambda for API consistency.The
Delegateclass usesBindLambdafor lambda binding, butMulticastDelegateusesBind. Consider using consistent naming.template<class TLambda> -void Bind(const TLambda& lambda) +void BindLambda(const TLambda& lambda) { Add((TDelegate::TInstancePtr)(&lambda), LambdaStub<TLambda>); }
289-289: Consider using std::vector for better cache locality.
std::listhas poor cache locality for iteration. Since delegates are likely invoked more frequently than modified, consider usingstd::vectorwhich would improve invocation performance.StarEditor/src/StarEditorApp.cpp (1)
28-31: Consider implementing or removing commented configuration parameters.These commented parameters suggest planned functionality but leaving commented code in production can lead to maintenance issues. Consider either:
- Implementing the configuration parameters if they're needed soon
- Removing them and tracking the feature request elsewhere
- Adding a TODO comment explaining the timeline for implementation
StarEngine/src/StarEngine/Audio/AudioSource.cpp (1)
105-105: Consider removing profiling from simple helper function.The
GetAttenuationModelfunction is a simple switch statement that might not benefit from profiling overhead. Additionally, the profiling name format is inconsistent (includes return type) compared to other functions.Consider removing profiling from this simple function:
- SE_PROFILE_FUNCTION("ma_attenuation_model GetAttenuationModel");Or if profiling is desired, use consistent naming:
- SE_PROFILE_FUNCTION("ma_attenuation_model GetAttenuationModel"); + SE_PROFILE_FUNCTION("GetAttenuationModel");StarEngine-ScriptCore/premake5.lua (1)
3-4: Address commented Coral integration code.The commented-out Coral.Managed integration suggests incomplete migration. Consider either completing the integration or removing the commented code.
Would you like me to help determine if these Coral references should be removed or if the integration needs to be completed?
Also applies to: 14-14, 16-16
StarEngine/src/StarEngine/Asset/AssetExtensions.h (2)
31-36: Consider additional texture formats.The texture format coverage is good but consider adding support for additional common formats like .tga, .bmp, .dds, .ktx for broader compatibility.
// Textures { ".png", AssetType::Texture }, { ".jpg", AssetType::Texture }, { ".jpeg", AssetType::Texture }, +{ ".tga", AssetType::Texture }, +{ ".bmp", AssetType::Texture }, +{ ".dds", AssetType::Texture }, { ".hdr", AssetType::EnvMap },
37-40: Consider additional audio formats.Consider adding support for more audio formats like .mp3, .flac, .m4a for broader audio asset compatibility.
// Audio { ".wav", AssetType::Audio }, { ".ogg", AssetType::Audio }, +{ ".mp3", AssetType::Audio }, +{ ".flac", AssetType::Audio }, +{ ".m4a", AssetType::Audio },StarEditor/src/Panels/ContentBrowserPanel.cpp (1)
239-239: Consider removing commented-out code.While the pointer type update is consistent, the large block of commented code (lines 228-273) should be removed if it's no longer needed to keep the codebase clean.
StarEngine-ScriptCore/premake5-dotnet.lua (2)
31-33: Clean up commented code or document intent.The commented lines suggest unfinished configuration. Either implement these settings or remove them to avoid confusion.
If these are intentionally disabled, add a comment explaining why:
- --linkAppReferences(false) - - --links { "Coral.Managed" } + -- Note: App reference linking disabled for standalone assembly + -- linkAppReferences(false) + + -- Note: Coral.Managed linked via include instead of direct reference + -- links { "Coral.Managed" }
44-44: Add missing final newline.File should end with a newline for POSIX compliance.
} +StarEngine/src/StarEngine/Asset/MeshRuntimeSerializer.h (1)
12-17: Consider adding documentation and const correctness.The class interface is clean but could benefit from documentation and const correctness improvements.
+ /** + * Runtime serializer for mesh assets, handling serialization to/from asset packs. + */ class MeshRuntimeSerializer { public: + /** + * Serializes a mesh asset to an asset pack. + * @param handle Asset handle to serialize + * @param stream Output stream writer + * @param outInfo Serialization metadata output + * @return true if serialization succeeded + */ - bool SerializeToAssetPack(AssetHandle handle, FileStreamWriter& stream, AssetSerializationInfo& outInfo); + bool SerializeToAssetPack(AssetHandle handle, FileStreamWriter& stream, AssetSerializationInfo& outInfo) const; + + /** + * Deserializes a mesh asset from an asset pack. + * @param stream Input stream reader + * @param assetInfo Asset metadata from pack + * @return Deserialized mesh asset or nullptr on failure + */ Ref<Asset> DeserializeFromAssetPack(FileStreamReader& stream, const AssetPackFile::AssetInfo& assetInfo); };StarEditor/Resources/LUA/StarEngine.lua (1)
27-33: Consider using path.normalize for cross-platform compatibility.The manual path separator replacement might be error-prone. Consider using premake's built-in path utilities.
if is_windows then - monoLibsPath = path.join(starengineDir, "StarEditor", "mono", "lib", "mono", "4.5"):gsub("/", "\\") - monoLibsFacadesPath = path.join(monoLibsPath, "Facades"):gsub("/", "\\") + monoLibsPath = path.normalize(path.join(starengineDir, "StarEditor", "mono", "lib", "mono", "4.5")) + monoLibsFacadesPath = path.normalize(path.join(monoLibsPath, "Facades")) else monoLibsPath = path.join(starengineDir, "StarEditor", "mono", "linux", "lib", "mono", "4.5") monoLibsFacadesPath = path.join(monoLibsPath, "Facades") end.gitmodules (1)
1-70: Excellent submodule standardization with minor consistency issues.The standardization to HTTPS URLs and addition of
shallow=trueandignore=dirtysettings is excellent for performance and consistency. However, there are a few inconsistencies to address:
backward-cppandJoltPhysicsare missing theshallow=truesetting- Consider adding
ignore=dirtytobackward-cppfor consistency[submodule "StarEngine/vendor/backward-cpp"] path = StarEngine/vendor/backward-cpp url = https://github.com/bombela/backward-cpp + ignore = dirty + shallow = true [submodule "StarEngine/vendor/JoltPhysics/JoltPhysics"] path = StarEngine/vendor/JoltPhysics/JoltPhysics url = https://github.com/jrouwe/JoltPhysics + ignore = dirty + shallow = trueDockerfile (1)
1-44: Well-structured Dockerfile with room for optimization.The Dockerfile follows good practices with version pinning for critical dependencies and selective copying. Consider these improvements:
- Pin package versions for reproducibility:
- RUN apt-get install -y build-essential clang mold llvm libtbb-dev + RUN apt-get install -y build-essential=12.9ubuntu3 clang=1:14.0-55~exp2 mold=1.11.0+dfsg-1 llvm=1:14.0-55~exp2 libtbb-dev=2021.5.0-7ubuntu2
- Optimize layer caching by combining apt commands:
-RUN apt-get update - -RUN mkdir -p /workspace +RUN apt-get update && mkdir -p /workspace
- Clean apt cache to reduce image size:
- RUN apt-get install -y libdw-dev libunwind-dev + RUN apt-get install -y libdw-dev libunwind-dev && \ + apt-get clean && \ + rm -rf /var/lib/apt/lists/*StarEngine/src/StarEngine/Asset/AssetRegistry.h (1)
17-20: Consider safer API design for Get() method.The
Get()method returns a const reference, which could throw an exception if the handle doesn't exist. Consider returning an optional or adding a safe getter.// note: no non-const Get() function. If you need to modify the metadata, use Set(). // This aids correct usage in a multi-threaded environment. const AssetMetadata& Get(const AssetHandle handle) const; + const AssetMetadata* TryGet(const AssetHandle handle) const; + std::optional<AssetMetadata> GetOptional(const AssetHandle handle) const; void Set(const AssetHandle handle, const AssetMetadata& metadata);StarEngine/src/StarEngine/Asset/AssetRegistry.cpp (2)
36-40: Minor: Inconsistent logging in Remove method.Line 38 has a formatting inconsistency - missing format specifier for the handle parameter.
- ASSET_LOG("Removing handle", handle); + ASSET_LOG("Removing handle {}", handle);
8-13: Consider making logging configurable at runtime.The current compile-time logging toggle is inflexible for debugging production issues. Consider using a runtime configuration or log level system.
-#define SE_ASSETREGISTRY_LOG 0 -#if SE_ASSETREGISTRY_LOG -#define ASSET_LOG(...) SE_CORE_TRACE_TAG("ASSET", __VA_ARGS__) -#else -#define ASSET_LOG(...) -#endif +#define ASSET_LOG(...) SE_CORE_TRACE_TAG("ASSET", __VA_ARGS__)And control logging through the existing log level system instead.
.editorconfig (1)
1-314: Consider simplifying the configuration for maintainability.This is an extremely comprehensive configuration with 300+ lines of detailed formatting rules. While thorough, such extensive configuration can be:
- Hard to maintain and update
- Potentially conflicting with IDE defaults
- Overwhelming for new contributors
Consider focusing on the most critical formatting rules (indentation, braces, naming) and relying on IDE defaults for less critical styling preferences. This would improve maintainability while still ensuring consistency.
StarEngine/Platform/Windows/WindowsProcessHelper.cpp (1)
4-5: Address deprecated std::codecvt usage.While the comment acknowledges the deprecation, consider migrating to modern alternatives like
std::formatwith wide string support or Windows-specific conversion functions (MultiByteToWideChar) for better long-term maintainability.// Alternative using Windows API: std::wstring ConvertUtf8ToWide(const std::string& utf8str) { if (utf8str.empty()) return std::wstring(); int size = MultiByteToWideChar(CP_UTF8, 0, utf8str.c_str(), -1, nullptr, 0); std::wstring result(size - 1, 0); MultiByteToWideChar(CP_UTF8, 0, utf8str.c_str(), -1, &result[0], size); return result; }StarEngine/src/StarEngine/Asset/MeshSourceFile.h (2)
19-41: Add validation or checksum fields to Metadata.For binary file format integrity, consider adding validation fields such as checksums or magic numbers to detect corruption.
struct Metadata { uint32_t Flags; + uint32_t Checksum; // CRC32 or similar AABB BoundingBox; // ... existing fields };
45-46: Consider making version non-const for runtime modification.Having a const version field with inline initialization is unusual for a file format structure that might need version updates at runtime.
- const char HEADER[4] = { 'S','E','M','S' }; - uint32_t Version = 1; + char HEADER[4] = { 'S','E','M','S' }; + uint32_t Version = 1;StarEngine/src/StarEngine/Core/ApplicationSettings.h (3)
17-25: Consider using string_view consistently for all string parametersWhile
HasKey,Get*, andSetmethods usestd::string_viewfor the key parameter, theGetmethod usesconst std::string&for the default value. For consistency and to avoid unnecessary string copies, consider usingstd::string_viewfor all string parameters where the strings are not stored.- std::string Get(std::string_view name, const std::string& defaultValue = "") const; + std::string Get(std::string_view name, std::string_view defaultValue = "") const;
28-28: Consider using unordered_map for better lookup performanceSince the settings are accessed by key without need for ordering,
std::unordered_mapwould provide O(1) average-case lookup performance compared to O(log n) forstd::map.- std::map<std::string, std::string> m_Settings; + std::unordered_map<std::string, std::string> m_Settings;Also add the required header:
#include <map> +#include <unordered_map>
9-29: Document thread safety guaranteesThe class doesn't appear to have any synchronization mechanisms. If this class is intended to be used from multiple threads, consider adding mutex protection or clearly document that it's not thread-safe.
Consider either:
- Adding a mutable mutex member and protecting all public methods
- Documenting that the class is not thread-safe and users must synchronize access
- Creating a thread-safe wrapper if multi-threaded access is needed
StarEngine/src/StarEngine/Asset/MeshColliderAsset.h (2)
9-14: Consider making the enum class explicitly sizedWhile
uint8_tis specified, it's good practice to be explicit about the underlying type in the enum declaration itself for better portability.- enum class ECollisionComplexity : uint8_t + enum class ECollisionComplexity : std::uint8_tAlso ensure
<cstdint>is included for the standard integer types.
19-32: Consider adding validation for physics parametersSeveral parameters have constraints that aren't enforced:
VertexWeldToleranceandAreaTestEpsilonshould be non-negative- Scale vectors should have positive components (unless negative scaling is intentional)
Consider either:
- Making members private with validated setters
- Adding a
Validate()method that checks parameter validity- Using a builder pattern for construction with validation
Example validation method:
bool Validate() const { return VertexWeldTolerance >= 0.0f && AreaTestEpsilon >= 0.0f && ColliderScale.x > 0.0f && ColliderScale.y > 0.0f && ColliderScale.z > 0.0f; }StarEngine/src/StarEngine/Asset/AssetSystem/RuntimeAssetSystem.h (2)
51-59: Document locking order to prevent deadlocksWith multiple mutexes (
m_AssetLoadingQueueMutex,m_LoadedAssetsMutex,m_AMLoadedAssetsMutex), there's a risk of deadlock if they're acquired in different orders in different methods.Document and enforce a consistent locking order:
// Locking order (to prevent deadlocks): // 1. m_AssetLoadingQueueMutex // 2. m_LoadedAssetsMutex // 3. m_AMLoadedAssetsMutex // Never acquire a lower-numbered mutex while holding a higher-numbered one
19-26: Consider adding thread safety assertionsThe comments indicate that
GetAssetshould be called on the asset thread, but there's no runtime verification. Consider adding debug assertions to catch incorrect usage.// Add a member to track the asset thread ID private: std::thread::id m_AssetThreadId; // In GetAsset: Ref<Asset> GetAsset(const AssetHandle sceneHandle, const AssetHandle assetHandle) { SE_CORE_ASSERT(std::this_thread::get_id() == m_AssetThreadId, "GetAsset must be called from asset thread"); // ... rest of implementation }StarEngine/src/StarEngine/Asset/AssetMetadata.h (1)
22-23: Address the TODO comment for FileLastWriteTime.The TODO indicates uncertainty about the semantics of this field. Consider documenting whether this tracks the file's modification time when loaded or the current file system timestamp.
Would you like me to help clarify the documentation for this field or open an issue to track this?
StarEditor/src/EditorLayer.cpp (1)
700-700: Update commented file extension.The commented code still references
.hazelextension while the active code uses.starscene. Consider updating or removing this outdated comment.- // std::string filepath = FileDialogs::OpenFile("StarEngine Scene (*.hazel)\0*.hazel\0"); + // std::string filepath = FileDialogs::OpenFile("StarEngine Scene (*.starscene)\0*.starscene\0");StarEngine/src/StarEngine/Core/Events/ApplicationEvent.h (1)
9-31: Consider implementing the TODO suggestion for storing previous size.Storing the previous window size could be valuable for:
- Calculating size deltas for smooth transitions
- Implementing window state restoration
- Debugging resize-related issues
This would make the event more informative without significant overhead.
Would you like me to implement this enhancement or create an issue to track it?
StarEngine/src/StarEngine/Asset/AssetTypes.h (1)
40-65: Consider adding validation for unknown asset types.The function silently returns
AssetType::Nonefor unknown types, which could hide errors during development.inline AssetType AssetTypeFromString(std::string_view assetType) { if (assetType == "None") return AssetType::None; // ... other cases ... if (assetType == "AnimationGraph") return AssetType::AnimationGraph; + SE_CORE_WARN("Unknown asset type: {0}", assetType); return AssetType::None; }StarEngine/src/StarEngine/Asset/MeshRuntimeSerializer.cpp (1)
10-12: Remove unused ACL includes.The ACL headers are included but the animation serialization code is commented out (lines 69-129, 229-257).
-#include <acl/core/compressed_tracks.h> -#include <acl/core/iallocator.h> -StarEngine/src/StarEngine/Core/Base.h (2)
7-12: Remove unnecessary semicolon after namespace closing brace.The semicolon on line 12 after the namespace closing brace is not needed in C++.
namespace StarEngine { void InitializeCore(); void ShutdownCore(); -}; +}
59-59: Remove misplaced comment.The comment "Pointer wrappers" on line 59 doesn't relate to the rounding functions. It should be removed as the actual pointer wrappers comment is correctly placed on line 66.
- // Pointer wrappers template<typename T> T RoundDown(T x, T fac) { return x / fac * fac; }StarEngine/src/StarEngine/Core/ApplicationSettings.cpp (2)
18-22: Consider handling deserialization failure in constructor.The constructor silently ignores deserialization failures. Consider logging a warning or providing a way for callers to check if initial loading succeeded.
ApplicationSettings::ApplicationSettings(const std::filesystem::path& filepath) : m_FilePath(filepath) { - Deserialize(); + if (!Deserialize()) + { + SE_CORE_WARN("Failed to load application settings from: {}", filepath.string()); + } }
72-74: Use string_view directly with C++20 heterogeneous lookup.Creating a temporary string in
HasKeyis inefficient. If using C++20, you can use heterogeneous lookup with string_view directly.bool ApplicationSettings::HasKey(std::string_view key) const { - return m_Settings.find(std::string(key)) != m_Settings.end(); + return m_Settings.find(key) != m_Settings.end(); }Note: This requires C++20 and the map to be declared with transparent comparator like
std::unordered_map<std::string, std::string, std::hash<std::string_view>, std::equal_to<>>.StarEngine/src/StarEngine/Asset/AssetSystem/EditorAssetSystem.h (3)
21-24: Fix typo in comment.There's a typo in the comment: "transfered" should be "transferred".
// Get an asset immediately (on asset thread). // If the asset needs to be loaded, it will be loaded into "ready assets" and transfered back to main thread -// at next asset sync. +// transferred back to main thread at next asset sync. Ref<Asset> GetAsset(const AssetMetadata& request);
50-51: Clarify the comment about m_Running initialization.The current comment is confusing. Consider rephrasing for clarity.
Thread m_Thread; -std::atomic<bool> m_Running = true; // not false. This ensures that if Stop() is called after the thread is dispatched but before it actually starts running, then the thread is correctly stopped. +std::atomic<bool> m_Running = true; // Initialize to true to ensure Stop() works correctly if called before thread starts
63-64: Consider documenting the purpose of m_AssetUpdatePerf.The
m_AssetUpdatePerfmember variable appears to track performance metrics but its usage is not clear from the header. Consider adding a comment explaining its purpose or removing it if unused.// Asset Monitoring +// Time taken (in ms) for the last asset monitoring update float m_AssetUpdatePerf = 0.0f;StarEngine/Platform/Windows/WindowsFileSystem.cpp (1)
7-10: Remove unnecessary GLFW includes.The GLFW headers appear to be unrelated to file system operations and should be removed.
-#include <GLFW/glfw3.h> - -#define GLFW_EXPOSE_NATIVE_WIN32 -#include <GLFW/glfw3native.h>StarEditor/premake5.lua (2)
19-20: Consider removing vendor headers from files list.Including all GLM headers in the files list may slow down IDE operations and isn't necessary for compilation. These files are already accessible via include paths.
- "../StarEngine/vendor/glm/glm/**.hpp", -
73-75: Consider CPU compatibility for ISA extensions.The Release configuration enables AVX2 and other ISA extensions that may not be available on all target CPUs. Consider making these configurable or detecting CPU capabilities at runtime.
Add a comment or make it configurable:
filter "configurations:Release" optimize "On" + -- Note: These ISA extensions require compatible CPUs (AVX2: Intel Haswell/AMD Excavator or newer) vectorextensions "AVX2" isaextensions { "BMI", "POPCNT", "LZCNT", "F16C" }StarEngine/premake5.lua (1)
45-46: Consider refactoring the PCH filter for maintainability.The filter expression is very long and could be difficult to maintain. Consider using a table-based approach:
- filter "files:vendor/FastNoise/**.cpp or files:vendor/yaml-cpp/src/**.cpp or files:vendor/imgui/misc/cpp/imgui_stdlib.cpp or files:src/StarEngine/Tiering/TieringSerializer.cpp or files:src/StarEngine/Core/ApplicationSettings.cpp" + local noPCHFiles = { + "vendor/FastNoise/**.cpp", + "vendor/yaml-cpp/src/**.cpp", + "vendor/imgui/misc/cpp/imgui_stdlib.cpp", + "src/StarEngine/Tiering/TieringSerializer.cpp", + "src/StarEngine/Core/ApplicationSettings.cpp" + } + filter("files:" .. table.concat(noPCHFiles, " or files:")) flags { "NoPCH" }StarEngine/src/StarEngine/Core/Assert.h (1)
18-18: Consider making verify macros conditional.
SE_ENABLE_VERIFYis always defined, which means verify checks will run even in distribution builds. This might impact performance.-#define SE_ENABLE_VERIFY +#ifndef SE_DIST +#define SE_ENABLE_VERIFY +#endifStarEngine/src/StarEngine/Asset/AssetManager/AssetManagerBase.h (2)
28-29: Consider using template methods for type safety.The
GetAssetandGetAssetAsyncmethods return genericAssetreferences. Consider adding template methods for type-safe asset retrieval:virtual Ref<Asset> GetAsset(AssetHandle assetHandle) = 0; virtual AsyncAssetResult<Asset> GetAssetAsync(AssetHandle assetHandle) = 0; + +template<typename T> +Ref<T> GetAsset(AssetHandle assetHandle) { + return std::dynamic_pointer_cast<T>(GetAsset(assetHandle)); +} + +template<typename T> +AsyncAssetResult<T> GetAssetAsync(AssetHandle assetHandle) { + // Implementation would need to handle type conversion for async results + return GetAssetAsync(assetHandle).Then([](Ref<Asset> asset) { + return std::dynamic_pointer_cast<T>(asset); + }); +}
19-63: Add documentation for the interface methods.The interface lacks documentation. Consider adding brief descriptions for each method to clarify their purpose and expected behavior, especially for methods with similar names like
IsAssetLoaded,IsAssetValid, andIsAssetMissing.StarEngine/src/StarEngine/Asset/AssetSystem/RuntimeAssetSystem.cpp (2)
9-13: Consider adding thread start verification.The thread is started in the constructor without checking if it started successfully. Consider adding error handling.
RuntimeAssetSystem::RuntimeAssetSystem() : m_Thread("Asset Thread") { - m_Thread.Dispatch([this]() { AssetThreadFunc(); }); + if (!m_Thread.Dispatch([this]() { AssetThreadFunc(); })) + { + SE_CORE_ERROR("Failed to start asset loading thread"); + throw std::runtime_error("Failed to start asset loading thread"); + } }
84-84: Improve readability of asset loading check.The comma operator in the if statement is valid but reduces readability.
- if (asset = m_AssetPack->LoadAsset(alr.SceneHandle, alr.Handle); asset) + asset = m_AssetPack->LoadAsset(alr.SceneHandle, alr.Handle); + if (asset)StarEngine/src/StarEngine/Core/Events/Event.h (1)
47-47: Document the purpose of the Synced flag.The
Syncedflag's purpose is explained in the comment but it would be clearer with a more descriptive name or additional documentation about when and how it should be set.Consider renaming to
ReadyForProcessingorAssetThreadSyncedfor clarity.StarEngine/Platform/Windows/WindowsRenderThread.cpp (1)
133-138: Thread identification logic is correctThe implementation correctly identifies the render thread. Consider enabling the verification during debug builds for better error detection.
StarEngine/src/StarEngine/Asset/Asset.h (2)
56-74: Address the TODO: AudioFile placementThe comment indicates
AudioFiledoesn't belong in this header. Consider moving it to a dedicated audio asset header file for better organization.Would you like me to create an issue to track moving AudioFile to a more appropriate location?
76-94: Consider making type conversions explicitThe implicit conversions to
Ref<T>andboolcould lead to unexpected behavior. Consider making at least the bool conversion explicit to avoid ambiguous conditions.- operator bool() const { return IsReady; } + explicit operator bool() const { return IsReady; }StarEngine/src/StarEngine/Asset/AssetSerializer.h (1)
35-79: Document rationale for empty Serialize methodsSeveral serializers (TextureSerializer, FontSerializer, EnvironmentSerializer) have empty Serialize implementations. Consider adding comments explaining why serialization is not needed for these asset types.
StarEngine/src/StarEngine/Core/Buffer.h (1)
140-146: Reuse base class Copy implementationThe static Copy method duplicates logic from the base Buffer::Copy.
static BufferSafe Copy(const void* data, uint64_t size) { BufferSafe buffer; - buffer.Allocate(size); - memcpy(buffer.Data, data, size); + Buffer temp = Buffer::Copy(data, size); + buffer.Data = temp.Data; + buffer.Size = temp.Size; + temp.Data = nullptr; // Prevent double delete return buffer; }StarEngine/src/StarEngine/Asset/AssetManager/RuntimeAssetManager.cpp (3)
51-51: Fix typo in comment- // If we're main thread, we can just loading the asset as normal + // If we're main thread, we can just load the asset as normal
124-134: Implement stubbed methodsBoth
EnsureAllLoadedCurrentandEnsureCurrentare stubbed with verification failures. These need proper implementation.Would you like me to help implement these methods or create an issue to track this TODO?
195-200: Implement GetAllAssetsWithType methodThis method is stubbed and needs implementation to return assets of the specified type.
Would you like me to help implement this method or create an issue to track this TODO?
StarEngine/src/StarEngine/Asset/AssetSystem/EditorAssetSystem.cpp (2)
85-90: Consider using sleep instead of condition variable with timeoutAs noted in the comment, the periodic timeout makes the condition variable somewhat redundant. Consider simplifying to a sleep with periodic queue checks.
- // need to wake periodically (here 100ms) so that AssetMonitorUpdate() is called regularly to check for updated file timestamps - // (which kinda makes condition variable redundant. may as well just sleep(100ms)) - m_AssetLoadingQueueCV.wait_for(lock, std::chrono::milliseconds(100), [this] { - return !m_Running || !m_AssetLoadingQueue.empty(); - }); + // Wake periodically (100ms) to check for updated file timestamps + lock.unlock(); + std::this_thread::sleep_for(std::chrono::milliseconds(100));
147-153: Consider using shared_lock for better concurrencySince you're only reading the loaded assets during iteration, consider using a shared_lock to allow concurrent reads.
- std::scoped_lock<std::mutex> lock(m_AMLoadedAssetsMutex); + std::shared_lock<std::shared_mutex> lock(m_AMLoadedAssetsMutex);Note: This would require changing
m_AMLoadedAssetsMutexfromstd::mutextostd::shared_mutex.StarEngine/src/StarEngine/Asset/AssetManager/RuntimeAssetManager.h (1)
12-12: Incomplete or unclear commentThe comment "// AssetPack" appears incomplete or unclear about its purpose.
Either remove it or expand it to explain the relationship with AssetPack.
StarEngine/src/StarEngine/Asset/MeshSerializer.cpp (5)
27-37: Consider adding error handling for element conversion failures.The decode function should handle potential conversion failures when reading individual elements to prevent runtime exceptions.
static bool decode(const Node& node, std::vector<uint32_t>& result) { if (!node.IsSequence()) return false; result.resize(node.size()); for (size_t i = 0; i < node.size(); i++) - result[i] = node[i].as<uint32_t>(); + { + try { + result[i] = node[i].as<uint32_t>(); + } catch (const YAML::Exception&) { + return false; + } + } return true; }
78-81: Improve readability of complex conditional logic.The conditional logic is difficult to read due to line length and nesting.
- if (auto meshSource = AssetManager::GetAsset<MeshSource>(mesh->GetMeshSource()); meshSource && meshSource->GetSubmeshes().size() == mesh->GetSubmeshes().size()) - out << YAML::Value << std::vector<uint32_t>(); - else - out << YAML::Value << mesh->GetSubmeshes(); + auto meshSource = AssetManager::GetAsset<MeshSource>(mesh->GetMeshSource()); + bool useDefaultSubmeshes = meshSource && + meshSource->GetSubmeshes().size() == mesh->GetSubmeshes().size(); + + if (useDefaultSubmeshes) + out << YAML::Value << std::vector<uint32_t>(); + else + out << YAML::Value << mesh->GetSubmeshes();
104-105: Missing user feedback for invalid mesh source.The TODO comment indicates that user feedback is needed when mesh source validation fails.
Would you like me to implement proper error reporting here or create an issue to track this task?
170-171: Remove Windows-specific commented code.The commented
GetLastError()is Windows-specific and not portable. Consider removing it entirely.-// SE_CORE_ASSERT(false, "GetLastError() = {0}", GetLastError());
114-125: Consider extracting common dependency registration logic.The dependency registration logic is duplicated between
RegisterMeshDependenciesFromYAMLandRegisterStaticMeshDependenciesFromYAML. Consider creating a shared helper function.static void RegisterMeshSourceDependency(const YAML::Node& data, AssetHandle handle, const char* nodeName) { Project::GetEditorAssetManager()->DeregisterDependencies(handle); AssetHandle meshSourceHandle = 0; if (auto rootNode = data[nodeName]; rootNode) { meshSourceHandle = rootNode["MeshSource"].as<uint64_t>(0); } // must always register something, even if it's 0 Project::GetEditorAssetManager()->RegisterDependency(meshSourceHandle, handle); }Also applies to: 271-281
StarEngine/src/StarEngine/Asset/AssetSerializer.cpp (2)
367-417: Incomplete audio file serialization implementation.The AudioFileSourceSerializer has an empty
Serializemethod and disabled pack serialization. The comment suggests this is due to changes in file path lookup strategy.Would you like me to help implement the missing serialization logic or create an issue to track this incomplete implementation?
595-597: Improve readability of ternary operator.The ternary operator split across lines is difficult to read.
- spatialConfig->AttenuationMod = spConfigData["AttenuationModel"] ? static_cast<AttenuationModel>(spConfigData["AttenuationModel"].as<int>()) - : AttenuationModel::Inverse; + spatialConfig->AttenuationMod = spConfigData["AttenuationModel"] + ? static_cast<AttenuationModel>(spConfigData["AttenuationModel"].as<int>()) + : AttenuationModel::Inverse;StarEngine/src/StarEngine/Asset/AssetManager/EditorAssetManager.h (2)
116-116: Performance concern with immediate registry writes
WriteRegistryToFile()is called immediately after creating each new asset. This could cause performance issues when creating multiple assets in succession.Consider implementing a deferred write mechanism or batching registry updates to improve performance when creating multiple assets.
92-141: Document thread safety requirements for CreateOrReplaceAssetThis method directly accesses
m_LoadedAssetswithout synchronization. While the member is documented as main-thread only, the method itself should document this requirement.Add a comment before the method declaration:
+ // Note: This method must only be called from the main thread as it directly modifies m_LoadedAssets template<typename T, typename... Args> Ref<T> CreateOrReplaceAsset(const std::filesystem::path& path, Args&&... args)
| indent_style = tab | ||
| tab_width = 4 | ||
| indent_size = 4 |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Inconsistent indentation configuration across file types.
There are conflicting indentation settings:
- C++ files:
indent_size = 4,tab_width = 4 - C# files:
indent_size = 3,tab_width = 3 - C#/VB files:
indent_size = 4,tab_width = 4
This inconsistency could lead to formatting issues when working with mixed codebases.
Consider standardizing on a single indentation approach:
[*.cs]
# Indentation and spacing
-indent_size = 3
+indent_size = 4
indent_style = tab
-tab_width = 3
+tab_width = 4Also applies to: 82-84, 297-299
🤖 Prompt for AI Agents
In .editorconfig around lines 6 to 8, there are conflicting indentation settings
for different file types causing inconsistent formatting. Review all
indentation-related settings for C++, C#, and VB files, and standardize the
indent_size and tab_width values to a single consistent approach across the
entire file. Apply the same standardization to lines 82-84 and 297-299 to ensure
uniform indentation configuration throughout.
| StarEditor/SandboxProject/Assets/Scripts/Binaries/Microsoft*.dll | ||
|
|
||
| # File Exceptions | ||
| !AssetRegistryCache.hzr |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify the scope of AssetRegistryCache.hzr exception.
The pattern !AssetRegistryCache.hzr will include this file from any directory. Consider using a more specific path if this file should only be tracked from certain locations.
🏁 Script executed:
#!/bin/bash
# Find all AssetRegistryCache.hzr files in the repository
fd "AssetRegistryCache.hzr" --type f
# Check if there are multiple locations where this file might exist
echo -e "\nChecking for asset registry related files:"
fd -e hzr --type fLength of output: 158
Remove or Update Redundant .gitignore Exception
The !AssetRegistryCache.hzr rule in .gitignore (line 74) doesn’t match any existing files in the repository, so it’s effectively a no-op. You can either:
- Remove this exception if no
.hzrfiles should ever be tracked, or - Specify the exact path (e.g.,
!/DotNet/StarEditor/AssetRegistryCache.hzr) if you intend to include a generated file at a known location.
• File: .gitignore
• Line: 74
• Suggested change:
- !AssetRegistryCache.hzr📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| !AssetRegistryCache.hzr |
🤖 Prompt for AI Agents
In the .gitignore file at line 74, the exception rule "!AssetRegistryCache.hzr"
does not match any existing files and is redundant. To fix this, either remove
this line entirely if no .hzr files should be tracked, or update it to specify
the exact path of the file you want to include, such as
"!/DotNet/StarEditor/AssetRegistryCache.hzr", to correctly track the intended
file.
| Dbghelp = { | ||
| Windows = { LibName = " Dbghelp" }, | ||
| }, |
There was a problem hiding this comment.
Fix typo in Dbghelp library name.
There's a tab character in the library name which could cause linking issues.
Dbghelp = {
- Windows = { LibName = " Dbghelp" },
+ Windows = { LibName = "Dbghelp" },
},📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| Dbghelp = { | |
| Windows = { LibName = " Dbghelp" }, | |
| }, | |
| Dbghelp = { | |
| Windows = { LibName = "Dbghelp" }, | |
| }, |
🤖 Prompt for AI Agents
In Dependencies.lua around lines 179 to 181, the Dbghelp library name contains a
tab character before "Dbghelp" which can cause linking issues. Remove the tab
character so that the library name is a clean string "Dbghelp" without any
leading whitespace or control characters.
| ARG PREMAKE_TAG=5.0.0-beta2 | ||
| RUN curl -Lo premake.tar.gz https://github.com/premake/premake-core/releases/download/v$PREMAKE_TAG/premake-$PREMAKE_TAG-linux.tar.gz | ||
| RUN tar xzf premake.tar.gz | ||
|
|
||
| # `Linux-Build` wants premake on the path | ||
| ENV PATH="/workspace:${PATH}" | ||
|
|
||
| # Download and extract Vulkan SDK | ||
| ARG VULKAN_VER=1.3.283.0 | ||
| RUN curl -Lo vulkan.tar.xz https://sdk.lunarg.com/sdk/download/$VULKAN_VER/linux/vulkansdk-linux-x86_64-$VULKAN_VER.tar.xz | ||
| RUN tar xJf vulkan.tar.xz | ||
|
|
||
| ENV VULKAN_SDK=/workspace/$VULKAN_VER/x86_64 |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider security implications of downloading external artifacts.
The Dockerfile downloads premake and Vulkan SDK from external sources. Consider adding checksum verification for security.
ARG PREMAKE_TAG=5.0.0-beta2
+ ARG PREMAKE_SHA256=expected_checksum_here
RUN curl -Lo premake.tar.gz https://github.com/premake/premake-core/releases/download/v$PREMAKE_TAG/premake-$PREMAKE_TAG-linux.tar.gz
+ RUN echo "$PREMAKE_SHA256 premake.tar.gz" | sha256sum -c -
RUN tar xzf premake.tar.gz
ARG VULKAN_VER=1.3.283.0
+ ARG VULKAN_SHA256=expected_checksum_here
RUN curl -Lo vulkan.tar.xz https://sdk.lunarg.com/sdk/download/$VULKAN_VER/linux/vulkansdk-linux-x86_64-$VULKAN_VER.tar.xz
+ RUN echo "$VULKAN_SHA256 vulkan.tar.xz" | sha256sum -c -
RUN tar xJf vulkan.tar.xz📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ARG PREMAKE_TAG=5.0.0-beta2 | |
| RUN curl -Lo premake.tar.gz https://github.com/premake/premake-core/releases/download/v$PREMAKE_TAG/premake-$PREMAKE_TAG-linux.tar.gz | |
| RUN tar xzf premake.tar.gz | |
| # `Linux-Build` wants premake on the path | |
| ENV PATH="/workspace:${PATH}" | |
| # Download and extract Vulkan SDK | |
| ARG VULKAN_VER=1.3.283.0 | |
| RUN curl -Lo vulkan.tar.xz https://sdk.lunarg.com/sdk/download/$VULKAN_VER/linux/vulkansdk-linux-x86_64-$VULKAN_VER.tar.xz | |
| RUN tar xJf vulkan.tar.xz | |
| ENV VULKAN_SDK=/workspace/$VULKAN_VER/x86_64 | |
| ARG PREMAKE_TAG=5.0.0-beta2 | |
| + ARG PREMAKE_SHA256=expected_checksum_here | |
| RUN curl -Lo premake.tar.gz https://github.com/premake/premake-core/releases/download/v$PREMAKE_TAG/premake-$PREMAKE_TAG-linux.tar.gz | |
| + RUN echo "$PREMAKE_SHA256 premake.tar.gz" | sha256sum -c - | |
| RUN tar xzf premake.tar.gz | |
| # `Linux-Build` wants premake on the path | |
| ENV PATH="/workspace:${PATH}" | |
| # Download and extract Vulkan SDK | |
| ARG VULKAN_VER=1.3.283.0 | |
| + ARG VULKAN_SHA256=expected_checksum_here | |
| RUN curl -Lo vulkan.tar.xz https://sdk.lunarg.com/sdk/download/$VULKAN_VER/linux/vulkansdk-linux-x86_64-$VULKAN_VER.tar.xz | |
| + RUN echo "$VULKAN_SHA256 vulkan.tar.xz" | sha256sum -c - | |
| RUN tar xJf vulkan.tar.xz | |
| ENV VULKAN_SDK=/workspace/$VULKAN_VER/x86_64 |
🤖 Prompt for AI Agents
In Dockerfile lines 13 to 25, the downloads of premake and Vulkan SDK lack
checksum verification, which poses a security risk. To fix this, add checksum
verification steps after each download by including the expected checksum values
and using a tool like sha256sum to verify the integrity of the downloaded files
before extracting them. If the checksum does not match, the build should fail to
prevent using tampered files.
| defines | ||
| { | ||
| "TRACY_ENABLE", | ||
| "TRACY_ON_DEMAND", | ||
| "TRACY_CALLSTACK=10", | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify Tracy configuration values.
The Tracy profiling defines look appropriate, but please verify that the callstack depth of 10 is optimal for your debugging needs.
🌐 Web query:
What are the recommended Tracy profiling configuration values for TRACY_CALLSTACK?
💡 Result:
The recommended configuration values for Tracy profiling regarding TRACY_CALLSTACK depend on the platform and the application's needs, particularly the balance between profiling detail and performance overhead.
Here are some common recommendations:
-
TRACY_CALLSTACK:
Enable this to capture call stacks on profiling events. This is useful for seeing the chain of function calls. It has a performance cost. -
TRACY_CALLSTACK_MAX:
This defines the maximum depth of the call stack to capture. Typical values range from 16 to 64. Setting it too high can impact performance and memory. -
TRACY_CALLSTACK_BUF_SIZE:
Buffer size for call stack data. Larger buffers can hold more call stack information but consume more memory. -
TRACY_CALLSTACK_RESOLUTION:
Defines the frequency or granularity of capturing call stacks; depends on the profiler setup. -
Platform-specific settings:
On Windows with MSVC, enabling frame pointers can improve call stack accuracy. On Linux or other platforms, options like frame pointer omission (-fno-omit-frame-pointer) help.
Since exact numeric recommendations can vary, a typical configuration for TRACY_CALLSTACK might be:
#define TRACY_CALLSTACK
#define TRACY_CALLSTACK_MAX 32
#define TRACY_CALLSTACK_BUF_SIZE 8192Would you like me to look up the most recent official recommendations or example configurations from the Tracy profiler documentation or repos?
Increase Tracy callstack depth
The current setting of a 10-frame callstack is below the typical 16–64 range and may limit diagnostic detail. Please consider raising it (32 is a common default) to strike a better balance between profiling detail and overhead.
• File: Sandbox/premake5.lua
Lines: 31–36
Current:
defines
{
"TRACY_ENABLE",
"TRACY_ON_DEMAND",
"TRACY_CALLSTACK=10",
}
Suggested:
defines
{
"TRACY_ENABLE",
"TRACY_ON_DEMAND",
"TRACY_CALLSTACK=32", -- or any value between 16 and 64 per your profiling needs
}
🤖 Prompt for AI Agents
In Sandbox/premake5.lua around lines 31 to 36, increase the Tracy callstack
depth by changing the define "TRACY_CALLSTACK=10" to "TRACY_CALLSTACK=32" to
provide more detailed profiling information while balancing overhead. Update the
value in the defines block accordingly.
|
|
||
| TReturn Invoke(TArgs...args) const | ||
| { | ||
| SE_CORE_ASSERT(IsBound(), "Trying to invoke unbound delegate."); |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify SE_CORE_ASSERT macro is defined.
The code uses SE_CORE_ASSERT but doesn't include the header that defines it.
🏁 Script executed:
#!/bin/bash
# Search for SE_CORE_ASSERT definition
ast-grep --pattern 'SE_CORE_ASSERT'Length of output: 28473
🏁 Script executed:
#!/bin/bash
# Display the top of the Delegate.h file to check includes
sed -n '1,50p' StarEngine/src/StarEngine/Core/Delegate.hLength of output: 1612
🏁 Script executed:
#!/bin/bash
# List all include directives in Delegate.h
grep -n "#include" StarEngine/src/StarEngine/Core/Delegate.hLength of output: 81
🏁 Script executed:
#!/bin/bash
# Display the top of Buffer.h to check how Assert.h is included
sed -n '1,50p' StarEngine/src/StarEngine/Core/Buffer.hLength of output: 1000
Include missing Assert header in Delegate.h
Delegate.h uses the SE_CORE_ASSERT macro but does not include the header where it’s defined. Add the Assert include to avoid undefined-macro errors:
• File: StarEngine/src/StarEngine/Core/Delegate.h
Insert after the <list> include:
#pragma once
-#include <list>
+#include <list>
+#include "StarEngine/Core/Assert.h"🤖 Prompt for AI Agents
In StarEngine/src/StarEngine/Core/Delegate.h at line 102, the SE_CORE_ASSERT
macro is used without including its defining header, causing potential
undefined-macro errors. Fix this by adding the appropriate Assert header include
right after the existing <list> include directive near the top of the file.
| // We don't want to Invoke new functions that may be added | ||
| // in one of the delegate calls. | ||
| // Hopefully none of the elements are removed from the list | ||
| // during the iteration. | ||
| const uint32_t numberOfInvocations = m_InvocationList.size(); | ||
|
|
||
| uint32_t i = 0; | ||
|
|
||
| for (const auto& element : m_InvocationList) | ||
| { | ||
| (*(element.Stub))(element.Object, std::forward<TArgs>(args)...); | ||
|
|
||
| if (++i >= numberOfInvocations) | ||
| break; | ||
| } | ||
| } |
There was a problem hiding this comment.
Address thread safety and concurrent modification concerns.
The comment "Hopefully none of the elements are removed from the list during the iteration" indicates a potential issue. If delegates can unbind themselves or others during invocation, this could cause crashes or undefined behavior.
Consider:
- Making a copy of the invocation list before iteration
- Adding thread-safety documentation if these are meant to be single-threaded only
- Implementing proper synchronization if multi-threaded usage is intended
🤖 Prompt for AI Agents
In StarEngine/src/StarEngine/Core/Delegate.h around lines 233 to 248, the
iteration over m_InvocationList is unsafe if elements can be removed during
invocation, risking crashes or undefined behavior. To fix this, create a local
copy of m_InvocationList before the loop and iterate over that copy to prevent
modification issues. Additionally, clarify in the documentation whether this
delegate is single-threaded or multi-threaded, and if multi-threaded, implement
appropriate synchronization mechanisms to ensure thread safety.
| class WindowTitleBarHitTestEvent : public Event | ||
| { | ||
| public: | ||
| WindowTitleBarHitTestEvent(int x, int y, int& hit) | ||
| : m_X(x), m_Y(y), m_Hit(hit) { | ||
| } | ||
|
|
||
| inline int GetX() const { return m_X; } | ||
| inline int GetY() const { return m_Y; } | ||
| inline void SetHit(bool hit) { m_Hit = (int)hit; } | ||
|
|
||
| EVENT_CLASS_TYPE(WindowTitleBarHitTest) | ||
| EVENT_CLASS_CATEGORY(EventCategoryApplication) | ||
| private: | ||
| int m_X; | ||
| int m_Y; | ||
| int& m_Hit; | ||
| }; |
There was a problem hiding this comment.
Reconsider using reference member for hit result.
The reference member m_Hit has several issues:
- Makes the class non-copyable and non-assignable
- Risk of dangling reference if the referenced variable goes out of scope
- The bool to int cast in
SetHit()should be explicit
Consider using a callback or storing the result differently.
class WindowTitleBarHitTestEvent : public Event
{
public:
- WindowTitleBarHitTestEvent(int x, int y, int& hit)
- : m_X(x), m_Y(y), m_Hit(hit) {
+ WindowTitleBarHitTestEvent(int x, int y)
+ : m_X(x), m_Y(y), m_Hit(0) {
}
inline int GetX() const { return m_X; }
inline int GetY() const { return m_Y; }
- inline void SetHit(bool hit) { m_Hit = (int)hit; }
+ inline void SetHit(bool hit) { m_Hit = static_cast<int>(hit); }
+ inline int GetHit() const { return m_Hit; }
private:
int m_X;
int m_Y;
- int& m_Hit;
+ int m_Hit;
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| class WindowTitleBarHitTestEvent : public Event | |
| { | |
| public: | |
| WindowTitleBarHitTestEvent(int x, int y, int& hit) | |
| : m_X(x), m_Y(y), m_Hit(hit) { | |
| } | |
| inline int GetX() const { return m_X; } | |
| inline int GetY() const { return m_Y; } | |
| inline void SetHit(bool hit) { m_Hit = (int)hit; } | |
| EVENT_CLASS_TYPE(WindowTitleBarHitTest) | |
| EVENT_CLASS_CATEGORY(EventCategoryApplication) | |
| private: | |
| int m_X; | |
| int m_Y; | |
| int& m_Hit; | |
| }; | |
| class WindowTitleBarHitTestEvent : public Event | |
| { | |
| public: | |
| WindowTitleBarHitTestEvent(int x, int y) | |
| : m_X(x), m_Y(y), m_Hit(0) { | |
| } | |
| inline int GetX() const { return m_X; } | |
| inline int GetY() const { return m_Y; } | |
| inline void SetHit(bool hit) { m_Hit = static_cast<int>(hit); } | |
| inline int GetHit() const { return m_Hit; } | |
| EVENT_CLASS_TYPE(WindowTitleBarHitTest) | |
| EVENT_CLASS_CATEGORY(EventCategoryApplication) | |
| private: | |
| int m_X; | |
| int m_Y; | |
| int m_Hit; | |
| }; |
🤖 Prompt for AI Agents
In StarEngine/src/StarEngine/Core/Events/ApplicationEvent.h between lines 57 and
74, the class WindowTitleBarHitTestEvent uses an int reference member m_Hit,
which causes issues with copying, assignment, and potential dangling references.
Replace the int& m_Hit member with a safer alternative such as a callback
function or a separate result storage mechanism. Also, make the bool to int cast
in SetHit() explicit using static_cast<int> for clarity and safety.
| class AssetReloadedEvent : public Event | ||
| { | ||
| public: | ||
| AssetReloadedEvent(AssetHandle assetHandle) : AssetHandle(assetHandle) {} |
There was a problem hiding this comment.
Fix naming conflict in AssetReloadedEvent constructor.
The constructor parameter assetHandle and member variable AssetHandle create a naming conflict, potentially causing confusion and shadowing issues.
- AssetReloadedEvent(AssetHandle assetHandle) : AssetHandle(assetHandle) {}
+ AssetReloadedEvent(AssetHandle assetHandle) : m_AssetHandle(assetHandle) {}And update the member variable:
public:
- AssetHandle AssetHandle;
+ AssetHandle GetAssetHandle() const { return m_AssetHandle; }
+
+ private:
+ AssetHandle m_AssetHandle;Also applies to: 41-41
🤖 Prompt for AI Agents
In StarEngine/src/StarEngine/Core/Events/EditorEvents.h at lines 28 and 41, the
constructor parameter named assetHandle conflicts with the member variable
AssetHandle, causing shadowing issues. Rename the constructor parameter to a
different name such as handle or assetHandleParam, and update the constructor
initializer list to assign the member variable AssetHandle from this renamed
parameter to avoid confusion and shadowing.
| std::string ToString() const override | ||
| { | ||
| std::stringstream ss; | ||
| ss << "AssetReloadedEvent"; | ||
| return ss.str(); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance ToString() to include event-specific data.
The ToString() method for AssetReloadedEvent doesn't include the asset handle information, reducing its debugging value.
std::string ToString() const override
{
std::stringstream ss;
- ss << "AssetReloadedEvent";
+ ss << "AssetReloadedEvent: AssetHandle=" << AssetHandle;
return ss.str();
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| std::string ToString() const override | |
| { | |
| std::stringstream ss; | |
| ss << "AssetReloadedEvent"; | |
| return ss.str(); | |
| } | |
| std::string ToString() const override | |
| { | |
| std::stringstream ss; | |
| ss << "AssetReloadedEvent: AssetHandle=" << AssetHandle; | |
| return ss.str(); | |
| } |
🤖 Prompt for AI Agents
In StarEngine/src/StarEngine/Core/Events/EditorEvents.h around lines 30 to 35,
the ToString() method for AssetReloadedEvent currently returns only the event
name without including the asset handle data. Modify the ToString() method to
append the asset handle information to the stringstream output so that the
returned string contains both the event name and the relevant asset handle
details for better debugging.
| postbuildcommands { | ||
| --'{COPY} "../StarEngine/vendor/NvidiaAftermath/lib/x64/windows/GFSDK_Aftermath_Lib.x64.dll" "%{cfg.targetdir}"', | ||
| } | ||
|
|
||
| filter { "system:windows", "configurations:Debug or configurations:Debug-AS" } | ||
| postbuildcommands { | ||
| "{COPYDIR} %{wks.location}/StarEditor/assets %{wks.location}/bin/" .. outputdir .. "/StarEditor/assets", | ||
| "{COPYDIR} %{wks.location}/StarEditor/Resources %{wks.location}/bin/" .. outputdir .. "/StarEditor/Resources", | ||
| "{COPYFILE} %{wks.location}/StarEditor/imgui.ini %{wks.location}/bin/" .. outputdir .. "/StarEditor/imgui.ini", | ||
| --'{COPY} "../StarEngine/vendor/assimp/bin/windows/Debug/assimp-vc143-mtd.dll" "%{cfg.targetdir}"', | ||
| } | ||
|
|
||
| filter "configurations:Debug" | ||
| defines "SE_DEBUG" | ||
| runtime "Debug" | ||
| symbols "on" | ||
| filter { "system:windows", "configurations:Release or configurations:Dist" } | ||
| postbuildcommands { | ||
| --'{COPY} "../StarEngine/vendor/assimp/bin/windows/Release/assimp-vc143-mt.dll" "%{cfg.targetdir}"', | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Clarify the status of commented post-build commands.
Several post-build commands are commented out, which might indicate missing runtime dependencies. Please verify if these DLLs are still needed and either remove the comments or uncomment the necessary commands.
🏁 Script executed:
#!/bin/bash
# Check if the referenced DLL paths exist
fd "GFSDK_Aftermath_Lib.x64.dll" ../StarEngine/vendor/
fd "assimp-vc143-mtd.dll" ../StarEngine/vendor/
fd "assimp-vc143-mt.dll" ../StarEngine/vendor/Length of output: 476
The previous search failed due to an incorrect path. Let’s search for these DLLs across the entire repo and locate any vendor directories:
🏁 Script executed:
#!/bin/bash
set -e
# Search for the DLL files across the repository
fd GFSDK_Aftermath_Lib.x64.dll
fd assimp-vc143-mtd.dll
fd assimp-vc143-mt.dll
# Locate any 'vendor' directories to confirm paths
fd -t d vendorLength of output: 343
Uncomment or remove the commented post-build DLL copy commands
The referenced DLLs still exist under StarEngine/vendor/..., so if your Windows builds require these runtime dependencies you should re-enable the copy commands; otherwise, clean up the commented blocks.
• File: StarEditor/premake5.lua (lines 43–55)
• DLL paths confirmed:
– StarEngine/vendor/NvidiaAftermath/lib/x64/windows/GFSDK_Aftermath_Lib.x64.dll
– StarEngine/vendor/assimp/bin/windows/Debug/assimp-vc143-mtd.dll
– StarEngine/vendor/assimp/bin/windows/Release/assimp-vc143-mt.dll
Action:
- If these DLLs must ship with your build, uncomment their
{COPY}commands. - If you’ve moved to a different packaging strategy, remove these commented blocks to avoid confusion.
🤖 Prompt for AI Agents
In StarEditor/premake5.lua between lines 43 and 55, the post-build commands that
copy required DLLs for Windows builds are commented out. To fix this, either
uncomment the {COPY} commands for the NvidiaAftermath and Assimp DLLs if these
DLLs are still needed at runtime, or remove these commented blocks entirely if
the project no longer requires copying these DLLs after build to keep the script
clean and clear.
| result, err = os.outputof("pkg-config --libs gtk+-3.0") | ||
| linkoptions { result } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for pkg-config command.
The os.outputof call could fail if pkg-config is not installed or GTK+3.0 is not available. Consider adding error handling.
-result, err = os.outputof("pkg-config --libs gtk+-3.0")
-linkoptions { result }
+result, errorCode = os.outputof("pkg-config --libs gtk+-3.0")
+if errorCode == 0 then
+ linkoptions { result }
+else
+ error("Failed to find GTK+3.0. Please install libgtk-3-dev")
+end📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| result, err = os.outputof("pkg-config --libs gtk+-3.0") | |
| linkoptions { result } | |
| result, errorCode = os.outputof("pkg-config --libs gtk+-3.0") | |
| if errorCode == 0 then | |
| linkoptions { result } | |
| else | |
| error("Failed to find GTK+3.0. Please install libgtk-3-dev") | |
| end |
🤖 Prompt for AI Agents
In StarEditor/premake5.lua around lines 61 to 62, the call to
os.outputof("pkg-config --libs gtk+-3.0") may fail if pkg-config is missing or
GTK+3.0 is not installed. Add error handling by checking if the call returns an
error; if so, handle it gracefully by logging an error message or providing a
fallback instead of proceeding with an invalid link option.
| bool FileSystem::WriteBytes(const std::filesystem::path& filepath, const Buffer& buffer) | ||
| { | ||
| std::ofstream stream(filepath, std::ios::binary | std::ios::trunc); | ||
|
|
||
| if (!stream) | ||
| { | ||
| stream.close(); | ||
| return false; | ||
| } | ||
|
|
||
| stream.write((char*)buffer.Data, buffer.Size); | ||
| stream.close(); | ||
|
|
||
| return true; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve error handling in WriteBytes.
The current implementation has issues with error handling: closing an unopened stream and not checking write success.
bool FileSystem::WriteBytes(const std::filesystem::path& filepath, const Buffer& buffer)
{
std::ofstream stream(filepath, std::ios::binary | std::ios::trunc);
if (!stream)
{
- stream.close();
return false;
}
stream.write((char*)buffer.Data, buffer.Size);
+ bool success = stream.good();
stream.close();
- return true;
+ return success;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| bool FileSystem::WriteBytes(const std::filesystem::path& filepath, const Buffer& buffer) | |
| { | |
| std::ofstream stream(filepath, std::ios::binary | std::ios::trunc); | |
| if (!stream) | |
| { | |
| stream.close(); | |
| return false; | |
| } | |
| stream.write((char*)buffer.Data, buffer.Size); | |
| stream.close(); | |
| return true; | |
| } | |
| bool FileSystem::WriteBytes(const std::filesystem::path& filepath, const Buffer& buffer) | |
| { | |
| std::ofstream stream(filepath, std::ios::binary | std::ios::trunc); | |
| if (!stream) | |
| { | |
| return false; | |
| } | |
| stream.write((char*)buffer.Data, buffer.Size); | |
| bool success = stream.good(); | |
| stream.close(); | |
| return success; | |
| } |
🤖 Prompt for AI Agents
In StarEngine/Platform/Windows/WindowsFileSystem.cpp around lines 39 to 53, the
WriteBytes function improperly calls close on a stream that may not be open and
does not verify if the write operation succeeded. Remove the stream.close() call
from the error check after stream creation, as the stream is not open if the
check fails. After writing to the stream, check the stream's state to confirm
the write was successful before returning true; if the write failed, return
false accordingly.
| Buffer FileSystem::ReadBytes(const std::filesystem::path& filepath) | ||
| { | ||
| Buffer buffer; | ||
|
|
||
| std::ifstream stream(filepath, std::ios::binary | std::ios::ate); | ||
| SE_CORE_ASSERT(stream); | ||
|
|
||
| auto end = stream.tellg(); | ||
| stream.seekg(0, std::ios::beg); | ||
| auto size = end - stream.tellg(); | ||
| SE_CORE_ASSERT(size != 0); | ||
|
|
||
| buffer.Allocate((uint32_t)size); | ||
| stream.read((char*)buffer.Data, buffer.Size); | ||
| stream.close(); | ||
|
|
||
| return buffer; | ||
| } |
There was a problem hiding this comment.
Replace assertions with proper error handling.
Using assertions for file operations will crash the application in debug builds. Consider returning an empty buffer on failure.
Buffer FileSystem::ReadBytes(const std::filesystem::path& filepath)
{
Buffer buffer;
std::ifstream stream(filepath, std::ios::binary | std::ios::ate);
- SE_CORE_ASSERT(stream);
+ if (!stream)
+ return buffer;
auto end = stream.tellg();
stream.seekg(0, std::ios::beg);
auto size = end - stream.tellg();
- SE_CORE_ASSERT(size != 0);
+ if (size <= 0)
+ {
+ stream.close();
+ return buffer;
+ }
buffer.Allocate((uint32_t)size);
stream.read((char*)buffer.Data, buffer.Size);
+ if (!stream)
+ {
+ buffer.Release();
+ }
stream.close();
return buffer;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| Buffer FileSystem::ReadBytes(const std::filesystem::path& filepath) | |
| { | |
| Buffer buffer; | |
| std::ifstream stream(filepath, std::ios::binary | std::ios::ate); | |
| SE_CORE_ASSERT(stream); | |
| auto end = stream.tellg(); | |
| stream.seekg(0, std::ios::beg); | |
| auto size = end - stream.tellg(); | |
| SE_CORE_ASSERT(size != 0); | |
| buffer.Allocate((uint32_t)size); | |
| stream.read((char*)buffer.Data, buffer.Size); | |
| stream.close(); | |
| return buffer; | |
| } | |
| Buffer FileSystem::ReadBytes(const std::filesystem::path& filepath) | |
| { | |
| Buffer buffer; | |
| std::ifstream stream(filepath, std::ios::binary | std::ios::ate); | |
| if (!stream) | |
| return buffer; | |
| auto end = stream.tellg(); | |
| stream.seekg(0, std::ios::beg); | |
| auto size = end - stream.tellg(); | |
| if (size <= 0) | |
| { | |
| stream.close(); | |
| return buffer; | |
| } | |
| buffer.Allocate((uint32_t)size); | |
| stream.read((char*)buffer.Data, buffer.Size); | |
| if (!stream) | |
| { | |
| buffer.Release(); | |
| } | |
| stream.close(); | |
| return buffer; | |
| } |
🤖 Prompt for AI Agents
In StarEngine/Platform/Windows/WindowsFileSystem.cpp lines 55 to 72, replace the
SE_CORE_ASSERT checks on the file stream and file size with proper error
handling. Instead of asserting, check if the file stream is valid and if the
size is non-zero; if either check fails, return an empty Buffer object. This
prevents the application from crashing in debug builds and handles file read
failures gracefully.
| std::filesystem::path FileSystem::GetPersistentStoragePath() | ||
| { | ||
| if (!s_PersistentStoragePath.empty()) | ||
| return s_PersistentStoragePath; | ||
|
|
||
| PWSTR roamingFilePath; | ||
| HRESULT result = SHGetKnownFolderPath(FOLDERID_RoamingAppData, KF_FLAG_DEFAULT, NULL, &roamingFilePath); | ||
| SE_CORE_VERIFY(result == S_OK); | ||
| s_PersistentStoragePath = roamingFilePath; | ||
| s_PersistentStoragePath /= "StarEngine"; | ||
|
|
||
| if (!std::filesystem::exists(s_PersistentStoragePath)) | ||
| std::filesystem::create_directory(s_PersistentStoragePath); | ||
|
|
||
| return s_PersistentStoragePath; | ||
| } |
There was a problem hiding this comment.
Fix memory leak - free the allocated path string.
The SHGetKnownFolderPath function allocates memory that must be freed with CoTaskMemFree.
std::filesystem::path FileSystem::GetPersistentStoragePath()
{
if (!s_PersistentStoragePath.empty())
return s_PersistentStoragePath;
PWSTR roamingFilePath;
HRESULT result = SHGetKnownFolderPath(FOLDERID_RoamingAppData, KF_FLAG_DEFAULT, NULL, &roamingFilePath);
- SE_CORE_VERIFY(result == S_OK);
+ if (result != S_OK)
+ {
+ SE_CORE_ERROR("Failed to get roaming app data path");
+ return std::filesystem::path();
+ }
s_PersistentStoragePath = roamingFilePath;
+ CoTaskMemFree(roamingFilePath);
s_PersistentStoragePath /= "StarEngine";
if (!std::filesystem::exists(s_PersistentStoragePath))
std::filesystem::create_directory(s_PersistentStoragePath);
return s_PersistentStoragePath;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| std::filesystem::path FileSystem::GetPersistentStoragePath() | |
| { | |
| if (!s_PersistentStoragePath.empty()) | |
| return s_PersistentStoragePath; | |
| PWSTR roamingFilePath; | |
| HRESULT result = SHGetKnownFolderPath(FOLDERID_RoamingAppData, KF_FLAG_DEFAULT, NULL, &roamingFilePath); | |
| SE_CORE_VERIFY(result == S_OK); | |
| s_PersistentStoragePath = roamingFilePath; | |
| s_PersistentStoragePath /= "StarEngine"; | |
| if (!std::filesystem::exists(s_PersistentStoragePath)) | |
| std::filesystem::create_directory(s_PersistentStoragePath); | |
| return s_PersistentStoragePath; | |
| } | |
| std::filesystem::path FileSystem::GetPersistentStoragePath() | |
| { | |
| if (!s_PersistentStoragePath.empty()) | |
| return s_PersistentStoragePath; | |
| PWSTR roamingFilePath; | |
| HRESULT result = SHGetKnownFolderPath(FOLDERID_RoamingAppData, KF_FLAG_DEFAULT, NULL, &roamingFilePath); | |
| - SE_CORE_VERIFY(result == S_OK); | |
| + if (result != S_OK) | |
| + { | |
| + SE_CORE_ERROR("Failed to get roaming app data path"); | |
| + return std::filesystem::path(); | |
| + } | |
| s_PersistentStoragePath = roamingFilePath; | |
| + CoTaskMemFree(roamingFilePath); | |
| s_PersistentStoragePath /= "StarEngine"; | |
| if (!std::filesystem::exists(s_PersistentStoragePath)) | |
| std::filesystem::create_directory(s_PersistentStoragePath); | |
| return s_PersistentStoragePath; | |
| } |
🤖 Prompt for AI Agents
In StarEngine/Platform/Windows/WindowsFileSystem.cpp around lines 74 to 89, the
memory allocated by SHGetKnownFolderPath for roamingFilePath is not freed,
causing a memory leak. After assigning roamingFilePath to
s_PersistentStoragePath, call CoTaskMemFree(roamingFilePath) to properly release
the allocated memory.
| #define SE_FORCE_INLINE __forceinline | ||
| #define SE_EXPLICIT_STATIC static | ||
| #elif defined(SE_COMPILER_GCC) || defined(SE_COMPILER_CLANG) | ||
| #define SE_FORCE_INLINE __attribute__((always_inline)) inline | ||
| #define SE_EXPLICIT_STATIC | ||
| #else |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify the purpose of SE_EXPLICIT_STATIC macro.
The SE_EXPLICIT_STATIC macro is defined as static for MSVC but empty for GCC/Clang. This is unusual as static keyword behavior should be consistent across compilers. Please verify this is intentional and document its specific use case.
🏁 Script executed:
#!/bin/bash
# Search for usage of SE_EXPLICIT_STATIC to understand its purpose
rg "SE_EXPLICIT_STATIC" -A 3 -B 3Length of output: 7013
Document SE_EXPLICIT_STATIC’s intent in Base.h
The SE_EXPLICIT_STATIC macro is intentionally defined as static on MSVC to give internal linkage to free functions and explicit‐specializations defined in headers (avoiding multiple‐definition/ODR errors), whereas GCC/Clang rely on COMDAT and inline‐template deduplication so no static is needed. Please add a brief comment above the macro to explain this.
• File: StarEngine/src/StarEngine/Core/Base.h
Add just above the existing definitions:
#if defined(SE_COMPILER_MSVC)
+ // On MSVC, mark free functions and explicit template specializations
+ // in headers as static to prevent multiple-definition (ODR) errors.
#define SE_FORCE_INLINE __forceinline
#define SE_EXPLICIT_STATIC static
#elif defined(SE_COMPILER_GCC) || defined(SE_COMPILER_CLANG)
+ // GCC/Clang use COMDAT/inline-template deduplication, so no static needed.
#define SE_FORCE_INLINE __attribute__((always_inline)) inline
#define SE_EXPLICIT_STATIC
#else
#define SE_FORCE_INLINE inline
#define SE_EXPLICIT_STATIC
#endif📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| #define SE_FORCE_INLINE __forceinline | |
| #define SE_EXPLICIT_STATIC static | |
| #elif defined(SE_COMPILER_GCC) || defined(SE_COMPILER_CLANG) | |
| #define SE_FORCE_INLINE __attribute__((always_inline)) inline | |
| #define SE_EXPLICIT_STATIC | |
| #else | |
| #if defined(SE_COMPILER_MSVC) | |
| // On MSVC, mark free functions and explicit template specializations | |
| // in headers as static to prevent multiple-definition (ODR) errors. | |
| #define SE_FORCE_INLINE __forceinline | |
| #define SE_EXPLICIT_STATIC static | |
| #elif defined(SE_COMPILER_GCC) || defined(SE_COMPILER_CLANG) | |
| // GCC/Clang use COMDAT/inline-template deduplication, so no static needed. | |
| #define SE_FORCE_INLINE __attribute__((always_inline)) inline | |
| #define SE_EXPLICIT_STATIC | |
| #else | |
| #define SE_FORCE_INLINE inline | |
| #define SE_EXPLICIT_STATIC | |
| #endif |
🤖 Prompt for AI Agents
In StarEngine/src/StarEngine/Core/Base.h around lines 48 to 53, add a brief
comment above the SE_EXPLICIT_STATIC macro definitions explaining that on MSVC
it is defined as static to provide internal linkage for free functions and
explicit specializations in headers to avoid multiple-definition/ODR errors,
while on GCC/Clang it is left empty because they rely on COMDAT and
inline-template deduplication for this purpose.
| struct AtomicFlag | ||
| { | ||
| return std::make_shared<T>(std::forward<Args>(args)...); | ||
| } | ||
| SE_FORCE_INLINE void SetDirty() { flag.clear(); } | ||
| SE_FORCE_INLINE bool CheckAndResetIfDirty() { return !flag.test_and_set(); } | ||
|
|
||
| } | ||
| explicit AtomicFlag() noexcept { flag.test_and_set(); } | ||
| AtomicFlag(const AtomicFlag&) noexcept {} | ||
| AtomicFlag& operator=(const AtomicFlag&) noexcept { return *this; } | ||
| AtomicFlag(AtomicFlag&&) noexcept {}; | ||
| AtomicFlag& operator=(AtomicFlag&&) noexcept { return *this; } | ||
|
|
||
| private: | ||
| std::atomic_flag flag; | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Clarify the inverted atomic_flag semantics.
The AtomicFlag implementation uses inverted semantics where clear() sets the dirty state and test_and_set() clears it. This is counterintuitive and could lead to confusion. Consider adding detailed documentation explaining this design choice or refactoring to use more intuitive naming.
Also, remove the unnecessary semicolon after the move constructor body on line 89.
struct AtomicFlag
{
+ // Note: This uses inverted atomic_flag semantics where:
+ // - clear() means "set dirty" (flag = false)
+ // - test_and_set() means "clear dirty" (flag = true)
SE_FORCE_INLINE void SetDirty() { flag.clear(); }
SE_FORCE_INLINE bool CheckAndResetIfDirty() { return !flag.test_and_set(); }
explicit AtomicFlag() noexcept { flag.test_and_set(); }
AtomicFlag(const AtomicFlag&) noexcept {}
AtomicFlag& operator=(const AtomicFlag&) noexcept { return *this; }
- AtomicFlag(AtomicFlag&&) noexcept {};
+ AtomicFlag(AtomicFlag&&) noexcept {}
AtomicFlag& operator=(AtomicFlag&&) noexcept { return *this; }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| struct AtomicFlag | |
| { | |
| return std::make_shared<T>(std::forward<Args>(args)...); | |
| } | |
| SE_FORCE_INLINE void SetDirty() { flag.clear(); } | |
| SE_FORCE_INLINE bool CheckAndResetIfDirty() { return !flag.test_and_set(); } | |
| } | |
| explicit AtomicFlag() noexcept { flag.test_and_set(); } | |
| AtomicFlag(const AtomicFlag&) noexcept {} | |
| AtomicFlag& operator=(const AtomicFlag&) noexcept { return *this; } | |
| AtomicFlag(AtomicFlag&&) noexcept {}; | |
| AtomicFlag& operator=(AtomicFlag&&) noexcept { return *this; } | |
| private: | |
| std::atomic_flag flag; | |
| }; | |
| struct AtomicFlag | |
| { | |
| // Note: This uses inverted atomic_flag semantics where: | |
| // - clear() means "set dirty" (flag = false) | |
| // - test_and_set() means "clear dirty" (flag = true) | |
| SE_FORCE_INLINE void SetDirty() { flag.clear(); } | |
| SE_FORCE_INLINE bool CheckAndResetIfDirty() { return !flag.test_and_set(); } | |
| explicit AtomicFlag() noexcept { flag.test_and_set(); } | |
| AtomicFlag(const AtomicFlag&) noexcept {} | |
| AtomicFlag& operator=(const AtomicFlag&) noexcept { return *this; } | |
| AtomicFlag(AtomicFlag&&) noexcept {} | |
| AtomicFlag& operator=(AtomicFlag&&) noexcept { return *this; } | |
| private: | |
| std::atomic_flag flag; | |
| }; |
🤖 Prompt for AI Agents
In StarEngine/src/StarEngine/Core/Base.h lines 81 to 94, the AtomicFlag struct
uses inverted semantics where SetDirty calls flag.clear() and
CheckAndResetIfDirty calls !flag.test_and_set(), which is counterintuitive. Add
detailed comments explaining this inversion to clarify the design choice for
future maintainers. Also, remove the unnecessary semicolon after the move
constructor definition on line 89 to clean up the code.
| struct Flag | ||
| { | ||
| SE_FORCE_INLINE void SetDirty() noexcept { flag = true; } | ||
| SE_FORCE_INLINE bool CheckAndResetIfDirty() noexcept | ||
| { | ||
| if (flag) | ||
| return !(flag = !flag); | ||
| else | ||
| return false; | ||
| } | ||
|
|
||
| SE_FORCE_INLINE bool IsDirty() const noexcept { return flag; } | ||
|
|
||
| private: | ||
| bool flag = false; | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Simplify CheckAndResetIfDirty() logic.
The current implementation uses confusing logic that's hard to understand. Consider simplifying it for better readability.
struct Flag
{
SE_FORCE_INLINE void SetDirty() noexcept { flag = true; }
SE_FORCE_INLINE bool CheckAndResetIfDirty() noexcept
{
- if (flag)
- return !(flag = !flag);
- else
- return false;
+ bool wasDirty = flag;
+ flag = false;
+ return wasDirty;
}
SE_FORCE_INLINE bool IsDirty() const noexcept { return flag; }
private:
bool flag = false;
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| struct Flag | |
| { | |
| SE_FORCE_INLINE void SetDirty() noexcept { flag = true; } | |
| SE_FORCE_INLINE bool CheckAndResetIfDirty() noexcept | |
| { | |
| if (flag) | |
| return !(flag = !flag); | |
| else | |
| return false; | |
| } | |
| SE_FORCE_INLINE bool IsDirty() const noexcept { return flag; } | |
| private: | |
| bool flag = false; | |
| }; | |
| struct Flag | |
| { | |
| SE_FORCE_INLINE void SetDirty() noexcept { flag = true; } | |
| SE_FORCE_INLINE bool CheckAndResetIfDirty() noexcept | |
| { | |
| bool wasDirty = flag; | |
| flag = false; | |
| return wasDirty; | |
| } | |
| SE_FORCE_INLINE bool IsDirty() const noexcept { return flag; } | |
| private: | |
| bool flag = false; | |
| }; |
🤖 Prompt for AI Agents
In StarEngine/src/StarEngine/Core/Base.h between lines 96 and 111, the
CheckAndResetIfDirty() method uses convoluted logic to check and reset the flag.
Simplify this method by directly checking if the flag is true, resetting it to
false, and returning the original flag value. This will improve readability
without changing functionality.
| Buffer(const void* data, uint64_t size) | ||
| : Data((void*)data), Size(size) { | ||
| } |
There was a problem hiding this comment.
Avoid casting away const in constructor
The constructor casts away const from the data pointer, which violates const correctness and could lead to undefined behavior if the original data is actually const.
Consider making the constructor only accept non-const data or store a const pointer when appropriate:
- Buffer(const void* data, uint64_t size)
- : Data((void*)data), Size(size) {
+ Buffer(void* data, uint64_t size)
+ : Data(data), Size(size) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| Buffer(const void* data, uint64_t size) | |
| : Data((void*)data), Size(size) { | |
| } | |
| Buffer(void* data, uint64_t size) | |
| : Data(data), Size(size) { | |
| } |
🤖 Prompt for AI Agents
In StarEngine/src/StarEngine/Core/Buffer.h around lines 16 to 18, the
constructor casts away const from the data pointer, which breaks const
correctness. To fix this, change the constructor to accept a const void*
parameter and store the Data member as a const void* pointer instead of void*.
This preserves the const qualifier and prevents potential undefined behavior.
| byte* ReadBytes(uint64_t size, uint64_t offset) const | ||
| { | ||
| SE_CORE_ASSERT(offset + size <= Size, "Buffer overflow!"); | ||
| byte* buffer = snew byte[size]; | ||
| memcpy(buffer, (byte*)Data + offset, size); | ||
| return buffer; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Document memory ownership for ReadBytes
ReadBytes allocates and returns a new buffer, transferring ownership to the caller. This should be clearly documented to prevent memory leaks.
Consider returning a BufferSafe or std::unique_ptr<byte[]> to make ownership transfer explicit and prevent leaks.
🤖 Prompt for AI Agents
In StarEngine/src/StarEngine/Core/Buffer.h around lines 90 to 96, the ReadBytes
function allocates a new byte buffer and returns a raw pointer, transferring
ownership to the caller without clear documentation, risking memory leaks.
Update the function to return a std::unique_ptr<byte[]> to make ownership
transfer explicit and prevent leaks, and add documentation comments explaining
that the caller is responsible for managing the returned buffer's lifetime.
- Added lots of shaders from https://github.com/StudioCherno/Hazel - Fixed lots of bugs - Changed nvrhi origin - Planning to change the editor - ImGui Node Editor
There was a problem hiding this comment.
Actionable comments posted: 27
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
.gitignore (2)
31-38: **Fix glob patterns: use .ext or **/.ext instead of .extThe current patterns are unlikely to match as intended. Use
*.ext(matches anywhere) or**/*.ext(explicit recursive).-**.sln -**.vcxproj -**.vcxproj.filters -**.vcxproj.user -**.csproj -**.nsight-gfxproj -**.DotSettings -**.user +*.sln +*.vcxproj +*.vcxproj.filters +*.vcxproj.user +*.csproj +*.nsight-gfxproj +*.DotSettings +*.user
1-75: .gitignore misconfigurations found – please update exception and ignore rulesThe verification script shows that several patterns aren’t behaving as intended, causing needed files to remain ignored or unwanted files to slip through. Please address the following:
.gitignore:16– the ruleignores the entire directory (and overrides the earlier lines 11–12). As a result, even your intended binariesStarEditor/Resources/Scripts/
(StarEngine-ScriptCore.dll&.pdb) remain ignored. Add explicit negations after the directory rule:!StarEditor/Resources/Scripts/StarEngine-ScriptCore.dll !StarEditor/Resources/Scripts/StarEngine-ScriptCore.pdb.gitignore:19&:67– you ignore all ofthen “unignore” the folder itself (StarEditor/SandboxProject/Assets/Scripts/Binaries/**!…/Binaries). However, Git only unignores that directory entry, not its contents, sofoo.dllis still ignored (see match at line 19). To track all other DLLs in that folder while still ignoring system and Microsoft DLLs, add, after the!…/Binariesline:!StarEditor/SandboxProject/Assets/Scripts/Binaries/**/*.dll.gitignore:20– the specific file ruledid not match when tested (the file was not ignored). Please verify there’s no stray whitespace or comment character, or anchor it to the repo root with a leading slash:StarEditor/SandboxProject/Assets/SoundBank.hsb/StarEditor/SandboxProject/Assets/SoundBank.hsb.gitignore:75– the patternis ignoring that directory and all its contents (e.g.StarEditor/DotNetREADME.md). If you instead want to include DotNet, prefix with!and add a slash to cover its contents:!StarEditor/DotNet/ !StarEditor/DotNet/**- ✅ Your vendor exceptions (
!vendor/bin/,!StarEngine/vendor/assimp/bin,!StarEngine/vendor/mono/bin) are working as expected.Suggested diff to fix these issues:
.gitignore @@ -11,6 +11,10 @@ StarEditor/Resources/Scripts/ __pycache__ +# Include ScriptCore binaries +!StarEditor/Resources/Scripts/StarEngine-ScriptCore.dll +!StarEditor/Resources/Scripts/StarEngine-ScriptCore.pdb + StarEditor/SandboxProject/Assets/Scripts/Binaries/** StarEditor/SandboxProject/Assets/SoundBank.hsb @@ -65,7 +69,10 @@ # Folder Exceptions !StarEditor/SandboxProject/Assets/Scripts/Binaries +# Track all other DLLs in Binaries +!StarEditor/SandboxProject/Assets/Scripts/Binaries/**/*.dll + # System DLLs StarEditor/SandboxProject/Assets/Scripts/Binaries/System*.dll StarEditor/SandboxProject/Assets/Scripts/Binaries/Microsoft*.dll @@ -73,3 +80,6 @@ # File Exceptions !AssetRegistryCache.hzr -StarEditor/DotNet + +# Include DotNet folder and contents +!StarEditor/DotNet/ +!StarEditor/DotNet/**After applying these changes, re-run your verification script to confirm that only the intended files are ignored or tracked.
StarEngine/src/StarEngine/Audio/AudioListener.cpp (1)
21-31: Fix per-listener world-up initializationThe current code uses a function-static
setupWorldUpflag, so only the first listener ever has its world-up vector set. Subsequent calls for differentm_ListenerIndexskip the setup, which can lead to incorrect orientation in multi-listener configurations.Required change:
- Remove the
static bool setupWorldUpguard.- Unconditionally call
ma_engine_listener_set_world_upfor every listener each time its position is set.Suggested diff:
SE_PROFILE_FUNCTION("AudioListener::SetPosition"); auto* engine = static_cast<ma_engine*>(AudioEngine::GetEngine()); ma_engine_listener_set_position(engine, m_ListenerIndex, position.x, position.y, position.z); - static bool setupWorldUp = false; - if (!setupWorldUp) - { - ma_engine_listener_set_world_up(engine, m_ListenerIndex, 0, 1, 0); - setupWorldUp = true; - } + // Always align each listener’s up vector to +Y + ma_engine_listener_set_world_up(engine, m_ListenerIndex, 0, 1, 0);Alternatively, perform the world-up setup once per listener at creation time, or track a per-listener flag if you want to avoid redundant calls. (miniaud.io)
StarEngine/src/StarEngine/Audio/AudioSource.cpp (1)
17-35: Destructor can leak and may double-uninitialize on stop() failure.Current flow:
- Only uninitializes when
IsPlaying()is true; leaks when not playing.- If
ma_sound_stopfails, it uninitializes inside the error branch and then again after the block.Fix: Always uninitialize when not shutting down the engine; stop if playing; never call
ma_sound_uninittwice.void AudioSource::~AudioSource() { SE_PROFILE_FUNCTION("AudioSource::~AudioSource"); - if (!AudioEngine::ShuttingDownEngine()) - { - if (IsPlaying()) - { - if (ma_sound_stop(m_Sound.get()) != MA_SUCCESS) - { - SE_CORE_ERROR("Failed to stop playback device!"); - ma_sound_uninit(m_Sound.get()); - } - - ma_sound_uninit(m_Sound.get()); - m_Sound = nullptr; - } - } + if (!AudioEngine::ShuttingDownEngine()) + { + if (m_Sound) + { + if (IsPlaying()) + { + ma_result r = ma_sound_stop(m_Sound.get()); + if (r != MA_SUCCESS) + SE_CORE_ERROR("Failed to stop sound before uninit (result=%d)", (int)r); + } + ma_sound_uninit(m_Sound.get()); + m_Sound = nullptr; + } + } }Optional follow-up: make
m_Soundan RAII wrapper with a custom deleter to guaranteema_sound_uninit()is always called even if early returns happen elsewhere.
♻️ Duplicate comments (1)
.gitignore (1)
73-75: Redundant AssetRegistry exception remains; scope it or removeThe
!AssetRegistryCache.hzrexception still doesn’t correspond to a known tracked path and may have no effect. Either remove it or scope to the intended location (e.g., DotNet Editor cache) to avoid accidentally including files of the same name elsewhere.-!AssetRegistryCache.hzr +#!/ If needed, scope to the exact path (example): +#!/ !StarEditor/SomePath/AssetRegistryCache.hzrIf you share the intended path, I can provide a precise rule.
🧹 Nitpick comments (41)
.gitmodules (5)
21-24: Normalize URL: remove trailing slash for tracy.Minor nit for consistency and to avoid edge cases in tooling that compare URLs textually.
Apply this diff:
[submodule "StarEngine/vendor/tracy/tracy"] path = StarEngine/vendor/tracy/tracy - url = https://github.com/wolfpld/tracy/ + url = https://github.com/wolfpld/tracy
30-33: Personal fork for yaml-cpp—consider upstream or org mirror; also remove stray whitespace line.Using sheazywi/yaml-cpp ties you to a personal fork. Prefer upstream (jbeder/yaml-cpp) or mirror your required patches under the org to reduce bus factor. Also, Line 33 contains a stray tab-only line.
Apply this whitespace cleanup now:
[submodule "StarEngine/vendor/yaml-cpp"] path = StarEngine/vendor/yaml-cpp url = https://github.com/sheazywi/yaml-cpp - +If patches are required, I can help create an org-hosted mirror and document a rebasing workflow to track upstream while carrying your diffs.
3-49: Normalize URL styles across submodules for clarity and tooling friendliness.Mixed styles are used: some URLs include “.git”, one has a trailing slash, others don’t. While Git accepts all, a consistent convention reduces friction with tooling that does string equality on URLs.
Option A (no .git, no trailing slash). Apply these diffs:
-[submodule "StarEngine/vendor/spdlog"] - path = StarEngine/vendor/spdlog - url = https://github.com/gabime/spdlog.git +[submodule "StarEngine/vendor/spdlog"] + path = StarEngine/vendor/spdlog + url = https://github.com/gabime/spdlog -[submodule "StarEngine/vendor/tracy/tracy"] - path = StarEngine/vendor/tracy/tracy - url = https://github.com/wolfpld/tracy/ +[submodule "StarEngine/vendor/tracy/tracy"] + path = StarEngine/vendor/tracy/tracy + url = https://github.com/wolfpld/tracyIf you prefer the “.git” style, invert the change and add “.git” to all URLs for consistency.
3-49: Consider HTTPS vs SSH for contributor ergonomics.HTTPS is fine for CI and anonymous clones. If you expect internal contributors to push to forks frequently via submodule remotes, consider documenting SSH alternatives or providing a
git submodule foreachhelper to switch to SSH locally.I can add a short “Working with Submodules” doc including scripts to toggle HTTPS/SSH remotes.
3-49: Optional: enforce org-owned mirrors for third-party forks.For forks you rely on (GLFW, Box2D, imgui, nvrhi), mirroring under starbounded-dev ensures long-term availability and allows you to lock PR review/CI rules on the mirror.
I can set up a migration plan: create mirrors, switch URLs here, and open tracking issues to periodically rebase mirrors against upstream.
StarEditor/Resources/Shaders/Include/HLSL/Buffers.hlslh (5)
5-15: Be explicit about matrix layout to prevent accidental row/column-major togglesHLSL defaults to column-major, but that can be changed via pragmas elsewhere. Making the matrices explicitly column_major here avoids subtle transposition bugs when other headers flip pack_matrix. (learn.microsoft.com)
Apply:
- float4x4 ViewProjectionMatrix; - float4x4 InverseViewProjectionMatrix; - float4x4 ProjectionMatrix; - float4x4 InverseProjectionMatrix; - float4x4 ViewMatrix; - float4x4 InverseViewMatrix; + column_major float4x4 ViewProjectionMatrix; + column_major float4x4 InverseViewProjectionMatrix; + column_major float4x4 ProjectionMatrix; + column_major float4x4 InverseProjectionMatrix; + column_major float4x4 ViewMatrix; + column_major float4x4 InverseViewMatrix;
3-15: Consider DX12 portability: add register(...) fallbacks behind a preprocessor guardIf you plan to target DX12 with the same HLSL, DXC will ignore vk::binding for DXIL, and you may want stable register/space assignments to match your NVRHI binding layouts (or use NVRHI’s VulkanBindingOffsets strategy). Optional, but prevents surprises when bringing up a D3D12 backend. (github.com)
Example pattern:
#if defined(SHADER_API_VULKAN) [[vk::binding(0, 2)]] ConstantBuffer<Camera> u_Camera; #else ConstantBuffer<Camera> u_Camera : register(b0, space2); #endifRepeat similarly for u_ScreenData with your chosen slot/space.
18-24: Optional: use integer resolution for pixel dimensions, keep float inverses separateFull/Half resolutions are inherently integral. Using uint2 for the absolute sizes and keeping float2 for inverses can prevent accidental fractional math or precision drift where integer texel counts are assumed. This is optional and depends on how you consume these fields downstream.
Possible tweak:
- float2 InvFullResolution; - float2 FullResolution; - float2 InvHalfResolution; - float2 HalfResolution; + float2 InvFullResolution; + uint2 FullResolution; + float2 InvHalfResolution; + uint2 HalfResolution;If you go this route, update call sites accordingly.
16-16: Minor: replace magic numbers with shared macros to keep set/binding consistent across languagesDefining shared macros (or a generated header) for descriptor set and binding indices reduces drift between HLSL and GLSL includes, especially in large refactors.
Example:
// DescriptorSets.hlslh #define SET_GLOBAL 2 #define BINDING_CAMERA 0 #define BINDING_SCREENDATA 2 // Usage: [[vk::binding(BINDING_CAMERA, SET_GLOBAL)]] ConstantBuffer<Camera> u_Camera; [[vk::binding(BINDING_SCREENDATA, SET_GLOBAL)]] ConstantBuffer<ScreenData> u_ScreenData;
16-16: Note: both buffers in set 2 aligns well with NVRHI layouts, just ensure your layout items match b/t/u/s slotsKeeping per-frame data in one descriptor set is a sane choice. Just make sure your NVRHI BindingLayout matches these exact b#/set indices (or that you’re solely driving Vulkan via vk::binding), to avoid register-space collisions if a DX12 backend is introduced later. (developer.nvidia.com)
.gitignore (3)
10-10: Prefer explicit directory ignores with trailing slashesAdding a trailing slash clarifies intent (directory vs. file) and avoids edge cases where a file named the same could be ignored unintentionally.
-StarEditor/vendor/VulkanSDK +StarEditor/vendor/VulkanSDK/ -Binaries +Binaries/ -StarEditor/Tools/JoltViewer +StarEditor/Tools/JoltViewer/ -StarEditor/DotNet +StarEditor/DotNet/Also applies to: 15-15, 26-27, 75-75
40-43: Verify intention to ignore Makefiles repo-wideGlobal
Makefileignore can hide intentionally tracked Makefiles. If these are generated, consider scoping to generated locations instead (e.g., build/**/Makefile) and keep source Makefiles tracked.Would you like a follow-up patch to scope this to common generator outputs (Premake/CMake) used in this repo?
48-51: Confirm custom lfs/ and hooks/ folders are not needed in repoIgnoring these root folders is fine if they’re tooling artifacts. Ensure there’s no need to commit custom Git hooks or LFS configs stored here.
StarEngine/src/StarEngine/Audio/AudioEngine.h (2)
3-4: Prefer project-style include and verify include search path.miniaudio_engine.h appears to be a project header. Use quotes and an explicit project-relative path for consistency with the rest of the codebase and to avoid relying on toolchain/system header paths. Also, the forward declaration on Line 5 may be redundant once this header is included.
-#include <miniaudio_engine.h> +#include "StarEngine/Audio/miniaudio_engine.h"Follow-up:
- If "StarEngine/src" isn’t on the compiler’s include path in all configurations, the angle-bracket include may break on some platforms. Please confirm CI across Windows/Linux/macOS.
- Consider removing the redundant
struct ma_engine;forward declaration after verifying this header provides the full declaration.
19-20: Typo/mismatch in setter name ("Initalized").The API has
HasInitializedEngine()butSetInitalizedEngine(bool). Recommend adding a correctly spelled alias and deprecating the misspelled one to avoid churn.Example (header):
// Keep for backward compatibility; consider deprecating after a release cycle. static void SetInitalizedEngine(bool value) { s_HasInitializedAudioEngine = value; } // New, correctly spelled alias: static void SetInitializedEngine(bool value) { s_HasInitializedAudioEngine = value; }Happy to sweep usages across the repo if you’d like.
StarEngine/src/StarEngine/Audio/AudioListener.cpp (1)
36-39: Unnecessary cast; simplify for clarity.
AudioEngine::GetEngine()already returnsma_engine*. Thestatic_cast<ma_engine*>adds noise without value.- auto* engine = static_cast<ma_engine*>(AudioEngine::GetEngine()); + ma_engine* engine = AudioEngine::GetEngine();Consider applying the same simplification to the other methods in this file.
StarEngine/src/StarEngine/Audio/AudioSource.cpp (3)
41-44: Check return codes for start/stop operations.
ma_sound_start/stop/seekreturnma_result. Logging failures will make device/format issues much easier to diagnose.if (m_Sound) { - ma_sound_start(m_Sound.get()); + ma_result r = ma_sound_start(m_Sound.get()); + if (r != MA_SUCCESS) + SE_CORE_ERROR("Failed to start sound (result=%d)", (int)r); }if (m_Sound) { - ma_sound_stop(m_Sound.get()); - ma_sound_seek_to_pcm_frame(m_Sound.get(), 0); + ma_result r1 = ma_sound_stop(m_Sound.get()); + if (r1 != MA_SUCCESS) + SE_CORE_ERROR("Failed to stop sound (result=%d)", (int)r1); + + ma_result r2 = ma_sound_seek_to_pcm_frame(m_Sound.get(), 0); + if (r2 != MA_SUCCESS) + SE_CORE_ERROR("Failed to seek sound to start (result=%d)", (int)r2); m_CursorPos = 0; }Also applies to: 73-77
119-139: Remove redundant inner null-check and simplify looping set.Inside the existing
if (m_Sound)block,if (sound)is redundant (same pointer). Also, the looping set can be simplified.if (m_Sound) { ma_sound* sound = m_Sound.get(); ma_sound_set_volume(sound, config.VolumeMultiplier); ma_sound_set_pitch(sound, config.PitchMultiplier); - - if (sound) - { - if (config.Looping) - ma_sound_set_looping(sound, MA_TRUE); - else - ma_sound_set_looping(sound, MA_FALSE); - } + ma_sound_set_looping(sound, config.Looping ? MA_TRUE : MA_FALSE); if (m_Spatialization != config.Spatialization) { m_Spatialization = config.Spatialization; ma_sound_set_spatialization_enabled(sound, config.Spatialization); }
14-15: Consider RAII for ma_sound to make resource cleanup bulletproof.Define a custom deleter and use it in the member type (in AudioSource.h). This guarantees
ma_sound_uninitis called exactly once, even if early returns or exceptions are introduced later.Example:
struct MaSoundDeleter { void operator()(ma_sound* s) const noexcept { if (s) ma_sound_uninit(s); ::operator delete(s); } }; using UniqueMaSound = std::unique_ptr<ma_sound, MaSoundDeleter>;Then:
// allocation m_Sound = UniqueMaSound(static_cast<ma_sound*>(::operator new(sizeof(ma_sound))));If you want, I can prepare a focused patch touching AudioSource.h/.cpp to introduce this safely.
StarEngine/src/StarEngine/Core/Application.cpp (8)
231-237: ImGui: Avoid implicit this-capture and coalesce submits for orderingThe first lambda uses an explicit
apppointer, but the second uses[=]and accessesm_ImGuiLayervia implicitthis. Prefer explicit captures to avoid accidental lifetime bugs and coalesce Begin/End into a single submit to keep them adjacent in the queue.- if (m_Specification.EnableImGui) - { - Renderer::Submit([app]() { app->RenderImGui(); }); - Renderer::Submit([=]() { m_ImGuiLayer->End(); }); - } + if (m_Specification.EnableImGui) + { + ImGuiLayer* il = m_ImGuiLayer; + Renderer::Submit([app, il]() + { + app->RenderImGui(); + il->End(); + }); + }
191-194: Reduce per-frame INFO logging noise
SE_CORE_INFO("-- BEGIN/END FRAME ...")every frame will spam logs and impact perf. Consider TRACE or guard behind a verbosity flag.- SE_CORE_INFO("-- BEGIN FRAME {0}", frameCounter); + SE_CORE_TRACE("-- BEGIN FRAME {0}", frameCounter); @@ - SE_CORE_INFO("-- END FRAME {0}", frameCounter); + SE_CORE_TRACE("-- END FRAME {0}", frameCounter);Also applies to: 261-263
249-251: Guard modulo against zero FramesInFlightIf
Renderer::GetConfig().FramesInFlightends up zero (misconfig or platform fallback), modulo will UB. Cheap guard.- m_CurrentFrameIndex = (m_CurrentFrameIndex + 1) % Renderer::GetConfig().FramesInFlight; + const uint32_t fifo = glm::max<uint32_t>(1u, Renderer::GetConfig().FramesInFlight); + m_CurrentFrameIndex = (m_CurrentFrameIndex + 1) % fifo;
337-354: Resize handler is a no-op; rely on Window::ProcessEvents or forward explicitly
OnWindowResizeenqueues nothing (code commented out). Given Window::ProcessEvents() already callsm_SwapChain->OnResize(width, height), this handler can either:
- remain passive and just propagate the event to layers (intentional), or
- explicitly forward the resize to the renderer if you want immediate handling.
Please decide and remove dead commented code for clarity. If you choose explicit forwarding:
- auto& window = m_Window; - Renderer::Submit([&window, width, height]() mutable - { - //m_Window->GetDeviceManager()->ResizeSwapChain(); - //window->GetSwapChain().OnResize(width, height); - }); + auto* window = m_Window.get(); + Renderer::Submit([window, width, height]() + { + window->GetSwapChain().OnResize(width, height); + });Note: Avoid duplicating the resize if Window::ProcessEvents keeps doing it; pick one mechanism to prevent double work.
35-36: Broken event binding macro (unused but misleading)
Application::##fnis not a valid token pasting target here. If you keep the macro, use a direct pointer-to-member.-#define BIND_EVENT_FN(fn) std::bind(&Application::##fn, this, std::placeholders::_1) +#define BIND_EVENT_FN(fn) std::bind(&Application::fn, this, std::placeholders::_1)If it stays unused, consider removing it.
314-333: Event callback ordering vs layers: resolve the TODOYou currently dispatch to layers first, then to
m_EventCallbacks, despite the note suggesting engine callbacks might need to run first. Decide and codify the intended order; stale TODOs create ambiguity.Option A (callbacks first):
- Move the callback loop before the layer reverse-iteration.
- Document the contract in Application.h.
Option B (layers first, as-is):
- Delete the TODO and state in docs that layers can consume events before callbacks.
31-33: Extraneous externs
extern ImGuiContext* GImGui;isn’t used in this TU. Consider removing to reduce global coupling.
75-81: Remove redundant render-thread pumpRenderer::Init() already invokes the render-thread pump to compile shaders (see Renderer.cpp:285 where
Application::Get().GetRenderThread().Pump()is called). The extram_RenderThread.Pump()in Application.cpp is therefore redundant and can skew frame sequencing during startup.• File: StarEngine/src/StarEngine/Core/Application.cpp
Lines 75–81Proposed change:
- // Render one frame (TODO: maybe make a func called Pump or something) - m_RenderThread.Pump(); + // Renderer::Init() already pumps to compile shadersStarEditor/Resources/Shaders/Include/GLSL/Common.glslh (2)
3-5: Tighten math constants and literal types
- Use float literals consistently to avoid implicit int->float promotions.
- Provide a more precise PI (float-precision friendly), and a canonical epsilon.
Apply this diff:
-const float PI = 3.141592; -const float TwoPI = 2 * PI; -const float Epsilon = 0.00001; +const float PI = 3.14159265; +const float TwoPI = 2.0 * PI; +const float Epsilon = 1e-5;
23-30: sRGB->Linear conversion: clamp or document expected input range; clean up literals
- For robustness, clamp sRGB to [0,1] before conversion to avoid pow on out-of-range values.
- Remove ‘f’ suffixes; simplify constants.
Apply this diff:
-vec4 ToLinear(vec4 sRGB) +vec4 ToLinear(vec4 srgb) { - bvec4 cutoff = lessThan(sRGB, vec4(0.04045)); - vec4 higher = pow((sRGB + vec4(0.055))/vec4(1.055), vec4(2.4)); - vec4 lower = sRGB/vec4(12.92); - - return mix(higher, lower, cutoff); + srgb = clamp(srgb, 0.0, 1.0); + bvec4 cutoff = lessThan(srgb, vec4(0.04045)); + vec4 higher = pow((srgb + 0.055) / 1.055, vec4(2.4)); + vec4 lower = srgb / 12.92; + return mix(higher, lower, cutoff); }Optionally add a vec3 overload for convenience.
StarEditor/Resources/Shaders/Include/Common/GTAO.slh (1)
10-10: Cross-language numeric literalIf this header is shared by both HLSL and GLSL preprocessors, prefer “1.5” (no ‘f’). GLSL doesn’t require or standardize an ‘f’ suffix.
-#define XE_GTAO_OCCLUSION_TERM_SCALE (1.5f) +#define XE_GTAO_OCCLUSION_TERM_SCALE (1.5)StarEditor/Resources/Shaders/Include/HLSL/Types.hlslh (1)
3-23: Guard macro usage and document defaultRelying on an undefined macro implicitly evaluating to 0 is brittle across tools. Prefer an explicit guard and default.
Apply this diff around the top:
-#if (USE_HALF_FLOAT_PRECISION != 0) +#if !defined(USE_HALF_FLOAT_PRECISION) + #define USE_HALF_FLOAT_PRECISION 0 +#endif + +#if (USE_HALF_FLOAT_PRECISION != 0)Also consider making the “old” vs “new” fp16 selection a separate macro (e.g., USE_MIN16FLOAT) so it’s togglable from build configs.
StarEditor/Resources/Shaders/Include/GLSL/Buffers.glslh (3)
20-26: std140 offsets comment appears inaccurateWith std140, DirectionalLights (a struct with two vec3+floats) will consume at least 64 bytes. CameraPosition would then start at offset 64, not 32. Update or remove the “Offset = 32” comment to avoid confusion.
66-70: Spot shadow matrices UBO is exactly 64 KiB; borderline with device limits1024 mat4 is 1024*64 = 65536 bytes. This is at the typical UBO maximum and may fail on some devices due to alignment/padding or lower limits. Prefer SSBO for safety.
Example change:
-layout(std140, set = 1, binding = 8) uniform SpotShadowData +layout(std430, set = 1, binding = 8) readonly buffer SpotShadowData { - mat4 Mats[1024]; + mat4 Mats[]; } u_SpotLightMatrices;This also allows varying counts without recompilation.
71-80: Consider readonly/writeonly qualifiers for both index buffers
- s_VisiblePointLightIndicesBuffer is missing readonly/writeonly; annotate for intent and optimization.
- s_VisibleSpotLightIndicesBuffer is writeonly; if it’s later read in another stage, confirm the pipeline barriers accordingly.
-layout(std430, set = 1, binding = 9) buffer VisiblePointLightIndicesBuffer +layout(std430, set = 1, binding = 9) readonly buffer VisiblePointLightIndicesBufferStarEditor/Resources/Shaders/Include/GLSL/PBR.glslh (2)
29-38: Redundant function with inconsistent usage.
GeometrySchlickGGXduplicates the logic ofGaSchlickG1with the same k calculation. The function appears unused and creates potential confusion about which function to use.Consider removing this redundant function or clearly documenting why both are needed:
-float GeometrySchlickGGX(float NdotV, float roughness) -{ - float r = (roughness + 1.0); - float k = (r * r) / 8.0; - - float nom = NdotV; - float denom = NdotV * (1.0 - k) + k; - - return nom / denom; -}
40-48: Consider reusing existing functions to reduce duplication.The
GeometrySmithfunction reimplements the same logic already available inGaSchlickGGX. This creates code duplication and potential maintenance issues.Refactor to reuse the existing
GaSchlickGGXfunction:float GeometrySmith(vec3 N, vec3 V, vec3 L, float roughness) { float NdotV = max(dot(N, V), 0.0); float NdotL = max(dot(N, L), 0.0); - float ggx2 = GeometrySchlickGGX(NdotV, roughness); - float ggx1 = GeometrySchlickGGX(NdotL, roughness); - - return ggx1 * ggx2; + return GaSchlickGGX(NdotL, NdotV, roughness); }StarEditor/Resources/Shaders/Include/GLSL/PBR_Resources.glslh (1)
13-15: Address the TODO comment for BRDF LUT texture.The TODO suggests moving the BRDF LUT texture declaration to a header, which would improve organization and reusability.
Would you like me to help create a separate header for shared lookup tables like the BRDF LUT, or open an issue to track this refactoring task?
StarEditor/Resources/Shaders/Include/GLSL/Lighting.glslh (2)
7-17: Consider encapsulating global state to improve modularity.The
m_Paramsglobal instance may cause issues with shader composition and reusability. Consider passing parameters as function arguments instead.Consider refactoring to pass parameters explicitly:
-struct PBRParameters -{ - vec3 Albedo; - float Roughness; - float Metalness; - - vec3 Normal; - vec3 View; - float NdotV; -} m_Params; +struct PBRParameters +{ + vec3 Albedo; + float Roughness; + float Metalness; + + vec3 Normal; + vec3 View; + float NdotV; +}; -vec3 CalculateDirLights(vec3 F0) +vec3 CalculateDirLights(vec3 F0, PBRParameters params)
122-129: Code duplication with GetPointLightBufferIndex.This function is nearly identical to
GetPointLightBufferIndexexcept for the buffer name. Consider consolidating into a single parameterized function.Consider creating a generic function:
+int GetLightBufferIndex(int i, bool isSpotLight) +{ + ivec2 tileID = ivec2(gl_FragCoord) / ivec2(16, 16); + uint index = tileID.y * u_RendererData.TilesCountX + tileID.x; + uint offset = index * 1024; + + if (isSpotLight) + return s_VisibleSpotLightIndicesBuffer.Indices[offset + i]; + else + return s_VisiblePointLightIndicesBuffer.Indices[offset + i]; +} -int GetSpotLightBufferIndex(int i) -{ - ivec2 tileID = ivec2(gl_FragCoord) / ivec2(16, 16); - uint index = tileID.y * u_RendererData.TilesCountX + tileID.x; - - uint offset = index * 1024; - return s_VisibleSpotLightIndicesBuffer.Indices[offset + i]; -} +#define GetSpotLightBufferIndex(i) GetLightBufferIndex(i, true)StarEditor/Resources/Shaders/Include/GLSL/ShadowMapping.glslh (2)
12-12: Global variable needs better placement as noted in TODO.The
ShadowFadeglobal variable should be properly scoped or passed as a parameter.Would you like me to help refactor this to pass
ShadowFadeas a parameter to the shadow functions, or create an issue to track this improvement?
14-25: Consider using an array lookup instead of switch statement.While the AMD workaround is noted, modern drivers should handle array indexing properly. Consider testing if this is still needed.
If the AMD issue is resolved, simplify to:
vec3 GetShadowMapCoords(vec3 shadowMapCoords[4], uint cascade) { - // NOTE(Yan): this exists because AMD doesn't seem to like indexing of varyings (causes artifacts) - switch (cascade) - { - case 0: return shadowMapCoords[0]; - case 1: return shadowMapCoords[1]; - case 2: return shadowMapCoords[2]; - case 3: return shadowMapCoords[3]; - } - return vec3(0.0f); + return (cascade < 4) ? shadowMapCoords[cascade] : vec3(0.0f); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (41)
StarEditor/Resources/Shaders/DirShadowMap.glslis excluded by!**/*.glslStarEditor/Resources/Shaders/DirShadowMap_Anim.glslis excluded by!**/*.glslStarEditor/Resources/Shaders/EnvironmentIrradiance.glslis excluded by!**/*.glslStarEditor/Resources/Shaders/EnvironmentMipFilter.glslis excluded by!**/*.glslStarEditor/Resources/Shaders/EquirectangularToCubeMap.glslis excluded by!**/*.glslStarEditor/Resources/Shaders/Grid.glslis excluded by!**/*.glslStarEditor/Resources/Shaders/HZB.glslis excluded by!**/*.glslStarEditor/Resources/Shaders/HazelPBR_Anim.glslis excluded by!**/*.glslStarEditor/Resources/Shaders/HazelPBR_Static.glslis excluded by!**/*.glslStarEditor/Resources/Shaders/HazelPBR_Transparent.glslis excluded by!**/*.glslStarEditor/Resources/Shaders/JumpFlood_Composite.glslis excluded by!**/*.glslStarEditor/Resources/Shaders/JumpFlood_Init.glslis excluded by!**/*.glslStarEditor/Resources/Shaders/JumpFlood_Pass.glslis excluded by!**/*.glslStarEditor/Resources/Shaders/LightCulling.glslis excluded by!**/*.glslStarEditor/Resources/Shaders/PostProcessing/AO-Composite.glslis excluded by!**/*.glslStarEditor/Resources/Shaders/PostProcessing/Bloom.glslis excluded by!**/*.glslStarEditor/Resources/Shaders/PostProcessing/DOF.glslis excluded by!**/*.glslStarEditor/Resources/Shaders/PostProcessing/EdgeDetection.glslis excluded by!**/*.glslStarEditor/Resources/Shaders/PostProcessing/GTAO-Denoise.glslis excluded by!**/*.glslStarEditor/Resources/Shaders/PostProcessing/GTAO.hlslis excluded by!**/*.hlslStarEditor/Resources/Shaders/PostProcessing/Pre-Convolution.glslis excluded by!**/*.glslStarEditor/Resources/Shaders/PostProcessing/SSR-Composite.glslis excluded by!**/*.glslStarEditor/Resources/Shaders/PostProcessing/SSR.glslis excluded by!**/*.glslStarEditor/Resources/Shaders/PostProcessing/SceneComposite.glslis excluded by!**/*.glslStarEditor/Resources/Shaders/Pre-Integration.glslis excluded by!**/*.glslStarEditor/Resources/Shaders/PreDepth.glslis excluded by!**/*.glslStarEditor/Resources/Shaders/PreDepth_Anim.glslis excluded by!**/*.glslStarEditor/Resources/Shaders/PreethamSky.glslis excluded by!**/*.glslStarEditor/Resources/Shaders/Renderer2D.glslis excluded by!**/*.glslStarEditor/Resources/Shaders/Renderer2D_Circle.glslis excluded by!**/*.glslStarEditor/Resources/Shaders/Renderer2D_Line.glslis excluded by!**/*.glslStarEditor/Resources/Shaders/Renderer2D_Text.glslis excluded by!**/*.glslStarEditor/Resources/Shaders/SelectedGeometry.glslis excluded by!**/*.glslStarEditor/Resources/Shaders/SelectedGeometry_Anim.glslis excluded by!**/*.glslStarEditor/Resources/Shaders/Skybox.glslis excluded by!**/*.glslStarEditor/Resources/Shaders/SpotShadowMap.glslis excluded by!**/*.glslStarEditor/Resources/Shaders/SpotShadowMap_Anim.glslis excluded by!**/*.glslStarEditor/Resources/Shaders/TexturePass.glslis excluded by!**/*.glslStarEditor/Resources/Shaders/Wireframe.glslis excluded by!**/*.glslStarEditor/Resources/Shaders/Wireframe_Anim.glslis excluded by!**/*.glslStarEditor/Resources/Shaders/shader.glslis excluded by!**/*.glsl
📒 Files selected for processing (17)
.gitignore(2 hunks).gitmodules(1 hunks)StarEditor/Resources/Shaders/Include/Common/Common.slh(1 hunks)StarEditor/Resources/Shaders/Include/Common/GTAO.slh(1 hunks)StarEditor/Resources/Shaders/Include/GLSL/Buffers.glslh(1 hunks)StarEditor/Resources/Shaders/Include/GLSL/Common.glslh(1 hunks)StarEditor/Resources/Shaders/Include/GLSL/EnvironmentMapping.glslh(1 hunks)StarEditor/Resources/Shaders/Include/GLSL/Lighting.glslh(1 hunks)StarEditor/Resources/Shaders/Include/GLSL/PBR.glslh(1 hunks)StarEditor/Resources/Shaders/Include/GLSL/PBR_Resources.glslh(1 hunks)StarEditor/Resources/Shaders/Include/GLSL/ShadowMapping.glslh(1 hunks)StarEditor/Resources/Shaders/Include/HLSL/Buffers.hlslh(1 hunks)StarEditor/Resources/Shaders/Include/HLSL/Types.hlslh(1 hunks)StarEngine/src/StarEngine/Audio/AudioEngine.h(1 hunks)StarEngine/src/StarEngine/Audio/AudioListener.cpp(2 hunks)StarEngine/src/StarEngine/Audio/AudioSource.cpp(23 hunks)StarEngine/src/StarEngine/Core/Application.cpp(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
StarEngine/src/StarEngine/Audio/AudioListener.cpp (1)
StarEngine/src/StarEngine/Audio/AudioEngine.h (1)
GetEngine(15-15)
StarEngine/src/StarEngine/Audio/AudioSource.cpp (1)
StarEngine/src/StarEngine/Audio/AudioSource.h (1)
AudioSource(43-49)
StarEngine/src/StarEngine/Core/Application.cpp (4)
StarEngine/src/StarEngine/Core/Application.h (2)
Application(39-183)GetTime(120-130)StarEngine/src/StarEngine/Renderer/Renderer.cpp (10)
SetConfig(1221-1224)SetConfig(1221-1221)Init(202-438)Init(202-202)Shutdown(440-469)Shutdown(440-440)BeginFrame(825-828)BeginFrame(825-825)GetConfig(1216-1219)GetConfig(1216-1216)StarEngine/src/StarEngine/Core/Window.cpp (10)
Create(68-71)Create(68-68)Init(83-413)Init(83-83)Shutdown(427-445)Shutdown(427-427)ProcessEvents(454-472)ProcessEvents(454-454)BeginFrame(500-504)BeginFrame(500-500)StarEngine/src/StarEngine/Core/Input.cpp (6)
ClearReleasedKeys(355-377)ClearReleasedKeys(355-355)TransitionPressedKeys(303-311)TransitionPressedKeys(303-303)TransitionPressedButtons(313-329)TransitionPressedButtons(313-313)
🔇 Additional comments (39)
.gitmodules (9)
3-3: Good move to official spdlog upstream.Switching to gabime/spdlog is the right upstream. Nothing else to flag here.
7-7: Confirm intentional dependency on TheCherno’s GLFW fork.You’re pointing at TheCherno/glfw rather than the upstream glfw/glfw. If you rely on Hazel-specific patches, fine—otherwise prefer upstream or mirror the fork under your org to reduce third‑party dependency risk and improve availability.
Would you like me to scan build files for GLFW-specific patches and open an issue to track migrating to upstream or an org-owned fork?
11-11: LGTM: glm now points to the canonical upstream.Using g-truc/glm is correct and reduces maintenance.
15-15: Validate Box2D fork choice.This points to TheCherno/Box2D (fork) instead of erincatto/box2d (upstream). If you need the fork’s changes, consider mirroring it under starbounded-dev for reproducibility and to avoid unexpected force-pushes.
I can audit your CMake/ premake build to see if any fork-only headers/APIs are used before suggesting a safe migration path.
34-37: NFD-Extended looks correct; check license and platform coverage.This repo is commonly used; just ensure your binary distribution policy is compatible with its license across Windows/macOS/Linux.
I can add a license summary table to the README if you want.
38-41: LGTM: backward-cpp upstream selected.This is the canonical repository; good choice for crash backtraces.
46-49: nvrhi on ‘hazel’ branch—double-check that this matches your Vulkan integration plan.Given the renderer refactor to Vulkan via NVRHI, tracking the Hazel branch is probably intentional to match TheCherno ecosystem. Ensure we explicitly pin SHAs in the superproject and avoid
--remotein CI to keep builds stable.I can add a periodic workflow that verifies submodule SHAs against allowed branches and opens a PR when updates are available.
25-29: CI Confirmed Deterministic – Branch-tracking Has No Effect in WorkflowsOur inspection shows that CI never invokes
git submodule update --remote(no references in .github workflows), and the submodule is checked out at the commit pinned in the superproject (e.g.34653a0…for imgui). As a result, builds remain fully deterministic despitebranch = hazelin .gitmodules.• Location: .gitmodules (lines 25–29)
• CI Workflows: no--remoteusage detected under .github (per your script)Action is optional:
- If you want to prevent any chance of floating updates, you may remove the
branch = hazelline and bump imgui via PRs.- If you want to auto-track the hazel branch, add a scheduled job (e.g. GitHub Actions cron) that runs
git submodule update --remote StarEngine/vendor/imgui, updates the SHA, and submits a PR.
42-45: Optimize CI Submodule Cloning with Shallow & Recursive InitVerified that running:
git submodule sync --recursive git submodule update --init --recursive --jobs 4 --depth 1successfully initializes all vendor submodules (including JoltPhysics) at depth 1 without errors, drastically reducing clone times in CI.
Action items:
Add the above two commands as a pre-build step in your CI workflow (e.g. GitHub Actions) to ensure shallow, recursive checkouts by default.
If any submodule truly requires full history or annotated tags for version detection, increment its depth selectively:
# Example: bump depth for JoltPhysics only - run: git submodule update --init StarEngine/vendor/JoltPhysics/JoltPhysics --depth 50No changes to
.gitmodulesare needed—this is purely a CI configuration enhancement.StarEditor/Resources/Shaders/Include/HLSL/Buffers.hlslh (2)
1-26: Summary consistency checkThe AI summary claims u_Camera is bound to set 0, binding 2, but the code sets binding 0, set 2. The code is authoritative; just flagging the discrepancy to avoid confusion in reviews/docs.
16-16: Bindings Verified
- u_Camera’s
[[vk::binding(0,2)]]corresponds to GLSLlayout(set = 2, binding = 0)inBuffers.glslh.- u_ScreenData’s
[[vk::binding(2,2)]]corresponds to GLSLlayout(set = 2, binding = 2)inBuffers.glslh.No changes required.
.gitignore (4)
7-9: LGTM: cache/object artifacts are correctly ignoredGood coverage for obj/, cache directories, and pycache. This will keep the repo clean of build/cache noise.
Also applies to: 17-17
45-47: LGTM: editor-local layout/config are correctly ignoredThese are environment-specific and shouldn’t be versioned.
61-65: LGTM: GPU dump patterns cover common Nsight/NVRHI artifactsGood catch including .nvdbg, .nv-gpudmp, and gpudmp.json. This aligns with the Vulkan/NVRHI migration.
11-16: Move ScriptCore exceptions below the folder ignore in .gitignoreTo track the DLL and PDB while still ignoring everything else under StarEditor/Resources/Scripts/, you’ll need to:
- Remove the existing explicit ignores for the two binaries (they’re redundant under the folder rule)
- Ensure the
StarEditor/Resources/Scripts/ignore stays before the exceptions- Add exception rules (
!…) immediately after the folder ruleSuggested diff for .gitignore:
# … -StarEditor/Resources/Scripts/StarEngine-ScriptCore.dll -StarEditor/Resources/Scripts/StarEngine-ScriptCore.pdb StarEditor/StarEditor Intermediates/ Binaries StarEditor/Resources/Scripts/ +!StarEditor/Resources/Scripts/StarEngine-ScriptCore.dll +!StarEditor/Resources/Scripts/StarEngine-ScriptCore.pdb __pycache__ # …• If you intend to version these two binaries, apply the above change.
• If they should remain ignored, no .gitignore edits are needed.Optional: consider Git LFS for large or frequently changing binaries.
StarEngine/src/StarEngine/Audio/AudioListener.cpp (1)
7-7: Profiling integration looks good.Including Profiler.h and adding SE_PROFILE_FUNCTION scopes is consistent and low-overhead. No behavior changes introduced.
Also applies to: 13-13, 44-44
StarEngine/src/StarEngine/Audio/AudioSource.cpp (1)
5-5: Profiling scopes: consistent and descriptive.Good coverage and consistent naming of SE_PROFILE_FUNCTION across ctor/dtor and mutators. This should make audio hot paths visible in traces.
Also applies to: 12-12, 39-39, 49-49, 59-59, 69-69, 82-82, 104-104, 119-119, 162-162, 172-172, 190-190, 203-203, 214-214, 227-227, 237-237, 247-247, 257-257, 267-267, 277-277, 287-287, 297-297, 307-307, 317-317
StarEngine/src/StarEngine/Core/Application.cpp (1)
172-206: BeginFrame/EndFrame are safe no-ops; no immediate GPU work on main threadI verified that in Renderer.cpp, both Renderer::BeginFrame() and Renderer::EndFrame() are currently empty stubs with only commented-out API calls, so they perform no immediate GPU operations. This means the Run loop’s sequencing around NextFrame()/Kick() does not cross threads improperly and no additional wrapping in a RenderCommand is needed.
StarEditor/Resources/Shaders/Include/Common/GTAO.slh (1)
4-8: Bit assignments overlap across AO and Reflection flags — confirm they live in separate bitfieldsAO and reflection occlusion masks both use BIT(1)/BIT(2). If they’re stored in the same mask, these collide; if they’re distinct fields, you’re fine. Please confirm storage sites.
If they must coexist in one mask, use distinct bits, e.g.:
-#define HZ_REFLECTION_OCCLUSION_METHOD_GTAO BIT(1) -#define HZ_REFLECTION_OCCLUSION_METHOD_HBAO BIT(2) +#define HZ_REFLECTION_OCCLUSION_METHOD_GTAO BIT(3) +#define HZ_REFLECTION_OCCLUSION_METHOD_HBAO BIT(4)StarEditor/Resources/Shaders/Include/GLSL/PBR.glslh (4)
1-2: LGTM! Standard GLSL header guards and includes.The use of
#pragma onceand inclusion ofCommon.glslhfollows standard GLSL conventions.
4-13: LGTM! Correct GGX/Trowbridge-Reitz NDF implementation.The normal distribution function is correctly implemented using Disney's reparametrization of roughness (α = roughness²). The mathematical formula is accurate for PBR rendering.
15-27: LGTM! Proper Schlick-GGX geometric attenuation implementation.Both functions correctly implement the geometric attenuation:
GaSchlickG1computes the single-term G functionGaSchlickGGXproperly combines both masking and shadowing terms using Smith's method- The k-factor remapping (k = (r²)/8) is the correct formulation for analytic lights as noted in the comment
50-59: LGTM! Correct Fresnel implementations.Both Fresnel functions are properly implemented:
FresnelSchlickcorrectly implements the standard Schlick approximationFresnelSchlickRoughnessproperly accounts for roughness in the Fresnel term for IBLStarEditor/Resources/Shaders/Include/GLSL/PBR_Resources.glslh (2)
1-2: LGTM! Proper header guard.Standard use of
#pragma oncefor GLSL header protection.
3-20: Ensure descriptor set layouts are isolated per pipelineOur ripgrep search shows that
PBR_Resources.glslhdeclares set 1, bindings 0–3, but so do many of your post-processing shaders (EdgeDetection, SSR, DOF, SceneComposite, etc.) at the same set 1 binding indices. This is fine if each rendering pass uses its own descriptor-set-layout, but will break if you ever share a single layout object across both PBR and post-processing pipelines.Please verify that:
StarEditor/Resources/Shaders/Include/GLSL/PBR_Resources.glslh(lines 4–11 and 15–19) is only used with its dedicatedVkDescriptorSetLayout, matching its set 0 and set 1 bindings.- Each post-processing shader under
StarEditor/Resources/Shaders/PostProcessing/*similarly has its own layout, covering exactly the bindings it uses (e.g. set 1 binding 0–8 in SSR).- In your Vulkan pipeline setup code, you create separate
VkDescriptorSetLayoutinstances for the PBR pipeline and for each post-processing pipeline (or a shared layout only if all shaders bound to that set share identical bindings).If you plan to reuse the same descriptor set across multiple passes, consolidate all required bindings into one layout (ensuring binding numbers do not collide). Otherwise, keep them in separate descriptor-set layouts to avoid silent binding mismatches at draw time.
StarEditor/Resources/Shaders/Include/GLSL/Lighting.glslh (3)
1-6: LGTM! Proper stage guard and includes.The fragment stage pragma and includes are appropriately set up for a lighting shader.
79-115: LGTM! Well-structured point light calculation.The PBR lighting calculation is correctly implemented with proper attenuation, BRDF terms, and energy conservation.
147-189: LGTM! Proper spot light implementation with cone attenuation.The spot light calculation correctly implements cone attenuation with cutoff angle and edge softening.
StarEditor/Resources/Shaders/Include/GLSL/EnvironmentMapping.glslh (6)
1-2: LGTM! Standard header setup.Proper use of pragma once and inclusion of Common.glslh.
9-17: LGTM! Correct Van der Corput sequence implementation.The bit-reversal implementation for the radical inverse is mathematically correct and well-optimized.
26-41: LGTM! Proper importance sampling for GGX distribution.The implementation correctly generates samples following the GGX distribution with proper tangent-to-world space transformation.
82-95: LGTM! Correct cube map coordinate generation.The function properly generates cube map texture coordinates for all six faces.
97-112: LGTM! Robust orthonormal basis generation.The implementation correctly handles the degenerate case and generates a proper orthonormal basis.
114-122: LGTM! Standard hemisphere sampling implementation.The uniform hemisphere sampling is correctly implemented as noted in the comment.
StarEditor/Resources/Shaders/Include/GLSL/ShadowMapping.glslh (5)
1-6: LGTM! Proper conditional compilation and includes.The stage guards ensure the shadow mapping code is only compiled for appropriate shader stages.
27-32: LGTM! Proper adaptive shadow bias calculation.The bias calculation correctly accounts for the surface normal angle relative to the light direction.
57-122: LGTM! Comprehensive Poisson disk distributions.Both the 64-sample and 16-sample Poisson distributions are properly defined for different quality/performance tradeoffs.
152-175: LGTM! Well-implemented blocker search for PCSS.The blocker distance calculation correctly implements the first step of PCSS with proper sampling.
286-310: LGTM! Complete spot shadow implementation with soft shadow support.The function properly accumulates shadows from multiple spot lights with both hard and soft shadow paths.
| !vendor/bin/ | ||
| !StarEngine/vendor/assimp/bin | ||
| !StarEngine/vendor/mono/bin |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ensure vendor bin exceptions actually re-include contents
Re-including only the directory may not re-include files beneath previously ignored bin folders. Use recursive exceptions to reliably include contents.
-!vendor/bin/
-!StarEngine/vendor/assimp/bin
-!StarEngine/vendor/mono/bin
+!vendor/bin/**
+!StarEngine/vendor/assimp/bin/**
+!StarEngine/vendor/mono/bin/**📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| !vendor/bin/ | |
| !StarEngine/vendor/assimp/bin | |
| !StarEngine/vendor/mono/bin | |
| !vendor/bin/** | |
| !StarEngine/vendor/assimp/bin/** | |
| !StarEngine/vendor/mono/bin/** |
🤖 Prompt for AI Agents
In .gitignore around lines 3 to 5, the negated rules only re-include the empty
directories (!vendor/bin/, !StarEngine/vendor/assimp/bin,
!StarEngine/vendor/mono/bin) which won’t re-include files under those previously
ignored folders; replace them with recursive negations (e.g. use patterns like
!vendor/bin/** and !StarEngine/vendor/assimp/bin/** and
!StarEngine/vendor/mono/bin/**) so the contents of those bin directories are
actually re-included by Git.
| StarEditor/SandboxProject/Assets/Scripts/Binaries/** | ||
| StarEditor/SandboxProject/Assets/SoundBank.hsb | ||
|
|
There was a problem hiding this comment.
Binaries folder re-include is ineffective; add recursive exception before re-ignoring System/Microsoft DLLs**
Because the folder was ignored with a recursive pattern, re-include the contents recursively as well; then selectively ignore system DLLs.
-StarEditor/SandboxProject/Assets/Scripts/Binaries/**
+StarEditor/SandboxProject/Assets/Scripts/Binaries/**
@@
-!StarEditor/SandboxProject/Assets/Scripts/Binaries
+!StarEditor/SandboxProject/Assets/Scripts/Binaries/**
# System DLLs
StarEditor/SandboxProject/Assets/Scripts/Binaries/System*.dll
StarEditor/SandboxProject/Assets/Scripts/Binaries/Microsoft*.dllRationale: you can’t re-include files under a directory excluded by a previous recursive rule unless you also unignore the contents with /**.
Also applies to: 66-71
🤖 Prompt for AI Agents
.gitignore around lines 19-21 (and similarly lines 66-71): the current
re-include of StarEditor/SandboxProject/Assets/Scripts/Binaries/** is
ineffective because a prior recursive ignore prevents re-including children;
update the section to first unignore the directory contents recursively (use an
exception like !StarEditor/SandboxProject/Assets/Scripts/Binaries/**) and then
re-ignore the unwanted system DLLs inside that path with explicit patterns (e.g.
ignore System*/Microsoft* DLLs) so the directory is included but specific DLLs
remain excluded; apply the same change to the other occurrence at lines 66-71.
| # Meta Files | ||
| **.meta | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
**Simplify extensions: prefer .meta and .log
**.ext is non-standard here; *.ext already matches in all directories.
-**.meta
+*.meta
@@
-**.log
+*.logAlso applies to: 55-57
🤖 Prompt for AI Agents
In .gitignore around lines 52-54 (and also apply same change to lines 55-57),
the patterns use non-standard double-star prefix like **.meta; change these to
standard wildcard form *.meta (and *.log or other extensions referenced on
55-57) so they match files in all directories correctly; update each instance of
**.ext to *.ext and keep other patterns intact.
| url = https://github.com/gabime/spdlog.git | ||
|
|
||
| [submodule "StarEngine/vendor/GLFW"] | ||
| path = StarEngine/vendor/GLFW | ||
| url = ../../starbounded-dev/glfw.git | ||
| branch = master | ||
| [submodule "StarEngine/vendor/imgui"] | ||
| path = StarEngine/vendor/imgui | ||
| url = ../../starbounded-dev/imgui.git | ||
| branch = docking | ||
| url = https://github.com/TheCherno/glfw | ||
|
|
||
| [submodule "StarEngine/vendor/glm"] | ||
| path = StarEngine/vendor/glm | ||
| url = ../../g-truc/glm.git | ||
| branch = master | ||
| [submodule "StarEngine/vendor/yaml-cpp"] | ||
| path = StarEngine/vendor/yaml-cpp | ||
| url = https://github.com/sheazywi/yaml-cpp | ||
| [submodule "StarEngine/vendor/imguizmo"] | ||
| path = StarEngine/vendor/imguizmo | ||
| url = https://github.com/sheazywi/imguizmo.git | ||
| url = https://github.com/g-truc/glm | ||
|
|
||
| [submodule "StarEngine/vendor/Box2D"] | ||
| path = StarEngine/vendor/Box2D | ||
| url = https://github.com/sheazywi/box2d | ||
| url = https://github.com/TheCherno/Box2D | ||
|
|
||
| [submodule "StarEngine/vendor/msdf-atlas-gen"] | ||
| path = StarEngine/vendor/msdf-atlas-gen | ||
| url = https://github.com/sheazywi/msdf-atlas-gen | ||
|
|
||
| [submodule "StarEngine/vendor/tracy/tracy"] | ||
| path = StarEngine/vendor/tracy/tracy | ||
| url = https://github.com/wolfpld/tracy/ | ||
|
|
||
| [submodule "StarEngine/vendor/imgui"] | ||
| path = StarEngine/vendor/imgui | ||
| url = https://github.com/StudioCherno/imgui | ||
| branch = hazel | ||
|
|
||
| [submodule "StarEngine/vendor/yaml-cpp"] | ||
| path = StarEngine/vendor/yaml-cpp | ||
| url = https://github.com/sheazywi/yaml-cpp | ||
|
|
||
| [submodule "StarEngine/vendor/NFD-Extended/NFD-Extended"] | ||
| path = StarEngine/vendor/NFD-Extended/NFD-Extended | ||
| url = https://github.com/btzy/nativefiledialog-extended | ||
|
|
||
| [submodule "StarEngine/vendor/backward-cpp"] | ||
| path = StarEngine/vendor/backward-cpp | ||
| url = https://github.com/bombela/backward-cpp | ||
|
|
||
| [submodule "StarEngine/vendor/JoltPhysics/JoltPhysics"] | ||
| path = StarEngine/vendor/JoltPhysics/JoltPhysics | ||
| url = https://github.com/jrouwe/JoltPhysics | ||
|
|
||
| [submodule "StarEngine/vendor/nvrhi"] | ||
| path = StarEngine/vendor/nvrhi | ||
| url = https://github.com/StudioCherno/nvrhi | ||
| branch = hazel |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify removal of imguiZmo and any legacy OpenGL-era submodules.
Since imguiZmo blocks were removed, make sure there are no lingering includes or build references.
Run these checks:
If anything pops, I can provide targeted removal patches.
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Searching for ImGuizmo references (includes, links, cmake):"
rg -n -C2 -i 'imguizmo|ImGuizmo' --type-add 'cmake:*.cmake' --type-add 'build:*.build' --type-add 'premake:*.lua' --type-add 'cpp:*.{h,hpp,c,cc,cpp,ixx}' || true
echo
echo "Searching for OpenGL-era module references:"
rg -n -C2 -i 'gl(ad|fw|3|context|Bindable|VertexArray|Shader|Glew|Glad)' --type=cpp || trueLength of output: 327796
Action required — ImGuizmo & legacy OpenGL artifacts are still referenced
The grep results show ImGuizmo symbols/headers and an OpenGL loader (GLAD) still present. Either restore the removed imguiZmo submodule or remove/replace these references and update build files.
Files/locations that need attention (examples from the search):
- StarEditor/src/EditorLayer.cpp — #include "ImGuizmo.h" and many ImGuizmo:: calls (use/manipulate/isUsing/isOver).
- StarEngine/src/StarEngine/Platform/Vulkan/VulkanImGuiLayer.cpp — includes StarEngine/ImGui/ImGuizmo.h and calls ImGuizmo::BeginFrame(); uses imgui_impl_glfw backend.
- StarEngine/src/StarEngine/ImGui/ImGuizmo.h, ImGuizmo.cpp — local ImGuizmo implementation/header present.
- StarEngine/src/StarEngine/ImGui/ImGuiLayer.cpp & ImGuiBuild.cpp — imgui_impl_glfw includes and ImGuizmo::BeginFrame().
- StarEngine/vendor/GLAD/include/glad/glad.h — GLAD (OpenGL loader) still vendored.
- Multiple core/platform files include <GLFW/glfw3.h> and use glfw* APIs (Core/Window.cpp, Core/Input.cpp, Platform/Vulkan/*) — verify whether these are intentional (windowing/input) or leftover OpenGL-era usage.
Recommended next steps (pick one):
- Restore the removed imguiZmo/submodule if ImGuizmo and GLAD are still required; or
- Remove/replace all ImGuizmo and GLAD references, update includes and CMake/BUILD targets, and ensure no remaining compile/link references.
If you want, I can prepare targeted removal patches or a list of exact files/lines to change — tell me which approach to take.
🤖 Prompt for AI Agents
In .gitmodules around lines 3 to 49, ImGuizmo and GLAD artifacts are still
referenced even though the imguiZmo submodule was removed; choose either restore
the removed imguiZmo submodule or remove/replace all ImGuizmo/GLAD usage and
update build metadata. If restoring: re-add the imguiZmo/GLAD submodule entries
to .gitmodules and run git submodule update --init, then ensure CMake targets
reference the restored paths. If removing: delete
StarEngine/src/StarEngine/ImGui/ImGuizmo.h/.cpp and all ImGuizmo includes/calls
(e.g., StarEditor/src/EditorLayer.cpp, VulkanImGuiLayer.cpp, ImGuiLayer.cpp),
replace ImGuizmo logic with an alternative or guard it behind a feature flag,
remove vendored GLAD (StarEngine/vendor/GLAD) and replace any <glad/glad.h>
usages with the appropriate Vulkan/loader abstractions or platform-neutral
headers, and update all CMake/BUILD files and targets to remove submodule paths,
include directories, and link dependencies so no compile/link references remain.
| @@ -0,0 +1,3 @@ | |||
| #pragma once | |||
|
|
|||
| #define BIT(x) (1 << x) | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Make BIT macro type-safe and parenthesized to avoid sign/precedence pitfalls
Right now, BIT uses a signed 1 and doesn’t parenthesize x. This can produce negative masks for high bits and unexpected results in composed expressions.
Apply this diff:
-#define BIT(x) (1 << x)
+#define BIT(x) (1u << (x))This is safe in both HLSL and GLSL and composes better with other macros.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| #define BIT(x) (1 << x) | |
| #define BIT(x) (1u << (x)) |
🤖 Prompt for AI Agents
In StarEditor/Resources/Shaders/Include/Common/Common.slh around line 3, the BIT
macro uses a signed literal and lacks parentheses which can produce negative
masks for high bits and cause precedence issues; replace it with a type-safe,
fully parenthesized form using an unsigned literal and parentheses around the
parameter and result (e.g. use 1u and wrap x and the whole expression) so it is
safe in both HLSL and GLSL and composes correctly with other macros.
| // NOTE(Yan): Mats[0] because we only support ONE shadow casting spot light at the moment and it MUST be index 0 | ||
| vec4 coords = u_SpotLightMatrices.Mats[0] * vec4(worldPos, 1.0f); |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Document the single shadow-casting spot light limitation.
The comment indicates only one shadow-casting spot light is supported at index 0. This is a significant limitation that should be prominently documented.
🏁 Script executed:
#!/bin/bash
# Check if this limitation is documented elsewhere or if there are plans to support multiple shadow-casting spot lights
echo "Searching for documentation about spot light shadow limitations..."
rg -i "shadow.*spot.*light.*limit|one.*shadow.*spot" --type-add 'docs:*.{md,txt,rst}' -t docs
rg -i "TODO.*spot.*shadow|FIXME.*spot.*shadow" --type-add 'glsl:*.{glsl,glslh}' -t glslLength of output: 328
Document single shadow-casting spot-light limitation
After searching the repository, there is no existing documentation or TODO mentioning that only one shadow-casting spot light is supported (it MUST be at index 0). We should explicitly document this limitation in both the shader and the project’s user/developer documentation:
• In StarEditor/Resources/Shaders/Include/GLSL/ShadowMapping.glslh (around lines 299–300) – add or expand a top-of-file comment stating:
“Only one shadow-casting spot light is supported (Mats[0]). Additional spot lights will not cast shadows.”
• In the project docs (e.g. docs/, README.md, or a dedicated Graphics/Lighting guide) – create or update a section under “Shadow Mapping” that calls out:
- The one-light limitation
- How to configure index 0 for the shadow-casting spot light
- Future work if/when multiple shadow-casting spot lights will be supported
This will ensure developers and users are aware of the current restriction and avoid confusion when working with multiple spot lights.
🤖 Prompt for AI Agents
In StarEditor/Resources/Shaders/Include/GLSL/ShadowMapping.glslh around lines
299–300, the shader currently uses u_SpotLightMatrices.Mats[0] with an implicit
single-shadow-casting-spot-light requirement; add a short top-of-file comment
stating: "Only one shadow-casting spot light is supported (Mats[0]). Additional
spot lights will not cast shadows." and expand the inline comment at lines
299–300 to reference this top-of-file note and that the shadow-caster must be at
index 0; additionally, update the project documentation (e.g., docs/ or
README.md under Shadow Mapping or Graphics/Lighting) to document the one-light
limitation, instructions to ensure the shadow-casting spot light is placed at
index 0, and a brief note about future work to support multiple shadow-casting
spot lights.
| #if (USE_HALF_FLOAT_PRECISION != 0) | ||
| #if 1 // old fp16 approach (<SM6.2) | ||
| typedef min16float lpfloat; | ||
| typedef min16float2 lpfloat2; | ||
| typedef min16float3 lpfloat3; | ||
| typedef min16float4 lpfloat4; | ||
| typedef min16float3x3 lpfloat3x3; | ||
| #else // new fp16 approach (requires SM6.2 and -enable-16bit-types) - WARNING: perf degradation noticed on some HW, while the old (min16float) path is mostly at least a minor perf gain so this is more useful for quality testing | ||
| typedef float16_t lpfloat; | ||
| typedef float16_t2 lpfloat2; | ||
| typedef float16_t3 lpfloat3; | ||
| typedef float16_t4 lpfloat4; | ||
| typedef float16_t3x3 lpfloat3x3; | ||
| #endif |
There was a problem hiding this comment.
New fp16 path likely won’t compile: use vector/matrix templates for float16_t
DXC/HLSL does not provide float16_t2/3/4 or float16_t3x3 aliases. Use vector<float16_t, N> and matrix<float16_t, R, C>.
Apply this diff in the new fp16 branch:
- #else // new fp16 approach (requires SM6.2 and -enable-16bit-types) - WARNING: perf degradation noticed on some HW, while the old (min16float) path is mostly at least a minor perf gain so this is more useful for quality testing
- typedef float16_t lpfloat;
- typedef float16_t2 lpfloat2;
- typedef float16_t3 lpfloat3;
- typedef float16_t4 lpfloat4;
- typedef float16_t3x3 lpfloat3x3;
+ #else // new fp16 approach (requires SM6.2 and -enable-16bit-types)
+ typedef float16_t lpfloat;
+ typedef vector<float16_t, 2> lpfloat2;
+ typedef vector<float16_t, 3> lpfloat3;
+ typedef vector<float16_t, 4> lpfloat4;
+ typedef matrix<float16_t, 3, 3> lpfloat3x3;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| #if (USE_HALF_FLOAT_PRECISION != 0) | |
| #if 1 // old fp16 approach (<SM6.2) | |
| typedef min16float lpfloat; | |
| typedef min16float2 lpfloat2; | |
| typedef min16float3 lpfloat3; | |
| typedef min16float4 lpfloat4; | |
| typedef min16float3x3 lpfloat3x3; | |
| #else // new fp16 approach (requires SM6.2 and -enable-16bit-types) - WARNING: perf degradation noticed on some HW, while the old (min16float) path is mostly at least a minor perf gain so this is more useful for quality testing | |
| typedef float16_t lpfloat; | |
| typedef float16_t2 lpfloat2; | |
| typedef float16_t3 lpfloat3; | |
| typedef float16_t4 lpfloat4; | |
| typedef float16_t3x3 lpfloat3x3; | |
| #endif | |
| #if (USE_HALF_FLOAT_PRECISION != 0) | |
| #if 1 // old fp16 approach (<SM6.2) | |
| typedef min16float lpfloat; | |
| typedef min16float2 lpfloat2; | |
| typedef min16float3 lpfloat3; | |
| typedef min16float4 lpfloat4; | |
| typedef min16float3x3 lpfloat3x3; | |
| #else // new fp16 approach (requires SM6.2 and -enable-16bit-types) | |
| typedef float16_t lpfloat; | |
| typedef vector<float16_t, 2> lpfloat2; | |
| typedef vector<float16_t, 3> lpfloat3; | |
| typedef vector<float16_t, 4> lpfloat4; | |
| typedef matrix<float16_t, 3, 3> lpfloat3x3; | |
| #endif |
🤖 Prompt for AI Agents
In StarEditor/Resources/Shaders/Include/HLSL/Types.hlslh around lines 3-16, the
new fp16 branch uses non-existent aliases like float16_t2/3/4 and float16_t3x3;
replace those with the correct template forms: use vector<float16_t, N> for
2/3/4 component vectors and matrix<float16_t, 3, 3> for 3x3, i.e. typedef
vector<float16_t,2> lpfloat2, vector<float16_t,3> lpfloat3, vector<float16_t,4>
lpfloat4 and typedef matrix<float16_t,3,3> lpfloat3x3; keep lpfloat as float16_t
and ensure the typedefs are inside the new fp16 branch so the old min16float
path remains unchanged.
| Renderer::SetConfig(specification.RenderConfig); | ||
|
|
||
| m_Window = Window::Create(WindowProps(m_Specification.Name)); | ||
| m_Window->SetEventCallback(SE_BIND_EVENT_FN(Application::OnEvent)); | ||
| WindowSpecification windowSpec; | ||
| windowSpec.Title = specification.Name; | ||
| windowSpec.Width = specification.WindowWidth; | ||
| windowSpec.Height = specification.WindowHeight; | ||
| windowSpec.Decorated = specification.WindowDecorated; | ||
| windowSpec.Fullscreen = specification.Fullscreen; | ||
| windowSpec.VSync = specification.VSync; | ||
| windowSpec.IconPath = specification.IconPath; | ||
| m_Window = std::unique_ptr<Window>(Window::Create(windowSpec)); | ||
| m_Window->Init(); | ||
| m_Window->SetEventCallback([this](Event& e) { OnEvent(e); }); | ||
|
|
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
VSync setting is not honored downstream
You set windowSpec.VSync = specification.VSync; but Window::Init hardcodes deviceParams.vsyncEnabled = false (see Window.cpp: Init). The app-level VSync configuration is effectively ignored.
Proposed fix in Window::Init (StarEngine/src/StarEngine/Core/Window.cpp):
- deviceParams.vsyncEnabled = false;
+ deviceParams.vsyncEnabled = m_Specification.VSync;Optionally, wire this into present mode selection if applicable. Also check consistency with:
- deviceParams.maxFramesInFlight (currently 1) vs Renderer::GetConfig().FramesInFlight
- swapChainBufferCount (3) to avoid stutter when VSync is ON.
🏁 Script executed:
#!/bin/bash
rg -n "vsyncEnabled" -C2 StarEngine/src
rg -n "FramesInFlight" -C2 StarEngine/src/StarEngine/RendererLength of output: 13820
Fix Window VSync configuration propagation
The application’s VSync setting isn’t being applied: in StarEngine/src/StarEngine/Core/Window.cpp (around line 96), deviceParams.vsyncEnabled is hardcoded to false, so windowSpec.VSync = specification.VSync; has no effect.
• Location to update:
StarEngine/src/StarEngine/Core/Window.cpp:96
ChangetodeviceParams.vsyncEnabled = false;deviceParams.vsyncEnabled = m_Specification.VSync;
• Also verify present-mode selection in Vulkan (VulkanSwapChain.cpp:73) and DXGI present flags in DX11/DX12 to ensure they respect deviceParams.vsyncEnabled.
• Optionally review maxFramesInFlight (default 2 in DeviceManager.h) vs. RendererConfig::FramesInFlight (default 3) and the swapchain buffer count (3) when VSync is enabled to avoid rendering stalls.
--- a/StarEngine/src/StarEngine/Core/Window.cpp
+++ b/StarEngine/src/StarEngine/Core/Window.cpp
@@ -93,7 +93,7 @@ void Window::Init()
deviceParams.backBufferHeight = m_Specification.Height;
- deviceParams.vsyncEnabled = false;
+ deviceParams.vsyncEnabled = m_Specification.VSync;
deviceParams.enableDebugRuntime = true;
deviceParams.ignoredVulkanValidationMessageLocations = { 0xc81ad50e };
});🤖 Prompt for AI Agents
In StarEngine/src/StarEngine/Core/Window.cpp around line 96,
deviceParams.vsyncEnabled is hardcoded to false so the WindowSpecification.VSync
value set in Application.cpp is ignored; change the assignment to use the window
specification VSync member (i.e., set deviceParams.vsyncEnabled from the
Window/Specification VSync field) so the application VSync propagates through
deviceParams. After that, verify Vulkan present-mode selection in
StarEngine/src/StarEngine/Renderer/Vulkan/VulkanSwapChain.cpp (around line 73)
and DXGI present flags in the DX11/DX12 swapchain code so they respect
deviceParams.vsyncEnabled, and optionally reconcile maxFramesInFlight
(DeviceManager.h) vs RendererConfig::FramesInFlight and swapchain buffer count
to avoid stalls when VSync is enabled.
| Application::~Application() | ||
| { | ||
| SE_PROFILE_FUNCTION(); | ||
|
|
||
| ScriptEngine::Shutdown(); | ||
|
|
||
| NFD::Quit(); | ||
|
|
||
| EditorApplicationSettingsSerializer::SaveSettings(); | ||
|
|
||
| m_Window->SetEventCallback([](Event& e) {}); | ||
|
|
||
| m_RenderThread.Terminate(); | ||
|
|
||
| for (Layer* layer : m_LayerStack) | ||
| { | ||
| layer->OnDetach(); | ||
| delete layer; | ||
| } | ||
|
|
||
| //ScriptEngine::Shutdown(); | ||
| Project::SetActive(nullptr); | ||
| Font::Shutdown(); | ||
| AudioEngine::Shutdown(); | ||
|
|
||
| Renderer::Shutdown(); | ||
|
|
||
| delete m_Profiler; | ||
| m_Profiler = nullptr; | ||
| } |
There was a problem hiding this comment.
Fix shutdown ordering: Terminating render thread before layer/renderer teardown can drop GPU work and cause leaks
m_RenderThread.Terminate() runs before layer->OnDetach() and Renderer::Shutdown(). If any layer or the renderer enqueues GPU teardown via Renderer::Submit, those jobs will never execute, leading to resource leaks or hangs on next init. Keep the render thread alive through layer and renderer shutdown, flush, then terminate.
Apply this diff to correct the teardown sequence and ensure all queued work completes:
@@
- m_RenderThread.Terminate();
+ // Keep render thread alive while layers/renderer tear down so their GPU jobs can run
@@
- Renderer::Shutdown();
+ Renderer::Shutdown();
+
+ // Ensure queued GPU work from layer/renderer shutdown is fully processed
+ m_RenderThread.BlockUntilRenderComplete();
+ m_RenderThread.Terminate();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| Application::~Application() | |
| { | |
| SE_PROFILE_FUNCTION(); | |
| ScriptEngine::Shutdown(); | |
| NFD::Quit(); | |
| EditorApplicationSettingsSerializer::SaveSettings(); | |
| m_Window->SetEventCallback([](Event& e) {}); | |
| m_RenderThread.Terminate(); | |
| for (Layer* layer : m_LayerStack) | |
| { | |
| layer->OnDetach(); | |
| delete layer; | |
| } | |
| //ScriptEngine::Shutdown(); | |
| Project::SetActive(nullptr); | |
| Font::Shutdown(); | |
| AudioEngine::Shutdown(); | |
| Renderer::Shutdown(); | |
| delete m_Profiler; | |
| m_Profiler = nullptr; | |
| } | |
| Application::~Application() | |
| { | |
| ScriptEngine::Shutdown(); | |
| NFD::Quit(); | |
| EditorApplicationSettingsSerializer::SaveSettings(); | |
| m_Window->SetEventCallback([](Event& e) {}); | |
| // Keep render thread alive while layers/renderer tear down so their GPU jobs can run | |
| for (Layer* layer : m_LayerStack) | |
| { | |
| layer->OnDetach(); | |
| delete layer; | |
| } | |
| //ScriptEngine::Shutdown(); | |
| Project::SetActive(nullptr); | |
| Font::Shutdown(); | |
| AudioEngine::Shutdown(); | |
| Renderer::Shutdown(); | |
| // Ensure queued GPU work from layer/renderer shutdown is fully processed | |
| m_RenderThread.BlockUntilRenderComplete(); | |
| m_RenderThread.Terminate(); | |
| delete m_Profiler; | |
| m_Profiler = nullptr; | |
| } |
🤖 Prompt for AI Agents
In StarEngine/src/StarEngine/Core/Application.cpp around lines 99 to 126, the
render thread is terminated before layers are detached and the renderer is shut
down which can drop GPU work; to fix, move m_RenderThread.Terminate() to after
the loop that calls layer->OnDetach() and after Renderer::Shutdown(), ensure you
flush/flush-and-wait any pending render commands or call the renderer's
GPU-queue flush/synchronization API so all submitted GPU work completes before
terminating the render thread, then call m_RenderThread.Terminate() and finally
perform resource shutdowns (Project::SetActive(nullptr), Font::Shutdown(),
AudioEngine::Shutdown(), delete m_Profiler).
| void Application::ProcessEvents() | ||
| { | ||
| Input::TransitionPressedKeys(); | ||
| Input::TransitionPressedButtons(); | ||
|
|
||
| m_Window->ProcessEvents(); | ||
|
|
||
| // Note (0x): we have no control over what func() does. holding this lock while calling func() is a bad idea: | ||
| // 1) func() might be slow (means we hold the lock for ages) | ||
| // 2) func() might result in events getting queued, in which case we have a deadlock | ||
| std::scoped_lock<std::mutex> lock(m_EventQueueMutex); | ||
|
|
||
| // Process custom event queue, up until we encounter an event that is not yet sync'd | ||
| // If application queues such events, then it is the application's responsibility to call | ||
| // SyncEvents() at the appropriate time. | ||
| while (m_EventQueue.size() > 0) | ||
| { | ||
| const auto& [synced, func] = m_EventQueue.front(); | ||
| if (!synced) | ||
| { | ||
| break; | ||
| } | ||
| func(); | ||
| m_EventQueue.pop_front(); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Deadlock/perf hazard: Holding event-queue mutex while executing callbacks
The comment correctly warns against holding the lock while calling func(), but the code still does exactly that. If a callback queues another event, it will attempt to lock m_EventQueueMutex and deadlock. Also, long-running callbacks will block producers.
Apply this lock-free execution pattern (drain sync’d items under lock, then run them unlocked):
@@
- std::scoped_lock<std::mutex> lock(m_EventQueueMutex);
-
- // Process custom event queue, up until we encounter an event that is not yet sync'd
- // If application queues such events, then it is the application's responsibility to call
- // SyncEvents() at the appropriate time.
- while (m_EventQueue.size() > 0)
- {
- const auto& [synced, func] = m_EventQueue.front();
- if (!synced)
- {
- break;
- }
- func();
- m_EventQueue.pop_front();
- }
+ // Drain only sync'd events into a local list, then execute without holding the lock
+ std::deque<std::function<void()>> toProcess;
+ {
+ std::scoped_lock<std::mutex> lock(m_EventQueueMutex);
+ while (!m_EventQueue.empty() && m_EventQueue.front().first)
+ {
+ toProcess.emplace_back(std::move(m_EventQueue.front().second));
+ m_EventQueue.pop_front();
+ }
+ }
+ for (auto& func : toProcess)
+ func();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| void Application::ProcessEvents() | |
| { | |
| Input::TransitionPressedKeys(); | |
| Input::TransitionPressedButtons(); | |
| m_Window->ProcessEvents(); | |
| // Note (0x): we have no control over what func() does. holding this lock while calling func() is a bad idea: | |
| // 1) func() might be slow (means we hold the lock for ages) | |
| // 2) func() might result in events getting queued, in which case we have a deadlock | |
| std::scoped_lock<std::mutex> lock(m_EventQueueMutex); | |
| // Process custom event queue, up until we encounter an event that is not yet sync'd | |
| // If application queues such events, then it is the application's responsibility to call | |
| // SyncEvents() at the appropriate time. | |
| while (m_EventQueue.size() > 0) | |
| { | |
| const auto& [synced, func] = m_EventQueue.front(); | |
| if (!synced) | |
| { | |
| break; | |
| } | |
| func(); | |
| m_EventQueue.pop_front(); | |
| } | |
| } | |
| void Application::ProcessEvents() | |
| { | |
| Input::TransitionPressedKeys(); | |
| Input::TransitionPressedButtons(); | |
| m_Window->ProcessEvents(); | |
| // Drain only sync'd events into a local list, then execute without holding the lock | |
| std::deque<std::function<void()>> toProcess; | |
| { | |
| std::scoped_lock<std::mutex> lock(m_EventQueueMutex); | |
| while (!m_EventQueue.empty() && m_EventQueue.front().first) | |
| { | |
| toProcess.emplace_back(std::move(m_EventQueue.front().second)); | |
| m_EventQueue.pop_front(); | |
| } | |
| } | |
| for (auto& func : toProcess) | |
| func(); | |
| } |
🤖 Prompt for AI Agents
In StarEngine/src/StarEngine/Core/Application.cpp around lines 280 to 305, the
code currently holds m_EventQueueMutex while calling queued callbacks which can
deadlock or block producers; change to a lock-free execution pattern: under the
mutex, iterate the front of m_EventQueue, move all entries whose `synced` flag
is true into a local std::vector<std::function<void()>> (removing them from
m_EventQueue) and then release the lock; after unlocking, iterate that local
vector and invoke each function. Ensure functions are moved (not copied) out of
the deque while under lock and that the mutex is not held during invocation.
github-project-automation
bot
moved this from In Progress
to Done
in StarEngine Project Roadmap
A complete refactor of the renderer, removing OpenGL and replacing it with Vulkan using NVRHI.
Changing alot of other stuff.
Summary by CodeRabbit
New Features
Refactor
Bug Fixes
Chores
Removals