Bugfix/check time diffs #330
Conversation
WalkthroughRefactors time storage from microseconds to milliseconds, redesigns MaterialBuilder to accept material names and texture arrays, simplifies MeshBuilder::createCube to a single size parameter (and expands cube geometry), and adds GPU feature enumeration; multiple call sites updated to match new APIs. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/Engine/Platform/sdl2/SDL2Timer.cpp (1)
34-38: Critical: Incorrect unit conversion.
SDL_GetTicks64()already returns milliseconds, so multiplying by 1000 incorrectly converts the result to microseconds. Since the function is now namedgetMilliCurrentSeconds(), it should return milliseconds directly without the multiplication. This will cause all timing to be 1000× too large on SDL2 platforms, breaking FPS counters, animations, and any time-dependent behavior.🐛 Proposed fix
i64 SDL2Timer::getMilliCurrentSeconds() { - // There is no micro seconds tick from SDL2 - i64 ticks = static_cast<i64>(SDL_GetTicks64() * 1000l); + // SDL_GetTicks64() returns milliseconds since SDL library initialization + i64 ticks = static_cast<i64>(SDL_GetTicks64()); return ticks; }src/Engine/Platform/AbstractTimer.h (1)
82-100: Dead code:diffmodification has no effect on the returned value.The variable
diffis modified afterTime dt(diff)is constructed (lines 94-96), butdtis returned unchanged. The clamping logic has no effect on the return value.If the intent is to clamp the time difference, move the clamping before constructing
dt:🐛 Proposed fix
inline Time AbstractTimer::getTimeDiff() { const i64 currentTime = getMilliCurrentSeconds(); if (mLastTime == 0L) { mLastTime = currentTime; return 0l; } i64 diff = currentTime - mLastTime; mLastTime = currentTime; - Time dt(diff); if (mReqTimeSlice != -1) { if (diff > 1000) { diff = AbstractTimer::getRequestedTimeStep(); } } + Time dt(diff); return dt; }
🤖 Fix all issues with AI agents
In @src/Engine/Animation/AnimatorBase.h:
- Around line 85-86: VectorKey currently defaults Time to 1.0f while RotationKey
and ScalingKey default to 0.0, causing inconsistent behavior; change VectorKey's
Time default to 0.0f (and use default-initialized Value) so all key structs
(VectorKey, RotationKey, ScalingKey) share the same Time initialization
semantics — update the Time member in the VectorKey definition to 0.0f to match
the others.
In @src/Engine/Animation/AnimatorComponent.cpp:
- Line 99: Replace the stray std::cout debug prints in AnimatorComponent.cpp
with the project's logging API (e.g., osre_debug) or remove them: locate the
statement printing "Time: ..." (the line referencing time, mLastTime, delta and
ticksPerSecond) and the similar print at the later occurrence (around the usage
of mLastTime/ticksPerSecond), and change them to a concise osre_debug call that
includes the same contextual values or delete them if not needed; ensure you use
the logging macro/function consistent with other code and keep the message
succinct.
- Around line 207-211: The transform is inadvertently accumulating because the
code multiplies the existing matrix by toMat4(q) and then reassigns the
reference; change it so you compute the frame's transform from scratch: set a
local glm::mat4 (or assign mTransformArray[currentAnimationTrack]) to identity,
then apply transform = toMat4(q) (not *=), then apply scale(...) and
translate(...), and remove the redundant final
mTransformArray[currentAnimationTrack] = transform since transform is a
reference; update the block around mTransformArray/currentAnimationTrack,
toMat4(q), scale and translate accordingly.
In @src/Engine/App/Entity.cpp:
- Line 31: The #include <iostream> is only needed for the debug print at the
diagnostic near line 77; remove the diagnostic and then delete the unused
include to avoid an unnecessary dependency. Locate the debug output call (e.g.,
the diagnostic/console print in Entity::... around that spot) and remove it,
then remove the corresponding #include <iostream> from the top of Entity.cpp; if
you still need logging, replace the debug print with the project's logging
facility (e.g., use the logger class/function used elsewhere) instead of
iostream before removing the include.
- Line 77: Remove the stray debug print causing per-entity stdout spam by
deleting the std::cout << "dt = " << dt.asMilliSeconds() << " ms\n"; line in
Entity.cpp (and remove the corresponding <iostream> include if now unused); if
timing logs are needed instead, replace it with the existing Logger API (use the
class/methods referenced near lines 29/39) or wrap the print in a DEBUG-only
guard.
In @src/Engine/Common/osre_common.h:
- Line 197: The comment for the member i64 milliSeconds incorrectly says
"microseconds"; update the comment to reflect that this member stores
milliseconds (e.g., "The stored time stamp in milliseconds") so the
documentation matches the variable name and units.
In @src/Engine/Platform/sdl2/SDL2Timer.h:
- Line 45: The method getMilliCurrentSeconds() in SDL2Timer (SDL2Timer.cpp)
currently returns microseconds by multiplying SDL_GetTicks64() by 1000; change
the implementation to return milliseconds instead by using SDL_GetTicks64()
directly (or dividing the current microsecond value by 1000 if you prefer),
ensuring the return type i64 still represents milliseconds and update any
comment if present; keep the function signature getMilliCurrentSeconds() and
avoid changing units to prevent silent unit mismatches.
In @src/Engine/Platform/win32/Win32Timer.h:
- Around line 44-45: Typo in the documentation comment for the getter of current
milliseconds in Win32Timer.h: change the misspelled ",illiseconds" to
"milliseconds" in the comment (the triple-slash docblock that reads "Will return
the current milliseconds." / "@return The current ,illiseconds.") so the @return
line correctly reads "The current milliseconds."
In @src/Engine/RenderBackend/RenderBackendService.cpp:
- Around line 306-317: The enumerateGPU() method allocates a new GPUFeatureSet
every call causing a leak; modify RenderBackendService::enumerateGPU to check
mGPUFeatureSet and either return the existing object or delete it before
allocating a new one (or better, replace raw pointer with a smart pointer member
like std::unique_ptr<GPUFeatureSet> and reset it) so repeated calls do not leak;
ensure mGPUFeatureSet is initialized to nullptr in the constructor and properly
cleared in the destructor or when replaced.
In @src/Engine/RenderBackend/RenderBackendService.h:
- Line 206: enumerateGPU() currently returns hardcoded defaults; change it to
return the real GPUFeatureSet produced by the OpenGL backend by invoking or
reusing OGLRenderBackend::enumerateGPUCaps() (or its shared logic) so the
returned GPUFeatureSet contains the real ShaderLanguageType and shaderVersion
string from glGetString(GL_SHADING_LANGUAGE_VERSION) instead of fixed
ShaderLanguageType::GLSL and "450". Update enumerateGPU() to obtain the
capabilities from the OpenGL query path (or centralize capability parsing into a
helper used by both) and return that GPUFeatureSet to callers.
🧹 Nitpick comments (4)
src/Engine/RenderBackend/FontService.h (1)
60-62: Consider adding null-check for the returned material.The
createBuildinMaterialfunction can returnnullptrin several cases (e.g., cache creation failure, invalid shader sources). Whilefont.Name.empty()is checked earlier, other failure paths exist. Consider adding a null-check before returning for robustness.💡 Suggested improvement
Material *fontMaterial = MaterialBuilder::createBuildinMaterial(font.Name + ".mat", texResArray, VertexType::RenderVertex); + if (fontMaterial == nullptr) { + delete texRes; + return nullptr; + } return fontMaterial;samples/05_Animation/Animation.cpp (1)
75-80: Remove commented-out code before merging.The commented-out cube creation on line 77 appears to be a debugging artifact. Consider removing it to keep the sample code clean.
🧹 Suggested cleanup
MeshBuilder meshBuilder; - - if (Mesh *mesh = meshBuilder.createTriangle(VertexType::ColorVertex, BufferAccessType::ReadOnly).getMesh(); mesh != nullptr) { - //if (Mesh *mesh = meshBuilder.createCube(VertexType::ColorVertex, 0.5, 0.5, 0.5, BufferAccessType::ReadOnly).getMesh(); mesh != nullptr) { + if (Mesh *mesh = meshBuilder.createTriangle(VertexType::ColorVertex, BufferAccessType::ReadOnly).getMesh(); mesh != nullptr) { auto *rc = static_cast<RenderComponent*>(mEntity->getComponent(ComponentType::RenderComponentType)); rc->addStaticMesh(mesh); }src/Engine/RenderBackend/MeshBuilder.cpp (1)
182-183: Consider extracting repeated material creation pattern.The same two-line pattern for creating a default material without textures is repeated across
createTriangle,allocQuads,createCube, andallocPoints. Consider extracting a helper method to reduce duplication.// Example helper in private section or as static function: static Material* createDefaultMaterial(VertexType type) { TextureResourceArray texResArray; return MaterialBuilder::createBuildinMaterial("default.mat", texResArray, type); }src/Engine/RenderBackend/RenderBackendService.h (1)
57-66: Consider expanding ShaderLanguageType for future-proofing.The
ShaderLanguageTypeenum currently only includesGLSL. If the engine may support other backends in the future (Vulkan/SPIR-V, DirectX/HLSL, Metal), consider adding placeholder values now to maintain enum stability.enum class ShaderLanguageType { Invalid = -1, GLSL = 0, + HLSL, + SPIRV, + Metal, Count };
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (29)
contrib/cppcorecontrib/vcpkgsamples/04_terrain/TerrainRendering.cppsamples/05_Animation/Animation.cppsrc/Editor/src/RenderView/MainRenderView.cppsrc/Engine/Animation/AnimatorBase.hsrc/Engine/Animation/AnimatorComponent.cppsrc/Engine/App/AssimpWrapper.cppsrc/Engine/App/AssimpWrapper.hsrc/Engine/App/Entity.cppsrc/Engine/App/ParticleEmitter.cppsrc/Engine/Common/osre_common.hsrc/Engine/Platform/AbstractTimer.hsrc/Engine/Platform/sdl2/SDL2Timer.cppsrc/Engine/Platform/sdl2/SDL2Timer.hsrc/Engine/Platform/win32/Win32Timer.cppsrc/Engine/Platform/win32/Win32Timer.hsrc/Engine/Profiling/FPSCounter.cppsrc/Engine/RenderBackend/2D/RenderPass2D.hsrc/Engine/RenderBackend/DbgRenderer.cppsrc/Engine/RenderBackend/FontService.hsrc/Engine/RenderBackend/MaterialBuilder.cppsrc/Engine/RenderBackend/MaterialBuilder.hsrc/Engine/RenderBackend/MeshBuilder.cppsrc/Engine/RenderBackend/RenderBackendService.cppsrc/Engine/RenderBackend/RenderBackendService.htest/RenderTests/src/PrimTypeRenderTest.cpptest/RenderTests/src/RenderBufferAccessTest.cpptest/RenderTests/src/Textures/BaseTextureRenderTest.cpp
💤 Files with no reviewable changes (1)
- src/Engine/App/AssimpWrapper.h
🧰 Additional context used
🧬 Code graph analysis (19)
src/Engine/App/Entity.cpp (1)
test/UnitTests/src/Scene/ComponentTest.cpp (2)
dt(60-63)dt(60-60)
src/Engine/Platform/win32/Win32Timer.h (2)
src/Engine/Platform/sdl2/SDL2Timer.cpp (2)
getMilliCurrentSeconds(34-38)getMilliCurrentSeconds(34-34)src/Engine/Platform/win32/Win32Timer.cpp (2)
getMilliCurrentSeconds(33-40)getMilliCurrentSeconds(33-33)
test/RenderTests/src/PrimTypeRenderTest.cpp (1)
src/Engine/RenderBackend/MaterialBuilder.cpp (2)
createBuildinMaterial(289-366)createBuildinMaterial(289-290)
src/Engine/RenderBackend/DbgRenderer.cpp (1)
src/Engine/RenderBackend/MaterialBuilder.cpp (2)
createBuildinMaterial(289-366)createBuildinMaterial(289-290)
src/Engine/Platform/win32/Win32Timer.cpp (1)
src/Engine/Platform/sdl2/SDL2Timer.cpp (2)
getMilliCurrentSeconds(34-38)getMilliCurrentSeconds(34-34)
src/Engine/Platform/AbstractTimer.h (2)
src/Engine/Platform/sdl2/SDL2Timer.cpp (2)
getMilliCurrentSeconds(34-38)getMilliCurrentSeconds(34-34)src/Engine/Platform/win32/Win32Timer.cpp (2)
getMilliCurrentSeconds(33-40)getMilliCurrentSeconds(33-33)
src/Engine/Platform/sdl2/SDL2Timer.cpp (1)
src/Engine/Platform/win32/Win32Timer.cpp (2)
getMilliCurrentSeconds(33-40)getMilliCurrentSeconds(33-33)
src/Engine/Animation/AnimatorComponent.cpp (1)
src/Engine/RenderBackend/RenderBackendService.h (1)
RenderBackend(49-319)
src/Engine/RenderBackend/FontService.h (1)
src/Engine/RenderBackend/MaterialBuilder.cpp (2)
createBuildinMaterial(289-366)createBuildinMaterial(289-290)
src/Engine/RenderBackend/MaterialBuilder.h (1)
src/Engine/RenderBackend/MaterialBuilder.cpp (4)
createBuildinMaterial(289-366)createBuildinMaterial(289-290)create2DMaterial(55-105)create2DMaterial(55-55)
src/Engine/App/ParticleEmitter.cpp (1)
src/Engine/RenderBackend/MaterialBuilder.cpp (2)
createBuildinMaterial(289-366)createBuildinMaterial(289-290)
samples/04_terrain/TerrainRendering.cpp (1)
src/Engine/RenderBackend/MaterialBuilder.cpp (2)
createBuildinMaterial(289-366)createBuildinMaterial(289-290)
test/RenderTests/src/RenderBufferAccessTest.cpp (1)
src/Engine/RenderBackend/MaterialBuilder.cpp (2)
createBuildinMaterial(289-366)createBuildinMaterial(289-290)
src/Engine/App/AssimpWrapper.cpp (1)
src/Engine/RenderBackend/MaterialBuilder.cpp (2)
createBuildinMaterial(289-366)createBuildinMaterial(289-290)
src/Engine/Platform/sdl2/SDL2Timer.h (2)
src/Engine/Platform/sdl2/SDL2Timer.cpp (2)
getMilliCurrentSeconds(34-38)getMilliCurrentSeconds(34-34)src/Engine/Platform/win32/Win32Timer.cpp (2)
getMilliCurrentSeconds(33-40)getMilliCurrentSeconds(33-33)
src/Engine/RenderBackend/MeshBuilder.cpp (1)
src/Engine/RenderBackend/MaterialBuilder.cpp (2)
createBuildinMaterial(289-366)createBuildinMaterial(289-290)
src/Editor/src/RenderView/MainRenderView.cpp (1)
src/Engine/RenderBackend/MaterialBuilder.cpp (2)
createBuildinMaterial(289-366)createBuildinMaterial(289-290)
test/RenderTests/src/Textures/BaseTextureRenderTest.cpp (1)
src/Engine/RenderBackend/MaterialBuilder.cpp (2)
createBuildinMaterial(289-366)createBuildinMaterial(289-290)
src/Engine/RenderBackend/RenderBackendService.h (1)
src/Engine/RenderBackend/RenderBackendService.cpp (2)
enumerateGPU(306-317)enumerateGPU(306-306)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Analyze (cpp)
- GitHub Check: build
🔇 Additional comments (31)
src/Engine/RenderBackend/2D/RenderPass2D.h (2)
46-48: Minor formatting improvement.The blank line addition at line 47 improves visual spacing between the method declaration and the closing brace. No functional changes.
1-54: Verify alignment with PR objectives.This file contains only a formatting change (blank line addition) unrelated to the stated PR objective of "bugfix/check time diffs" and timer interface modernization. Please confirm that this change is intentional or part of a broader codebase-wide formatting pass, rather than an accidental inclusion in this PR.
contrib/vcpkg (1)
1-1: Verify submodule update compatibility with broader refactoring.This PR contains only a submodule pointer update; the actual implementation changes (timer refactoring, MaterialBuilder API consolidation) are in other files. Confirm that the new vcpkg commit (
41c447cc210dc39aa85d4a5f58b4a1b9e573b3dc) is compatible with all the timer and material system changes in this PR and introduces no breaking dependency changes.contrib/cppcore (1)
1-1: Clarify: This is a NEW submodule addition, not a pointer update.This PR adds
contrib/cppcoreas a new submodule pointing tohttps://github.com/kimkulling/cppcore.gitat commitf01433e31e70fb55ded1698fb710177d7189b97b. There is no prior SHA to compare against in this repo's history. Please confirm CI/build/tests pass with the new submodule and document its purpose/integration point in the PR description.Likely an incorrect or invalid review comment.
src/Editor/src/RenderView/MainRenderView.cpp (2)
97-98: LGTM!The material creation correctly adapts to the new API signature. The empty
TextureResourceArrayis appropriate since the axis geometry usesColorVertexand doesn't require textures.
151-152: LGTM!The material creation correctly adapts to the new API signature. The empty
TextureResourceArrayis appropriate since the grid geometry usesColorVertexand doesn't require textures.src/Engine/RenderBackend/MaterialBuilder.h (2)
50-54: LGTM!The API consolidation improves the material builder interface by unifying material creation into a single, more flexible method that accepts material names and texture resources. Documentation clearly describes the new parameters.
60-62: LGTM!The dedicated
createTextMaterialmethod appropriately separates text rendering concerns from the general material creation flow.src/Engine/Profiling/FPSCounter.cpp (1)
49-49: LGTM with dependency on timer correctness.The change from
microSecondstomilliSecondscorrectly aligns with the project-wide timing refactoring. The FPS calculation logic remains sound: accumulate time in milliseconds until 1000ms (1 second) elapses.Note: This depends on timer implementations correctly returning milliseconds. Please ensure the critical bug in
SDL2Timer.cpp(Line 36) is fixed, as it currently multiplies milliseconds by 1000, which would cause incorrect FPS calculations on SDL2 platforms.test/RenderTests/src/Textures/BaseTextureRenderTest.cpp (1)
89-94: LGTM!The material creation correctly migrates to the new
createBuildinMaterialAPI. The texture resource array is properly initialized with the spider texture, and the material name appropriately identifies the resource.test/RenderTests/src/RenderBufferAccessTest.cpp (1)
101-102: LGTM!The material creation correctly follows the updated MaterialBuilder API. The empty
TextureResourceArrayis valid, as the builder handles zero-texture materials appropriately.test/RenderTests/src/PrimTypeRenderTest.cpp (1)
109-110: LGTM!The material creation correctly uses the updated MaterialBuilder API with the material name and texture resource array parameters.
src/Engine/Platform/win32/Win32Timer.h (1)
46-46: LGTM!The method rename to
getMilliCurrentSeconds()correctly aligns with the implementation inWin32Timer.cpp, which properly returns milliseconds.src/Engine/Platform/AbstractTimer.h (1)
51-51: LGTM!The method rename from
getMicroCurrentSeconds()togetMilliCurrentSeconds()correctly reflects the unit change from microseconds to milliseconds.src/Engine/App/ParticleEmitter.cpp (1)
89-91: LGTM!The material creation correctly uses the updated
createBuildinMaterialAPI with an empty texture array (appropriate for color-only vertices) and the material name"default.mat".src/Engine/App/AssimpWrapper.cpp (1)
472-476: LGTM!The material creation call correctly migrates from
createTexturedMaterialtocreateBuildinMaterial, maintaining the same arguments and preserving the null-check error handling.src/Engine/Animation/AnimatorComponent.cpp (1)
219-219: LGTM!The addition of
setMatrixcall correctly integrates the animator with the render backend to apply the active track's model matrix during rendering.src/Engine/RenderBackend/RenderBackendService.cpp (1)
154-155: The original concern about an uninitialized pointer is incorrect.mGPUFeatureSetis initialized tonullptrin the header file (line 311), making it safe to delete inonClose()even ifenumerateGPU()was never called.However, there is a memory leak: if
enumerateGPU()is called multiple times, line 312 allocates a newGPUFeatureSetwithout deleting the old one first.Likely an incorrect or invalid review comment.
src/Engine/RenderBackend/DbgRenderer.cpp (1)
183-184: LGTM!The material creation correctly adapts to the new
createBuildinMaterialAPI signature. Using an emptyTextureResourceArrayis appropriate for the debug AABB rendering which usesColorVertextype without textures.samples/05_Animation/Animation.cpp (2)
47-53: LGTM!Good use of in-class member initializers for
mEntity,mKeyboardTransCtrl, andmAngle. This ensures consistent initialization and avoids potential undefined behavior.
87-92: Verify animation key timing consistency.The track duration was changed from
1.0fto1000.0fto align with the millisecond-based timing refactor. However,rot.Timeremains at1.0f. If track duration is in milliseconds, should the key time also be updated for consistency?src/Engine/RenderBackend/MeshBuilder.cpp (3)
132-133: LGTM!Material creation correctly uses the new
createBuildinMaterialAPI with an empty texture array for the triangle primitive.
247-248: LGTM!Consistent with the material creation pattern used in other mesh builder methods.
287-288: LGTM!Consistent material creation for point primitives.
samples/04_terrain/TerrainRendering.cpp (1)
137-138: LGTM!Material creation correctly adapts to the new API. The empty texture array is acceptable for a basic terrain sample, though future enhancements could add terrain textures.
src/Engine/RenderBackend/MaterialBuilder.cpp (3)
289-303: LGTM!The new
createBuildinMaterialsignature with material name and texture resource array provides better flexibility. The early validation and cache lookup are well implemented.
305-319: LGTM!Texture loading loop properly handles null entries and logs errors for failed texture loads while gracefully continuing with nullptr texture stages.
341-351: Verify shader resource ownership on partial failure.If
mat->createShader()succeeds butmat->getShader()returns nullptr, theShaderResourceat line 345 would never be created. However, if shader creation partially fails after the resource is created, verify that the resource ownership is properly managed.src/Engine/RenderBackend/RenderBackendService.h (1)
311-311: LGTM!Proper in-class initialization of
mGPUFeatureSetto nullptr.src/Engine/Common/osre_common.h (2)
203-208: LGTM!Constructor and setter correctly updated to work with milliseconds.
226-241: LGTM!Time conversion methods are correctly updated for millisecond-based storage:
asSeconds(): divides by 1000 (ms → s)asMilliSeconds(): returns stored value directlyasMicroSeconds(): multiplies by 1000 (ms → µs)
| f32 Time{ 1.0f }; | ||
| glm::vec3 Value{ 1 }; |
There was a problem hiding this comment.
Inconsistent default Time initialization across animation key structs.
VectorKey initializes Time to 1.0f, while RotationKey (Line 95) and ScalingKey (Line 108) both initialize Time to 0.0. This inconsistency could lead to unexpected animation behavior when keys are default-constructed.
🔧 Proposed fix to align initialization
struct VectorKey {
- f32 Time{ 1.0f };
- glm::vec3 Value{ 1 };
+ f32 Time{ 0.0f };
+ glm::vec3 Value{ };
};This aligns VectorKey with the other key structs and uses default initialization for Value (which zero-initializes the vector).
📝 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.
| f32 Time{ 1.0f }; | |
| glm::vec3 Value{ 1 }; | |
| struct VectorKey { | |
| f32 Time{ 0.0f }; | |
| glm::vec3 Value{ }; | |
| }; |
🤖 Prompt for AI Agents
In @src/Engine/Animation/AnimatorBase.h around lines 85 - 86, VectorKey
currently defaults Time to 1.0f while RotationKey and ScalingKey default to 0.0,
causing inconsistent behavior; change VectorKey's Time default to 0.0f (and use
default-initialized Value) so all key structs (VectorKey, RotationKey,
ScalingKey) share the same Time initialization semantics — update the Time
member in the VectorKey definition to 0.0f to match the others.
| /// @brief A time stamp. | ||
| struct Time { | ||
| i64 microSeconds = 0l; ///< The store time stamp in microseconds | ||
| i64 milliSeconds = 0l; ///< The store time stamp in microseconds |
There was a problem hiding this comment.
Fix stale comment - says "microseconds" but stores milliseconds.
The comment on line 197 still references "microseconds" but the member now stores milliseconds.
📝 Suggested fix
- i64 milliSeconds = 0l; ///< The store time stamp in microseconds
+ i64 milliSeconds = 0l; ///< The stored time stamp in milliseconds📝 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.
| i64 milliSeconds = 0l; ///< The store time stamp in microseconds | |
| i64 milliSeconds = 0l; ///< The stored time stamp in milliseconds |
🤖 Prompt for AI Agents
In @src/Engine/Common/osre_common.h at line 197, The comment for the member i64
milliSeconds incorrectly says "microseconds"; update the comment to reflect that
this member stores milliseconds (e.g., "The stored time stamp in milliseconds")
so the documentation matches the variable name and units.
| /// @brief Will return the current milliseconds. | ||
| /// @return The current ,illiseconds. | ||
| i64 getMicroCurrentSeconds() override; | ||
| i64 getMilliCurrentSeconds() override; |
There was a problem hiding this comment.
Critical: Implementation returns microseconds but method name indicates milliseconds.
The implementation in SDL2Timer.cpp multiplies SDL_GetTicks64() (which returns milliseconds) by 1000, converting it to microseconds. However, the method is named getMilliCurrentSeconds(), creating a critical unit mismatch.
🐛 Proposed fix to correct the implementation
Fix the implementation in src/Engine/Platform/sdl2/SDL2Timer.cpp:
i64 SDL2Timer::getMilliCurrentSeconds() {
- // There is no micro seconds tick from SDL2
- i64 ticks = static_cast<i64>(SDL_GetTicks64() * 1000l);
+ // SDL_GetTicks64 returns milliseconds since SDL library initialization
+ i64 ticks = static_cast<i64>(SDL_GetTicks64());
return ticks;
}🤖 Prompt for AI Agents
In @src/Engine/Platform/sdl2/SDL2Timer.h at line 45, The method
getMilliCurrentSeconds() in SDL2Timer (SDL2Timer.cpp) currently returns
microseconds by multiplying SDL_GetTicks64() by 1000; change the implementation
to return milliseconds instead by using SDL_GetTicks64() directly (or dividing
the current microsecond value by 1000 if you prefer), ensuring the return type
i64 still represents milliseconds and update any comment if present; keep the
function signature getMilliCurrentSeconds() and avoid changing units to prevent
silent unit mismatches.
| const i64 ms = static_cast<i64>( ( currentTime.QuadPart - mGlobeTime.QuadPart ) ) | ||
| / static_cast<i64>(mGlobeFrequency.QuadPart/1000 ); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat src/Engine/Platform/sdl2/SDL2Timer.cppRepository: kimkulling/osre
Length of output: 1764
🏁 Script executed:
cat src/Engine/Platform/win32/Win32Timer.cppRepository: kimkulling/osre
Length of output: 1969
Address precision loss in millisecond calculation.
The division mGlobeFrequency.QuadPart/1000 is performed as integer division before being used as the divisor. When the frequency is not evenly divisible by 1000, precision is lost. Rearrange the calculation to preserve accuracy:
const i64 ms = static_cast<i64>((currentTime.QuadPart - mGlobeTime.QuadPart) * 1000)
/ static_cast<i64>(mGlobeFrequency.QuadPart);Additionally, note that SDL2Timer::getMilliCurrentSeconds() contains a bug: it multiplies SDL_GetTicks64() (which returns milliseconds) by 1000, resulting in microseconds instead of milliseconds. This should be fixed separately.
| /// @brief Will return the current milliseconds. | ||
| /// @return The current ,illiseconds. |
There was a problem hiding this comment.
Fix typo in comment.
Line 45 has a typo: ",illiseconds" should be "milliseconds".
📝 Proposed fix
/// @brief Will return the current milliseconds.
- /// @return The current ,illiseconds.
+ /// @return The current milliseconds.
i64 getMilliCurrentSeconds() override;📝 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.
| /// @brief Will return the current milliseconds. | |
| /// @return The current ,illiseconds. | |
| /// @brief Will return the current milliseconds. | |
| /// @return The current milliseconds. | |
| i64 getMilliCurrentSeconds() override; |
🤖 Prompt for AI Agents
In @src/Engine/Platform/win32/Win32Timer.h around lines 44 - 45, Typo in the
documentation comment for the getter of current milliseconds in Win32Timer.h:
change the misspelled ",illiseconds" to "milliseconds" in the comment (the
triple-slash docblock that reads "Will return the current milliseconds." /
"@return The current ,illiseconds.") so the @return line correctly reads "The
current milliseconds."
| GPUFeatureSet *RenderBackendService::enumerateGPU() { | ||
| if (mRenderTaskPtr == nullptr) { | ||
| osre_error(Tag, "Render task not available."); | ||
| return nullptr; | ||
| } | ||
|
|
||
| mGPUFeatureSet = new GPUFeatureSet; | ||
| mGPUFeatureSet->shaderLanguageType = ShaderLanguageType::GLSL; // Default | ||
| mGPUFeatureSet->shaderVersion = "450"; // Default | ||
|
|
||
| return mGPUFeatureSet; | ||
| } |
There was a problem hiding this comment.
Memory leak if enumerateGPU() is called multiple times.
Each call to enumerateGPU() allocates a new GPUFeatureSet without checking if mGPUFeatureSet already holds a previous allocation. This causes a memory leak on repeated calls.
🐛 Proposed fix
GPUFeatureSet *RenderBackendService::enumerateGPU() {
if (mRenderTaskPtr == nullptr) {
osre_error(Tag, "Render task not available.");
return nullptr;
}
-
- mGPUFeatureSet = new GPUFeatureSet;
+
+ if (mGPUFeatureSet == nullptr) {
+ mGPUFeatureSet = new GPUFeatureSet;
+ }
mGPUFeatureSet->shaderLanguageType = ShaderLanguageType::GLSL; // Default
mGPUFeatureSet->shaderVersion = "450"; // Default
return mGPUFeatureSet;
}📝 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.
| GPUFeatureSet *RenderBackendService::enumerateGPU() { | |
| if (mRenderTaskPtr == nullptr) { | |
| osre_error(Tag, "Render task not available."); | |
| return nullptr; | |
| } | |
| mGPUFeatureSet = new GPUFeatureSet; | |
| mGPUFeatureSet->shaderLanguageType = ShaderLanguageType::GLSL; // Default | |
| mGPUFeatureSet->shaderVersion = "450"; // Default | |
| return mGPUFeatureSet; | |
| } | |
| GPUFeatureSet *RenderBackendService::enumerateGPU() { | |
| if (mRenderTaskPtr == nullptr) { | |
| osre_error(Tag, "Render task not available."); | |
| return nullptr; | |
| } | |
| if (mGPUFeatureSet == nullptr) { | |
| mGPUFeatureSet = new GPUFeatureSet; | |
| } | |
| mGPUFeatureSet->shaderLanguageType = ShaderLanguageType::GLSL; // Default | |
| mGPUFeatureSet->shaderVersion = "450"; // Default | |
| return mGPUFeatureSet; | |
| } |
🤖 Prompt for AI Agents
In @src/Engine/RenderBackend/RenderBackendService.cpp around lines 306 - 317,
The enumerateGPU() method allocates a new GPUFeatureSet every call causing a
leak; modify RenderBackendService::enumerateGPU to check mGPUFeatureSet and
either return the existing object or delete it before allocating a new one (or
better, replace raw pointer with a smart pointer member like
std::unique_ptr<GPUFeatureSet> and reset it) so repeated calls do not leak;
ensure mGPUFeatureSet is initialized to nullptr in the constructor and properly
cleared in the destructor or when replaced.
| /// @param eventData [in] The event data. | ||
| void sendEvent(const Common::Event *ev, const Common::EventData *eventData); | ||
|
|
||
| GPUFeatureSet *enumerateGPU(); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if enumerateGPU implementation queries actual GPU info
rg -n "enumerateGPU|GL_SHADING_LANGUAGE_VERSION|glGetString" --type cpp src/Engine/RenderBackend/Repository: kimkulling/osre
Length of output: 1355
🏁 Script executed:
#!/bin/bash
# View the implementation of enumerateGPU
sed -n '300,330p' src/Engine/RenderBackend/RenderBackendService.cppRepository: kimkulling/osre
Length of output: 1070
🏁 Script executed:
#!/bin/bash
# Check OGLRenderBackend::enumerateGPUCaps implementation
sed -n '71,100p' src/Engine/RenderBackend/OGLRenderer/OGLRenderBackend.cppRepository: kimkulling/osre
Length of output: 1433
🏁 Script executed:
#!/bin/bash
# Check how mRenderTaskPtr is used and if it calls enumerateGPUCaps
rg -n "mRenderTaskPtr|mOglCapabilities|GPUFeatureSet" src/Engine/RenderBackend/RenderBackendService.cpp | head -30Repository: kimkulling/osre
Length of output: 1589
🏁 Script executed:
#!/bin/bash
# Find GPUFeatureSet definition
rg -n "struct GPUFeatureSet|class GPUFeatureSet" --type cpp --type hpp -A 15Repository: kimkulling/osre
Length of output: 86
🏁 Script executed:
#!/bin/bash
# Find GPUFeatureSet definition
rg -n "struct GPUFeatureSet|class GPUFeatureSet" --type cpp -A 15Repository: kimkulling/osre
Length of output: 1383
Return actual GPU capabilities from enumerateGPU() instead of hardcoded defaults.
The enumerateGPU() method returns hardcoded values (ShaderLanguageType::GLSL and shaderVersion = "450"), while the actual GPU capabilities are queried in OGLRenderBackend::enumerateGPUCaps() using glGetString(GL_SHADING_LANGUAGE_VERSION). Connect the two so that callers receive the actual GPU's GLSL version and capabilities, not placeholder values.
🤖 Prompt for AI Agents
In @src/Engine/RenderBackend/RenderBackendService.h at line 206, enumerateGPU()
currently returns hardcoded defaults; change it to return the real GPUFeatureSet
produced by the OpenGL backend by invoking or reusing
OGLRenderBackend::enumerateGPUCaps() (or its shared logic) so the returned
GPUFeatureSet contains the real ShaderLanguageType and shaderVersion string from
glGetString(GL_SHADING_LANGUAGE_VERSION) instead of fixed
ShaderLanguageType::GLSL and "450". Update enumerateGPU() to obtain the
capabilities from the OpenGL query path (or centralize capability parsing into a
helper used by both) and return that GPUFeatureSet to callers.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| return nullptr; | ||
| } | ||
|
|
||
| mGPUFeatureSet = new GPUFeatureSet; |
Check warning
Code scanning / CodeQL
Resource not released in destructor Warning
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
samples/03_Instancing/Instancing.cpp (1)
104-117: Remove unused code or implement the intended rotation logic.The
onUpdatemethod has several issues:
- Line 105: The rotation matrix
rotis created but never used- Lines 109-113: The render pass and batch are empty—no actual rendering commands are issued
- The comment "Rotate the model" (line 104) doesn't match the implementation
For an instancing sample, this significantly undermines the demonstration's value.
Do you want me to suggest a complete implementation that applies the rotation matrix and properly demonstrates instancing?
🤖 Fix all issues with AI agents
In @samples/05_Animation/Animation.cpp:
- Around line 86-91: The timing is inconsistent: mTrack.duration is in
milliseconds (1000.0f) but rot.Time is set to 1.0f, which moves the rotation key
to 1ms instead of the track end; update the RotationKey time to match the track
duration (e.g., set rot.Time to mTrack.duration or convert both to the same
unit) before calling mTrack.animationChannels[0].RotationKeys.add(rot) so the
key remains at the end of the track.
In @src/Engine/Animation/AnimatorComponent.cpp:
- Line 99: The stray std::cout debug line printing Time/last
time/delta/ticksPerSecond should be removed or replaced with the project's
logging macro; locate the statement that prints "Time: " (references variables
time, mLastTime, ticksPerSecond) in AnimatorComponent.cpp (around where
mLastTime is used) and either delete it or change it to the appropriate logging
call (e.g., osre_debug or the module's logging macro declared via
DECL_OSRE_LOG_MODULE) so the message follows the project's logging conventions.
- Line 110: Remove the stray std::cout debug print in AnimatorComponent (the
line printing currentAnimationTrack) and either delete it or replace it with the
project logging API used elsewhere (e.g., the component/class-level logger or
Log::info/ENGINE_LOG) so the animation track is recorded via the unified logger;
update the call to include clear context like "Current animation track" and the
currentAnimationTrack variable and ensure any necessary headers are included for
the logging API.
- Around line 207-211: The transform accumulation bug is caused by using a
reference to mTransformArray[currentAnimationTrack] and applying transform *=
toMat4(q) so rotations and scales accumulate across frames; instead, rebuild the
transform from identity each frame: create a fresh glm::mat4 identity, then
apply rotation via toMat4(q), then scale(presenceScale), then
translate(presentPosition), and finally assign that result back to
mTransformArray[currentAnimationTrack] (do not use *= or mutate the previous
matrix).
In @src/Engine/RenderBackend/MeshBuilder.cpp:
- Around line 225-259: Remove the large commented-out vertex/color/uv block in
MeshBuilder.cpp (the multi-line /* ... */ block containing the 24 commented
vertices) to clean up dead code; simply delete the entire commented block so the
file contains only active code and rebuild to ensure no references depend on it.
🧹 Nitpick comments (2)
samples/05_Animation/Animation.cpp (1)
47-53: Good: member default-initialization; consider ownership clarity for raw pointers
mEntity/mKeyboardTransCtrlbeing raw pointers is fine if they’re non-owning, but it’s worth making that explicit (comment orobserver_ptr-style) to avoid future double-free/ownership confusion.src/Engine/RenderBackend/MeshBuilder.cpp (1)
262-289: Consider simplifying repetitive color initialization.All 24 color values are identical. This could be simplified with a loop for better maintainability.
♻️ Optional simplification
glm::vec3 col[Numvertices] = {}; - col[0] = glm::vec3(0.5, 0.5, 0.5); - col[1] = glm::vec3(0.5, 0.5, 0.5); - col[2] = glm::vec3(0.5, 0.5, 0.5); - col[3] = glm::vec3(0.5, 0.5, 0.5); - col[4] = glm::vec3(0.5, 0.5, 0.5); - col[5] = glm::vec3(0.5, 0.5, 0.5); - col[6] = glm::vec3(0.5, 0.5, 0.5); - col[7] = glm::vec3(0.5, 0.5, 0.5); - - col[8] = glm::vec3(0.5, 0.5, 0.5); - col[9] = glm::vec3(0.5, 0.5, 0.5); - col[10] = glm::vec3(0.5, 0.5, 0.5); - col[11] = glm::vec3(0.5, 0.5, 0.5); - col[12] = glm::vec3(0.5, 0.5, 0.5); - col[13] = glm::vec3(0.5, 0.5, 0.5); - col[14] = glm::vec3(0.5, 0.5, 0.5); - col[15] = glm::vec3(0.5, 0.5, 0.5); - - col[16] = glm::vec3(0.5, 0.5, 0.5); - col[17] = glm::vec3(0.5, 0.5, 0.5); - col[18] = glm::vec3(0.5, 0.5, 0.5); - col[19] = glm::vec3(0.5, 0.5, 0.5); - col[20] = glm::vec3(0.5, 0.5, 0.5); - col[21] = glm::vec3(0.5, 0.5, 0.5); - col[22] = glm::vec3(0.5, 0.5, 0.5); - col[23] = glm::vec3(0.5, 0.5, 0.5); + for (size_t i = 0; i < Numvertices; ++i) { + col[i] = glm::vec3(0.5, 0.5, 0.5); + }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
samples/00_HelloWorld/HelloWorld.cppsamples/02_Demo2D/Demo2D.cppsamples/03_Instancing/Instancing.cppsamples/05_Animation/Animation.cppsrc/Engine/Animation/AnimatorComponent.cppsrc/Engine/RenderBackend/MeshBuilder.cppsrc/Engine/RenderBackend/MeshBuilder.htest/RenderTests/src/Textures/Texture3DRenderTest.cpp
🧰 Additional context used
🧬 Code graph analysis (3)
src/Engine/RenderBackend/MeshBuilder.h (1)
src/Engine/RenderBackend/Mesh.h (1)
RenderBackend(31-178)
src/Engine/Animation/AnimatorComponent.cpp (8)
src/Engine/Animation/AnimatorBase.h (2)
App(36-38)RenderBackend(32-34)src/Engine/RenderBackend/MeshBuilder.h (1)
RenderBackend(29-133)src/Engine/RenderBackend/FontService.h (1)
RenderBackend(31-64)src/Engine/RenderBackend/RenderBackendService.h (1)
RenderBackend(49-319)src/Engine/App/TransformComponent.h (1)
RenderBackend(45-47)src/Engine/App/AppBase.h (1)
RenderBackend(49-54)src/Engine/App/AppBase.cpp (2)
onRender(419-423)onRender(419-419)src/Engine/App/Component.cpp (2)
onRender(86-95)onRender(86-86)
src/Engine/RenderBackend/MeshBuilder.cpp (1)
src/Engine/RenderBackend/MaterialBuilder.cpp (2)
createBuildinMaterial(289-366)createBuildinMaterial(289-290)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build
- GitHub Check: Analyze (cpp)
🔇 Additional comments (13)
samples/05_Animation/Animation.cpp (1)
75-79: The mesh lifetime concern is incorrect; no dangling pointer here.The code is safe:
meshBuilder(line 74) is a named local variable, not a temporary. WhencreateCube()returns a reference to itself andgetMesh()returns the heap-allocatedmActiveMeshpointer, that pointer remains valid because theMeshobject is not freed when theMeshBuildergoes out of scope.However, a minor improvement: guard
rcbefore dereferencing, and use0.8ffor the float literal (stylistic). These are consistent with existing patterns in the codebase, so not critical.src/Engine/RenderBackend/MeshBuilder.cpp (5)
132-133: Material creation updated to new API pattern.The material creation now correctly uses the new
createBuildinMaterialsignature with an empty texture array and explicit material name.
182-183: Material creation consistent with new API.Same pattern as in
createTriangle. The refactor is applied consistently.
188-224: Cube creation refactored to proper 6-face mesh.The signature change removes explicit height and depth parameters, using a single width parameter to create a uniform cube. The implementation correctly expands from 8 to 24 vertices to support proper per-face attributes (normals, UVs). This is a breaking API change but appears intentional given the PR scope.
318-319: Material creation consistent across all mesh builders.The cube creation properly uses the refactored material API pattern.
358-359: Points allocation follows new material creation pattern.Consistent with the broader material API refactoring.
samples/00_HelloWorld/HelloWorld.cpp (1)
74-74: Cube creation updated to new API signature.The call correctly adapts to the simplified
createCubesignature that now takes only a single size parameter instead of width, height, and depth.samples/02_Demo2D/Demo2D.cpp (1)
121-121: Cube creation updated to match new API.Correctly updated to use the single-parameter size signature.
test/RenderTests/src/Textures/Texture3DRenderTest.cpp (1)
54-54: Test updated for new cube creation API.The test correctly uses the new single-size parameter signature.
src/Engine/RenderBackend/MeshBuilder.h (1)
69-69: Public API simplified for cube creation.The signature change from
createCube(type, w, h, d, access)tocreateCube(type, w, access)is a breaking change but aligns with the implementation that now creates uniform cubes. All call sites have been updated to use the new 3-parameter signature—no remaining old-style calls exist in the codebase.samples/03_Instancing/Instancing.cpp (1)
82-82: API change correctly applied. ThecreateCube()call at line 82 properly uses the updated signaturecreateCube(VertexType, width, BufferAccessType). This change is consistent across all samples in the codebase (HelloWorld, Demo2D, Animation, GeoInstanceRenderTest), confirming it was a coordinated refactor to use uniform cube dimensions instead of individual width/height/depth parameters. The code is syntactically correct and no action is required.src/Engine/Animation/AnimatorComponent.cpp (2)
23-32: Include and namespace additions are appropriate for the new functionality.The
RenderBackendService.hinclude andOSRE::RenderBackendnamespace are correctly added to support the newsetMatrixcall inonRender. Theiostreaminclude is only needed for debug output (addressed below).
216-222: Render integration looks correct.The
setMatrixcall properly sets the model matrix for the active animation track, which aligns with theRenderBackendServiceAPI shown in the relevant snippets.
| glm::mat4 &transform = mTransformArray[currentAnimationTrack]; | ||
| transform = toMat4(q); | ||
| transform *= toMat4(q); | ||
| transform = scale(transform, presenceScale); | ||
| transform = translate(transform, presentPosition); | ||
| mTransformArray[currentAnimationTrack] = transform; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's find and read the AnimatorComponent.cpp file
fd -t f "AnimatorComponent.cpp" --exec wc -l {} \;Repository: kimkulling/osre
Length of output: 107
🏁 Script executed:
# Read the file around lines 207-211 with broader context
fd -t f "AnimatorComponent.cpp" --exec sed -n '170,230p' {} \;Repository: kimkulling/osre
Length of output: 2235
🏁 Script executed:
# Get the function name and signature containing these lines
fd -t f "AnimatorComponent.cpp" --exec sed -n '1,220p' {} \; | tail -80Repository: kimkulling/osre
Length of output: 3251
🏁 Script executed:
# Search for mTransformArray initialization and reset logic
fd -t f "AnimatorComponent.cpp" --exec cat {} \; | grep -n "mTransformArray" | head -20Repository: kimkulling/osre
Length of output: 405
🏁 Script executed:
# Look at the broader context of this animation function
fd -t f "AnimatorComponent.cpp" --exec sed -n '150,260p' {} \;Repository: kimkulling/osre
Length of output: 3224
🏁 Script executed:
# Find function name/signature for the code at lines 207-211
fd -t f "AnimatorComponent.cpp" --exec sed -n '100,211p' {} \; | grep -B 50 "glm::mat4 &transform = mTransformArray\[currentAnimationTrack\]" | head -60Repository: kimkulling/osre
Length of output: 2176
🏁 Script executed:
# Get full function containing lines 207-211
fd -t f "AnimatorComponent.cpp" --exec sed -n '100,250p' {} \;Repository: kimkulling/osre
Length of output: 5272
Transform accumulation bug — matrix must be reset to identity each frame.
The transform matrix is obtained as a reference to its previous value and then multiplied (*=) instead of being freshly computed. Since the function interpolates keyframe values per-frame (position, rotation, scale), the transform should be rebuilt from identity each frame. Currently, rotations compound: Frame 1 applies rotation₁, Frame 2 applies rotation₁ × rotation₂, etc., causing exponential accumulation.
Fix: Reset the transform to identity before applying the interpolated keyframe values:
Proposed fix
- glm::mat4 &transform = mTransformArray[currentAnimationTrack];
- transform *= toMat4(q);
- transform = scale(transform, presenceScale);
- transform = translate(transform, presentPosition);
- mTransformArray[currentAnimationTrack] = transform;
+ glm::mat4 transform = glm::mat4(1.0f);
+ transform = translate(transform, presentPosition);
+ transform *= toMat4(q);
+ transform = scale(transform, presenceScale);
+ mTransformArray[currentAnimationTrack] = transform;🤖 Prompt for AI Agents
In @src/Engine/Animation/AnimatorComponent.cpp around lines 207 - 211, The
transform accumulation bug is caused by using a reference to
mTransformArray[currentAnimationTrack] and applying transform *= toMat4(q) so
rotations and scales accumulate across frames; instead, rebuild the transform
from identity each frame: create a fresh glm::mat4 identity, then apply rotation
via toMat4(q), then scale(presenceScale), then translate(presentPosition), and
finally assign that result back to mTransformArray[currentAnimationTrack] (do
not use *= or mutate the previous matrix).
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/Engine/Animation/AnimatorComponent.cpp (1)
173-200: Scaling keyframe selection uses the wrong key array size (modulo byRotationKeys.size()).
This can select an invalidnextFrame(and ifRotationKeysis empty, modulo-by-zero is UB).Proposed fix
- size_t nextFrame = (animFrameIndex + 1) % animChannel.RotationKeys.size(); + size_t nextFrame = (animFrameIndex + 1) % animChannel.ScalingKeys.size();
🤖 Fix all issues with AI agents
In @src/Engine/Animation/AnimatorComponent.cpp:
- Around line 212-218: The onRender method can index mTransformArray with
mActiveTrack before any tracks are added, causing out-of-bounds UB; update
AnimatorComponent::onRender to first verify renderBackendSrv and that
mTransformArray is non-empty and mActiveTrack is within bounds (e.g., check
!mTransformArray.empty() and mActiveTrack >= 0 && mActiveTrack <
mTransformArray.size()) before calling
renderBackendSrv->setMatrix(MatrixType::Model, mTransformArray[mActiveTrack]);
if the guard fails, either skip calling setMatrix or use a safe fallback
(identity matrix) and return appropriately.
- Around line 203-208: The current code mutates mTransformArray via a reference
(glm::mat4 &transform = mTransformArray[currentAnimationTrack]) causing
transforms to accumulate each frame; instead construct a fresh transform from
identity using the absolute values and a consistent T * R * S order: replace the
reference with a local glm::mat4 transform = glm::mat4(1.0f), then apply
translate(transform, presentPosition), then multiply by rotation (toMat4(q)),
then apply scale(presenceScale), and finally assign that single matrix back to
mTransformArray[currentAnimationTrack].
- Around line 94-100: The time calculation treats dt.asMilliSeconds() as
seconds—fix by converting milliseconds to seconds before applying
ticksPerSecond: replace using dt.asMilliSeconds() directly with
dt.asMilliSeconds() / 1000.0 (or use dt.asSeconds() if available) when computing
the variable time in AnimatorComponent (the variable named time and the use of
track->ticksPerSecond); ensure the comment about "happens in ticks" remains
correct after the change.
🧹 Nitpick comments (1)
src/Engine/Animation/AnimatorComponent.cpp (1)
23-31: Avoidusing namespace OSRE::RenderBackend;here (prefer explicit qualifiers).
This is a.cppso it’s not as risky as a header, but it still increases ambiguity/collision risk and doesn’t buy much sinceonRenderstill uses qualified names.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/Engine/Animation/AnimatorComponent.cppsrc/Engine/RenderBackend/MeshBuilder.h
🧰 Additional context used
🧬 Code graph analysis (2)
src/Engine/Animation/AnimatorComponent.cpp (1)
src/Engine/RenderBackend/RenderBackendService.h (1)
RenderBackend(49-319)
src/Engine/RenderBackend/MeshBuilder.h (1)
src/Engine/RenderBackend/Mesh.h (1)
RenderBackend(31-178)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build
- GitHub Check: Analyze (cpp)
🔇 Additional comments (2)
src/Engine/RenderBackend/MeshBuilder.h (2)
73-75: Good documentation enhancement.The added inline parameter documentation for
accessandnumLinesimproves clarity and consistency with the rest of the API documentation.
64-69: Breaking API change successfully implemented: all callers have been updated.The
createCubesignature has been simplified from accepting separate width, height, and depth parameters to a single width parameter. All call sites in the codebase (samples and tests) have been updated to use the new 3-parameter signature. The implementation correctly creates uniform cubes using the single dimension parameter for all three axes.
| // Calculate the time | ||
| auto time = static_cast<d32>(dt.asMilliSeconds()); | ||
| const double ticksPerSecond = track->ticksPerSecond != 0.0 ? track->ticksPerSecond : 25.0; | ||
|
|
||
| // every following time calculation happens in ticks | ||
| // Every following time calculation happens in ticks | ||
| time *= ticksPerSecond; | ||
|
|
There was a problem hiding this comment.
Fix time-unit mismatch: milliseconds are being treated as seconds when applying ticksPerSecond.
With dt.asMilliSeconds() you need a / 1000.0 (or equivalent) before multiplying by ticksPerSecond (which is “ticks per second”).
Proposed fix
- // Calculate the time
- auto time = static_cast<d32>(dt.asMilliSeconds());
const double ticksPerSecond = track->ticksPerSecond != 0.0 ? track->ticksPerSecond : 25.0;
-
- // Every following time calculation happens in ticks
- time *= ticksPerSecond;
+ // Calculate the time (all following time calculation happens in ticks)
+ double time = (static_cast<double>(dt.asMilliSeconds()) / 1000.0) * ticksPerSecond;📝 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.
| // Calculate the time | |
| auto time = static_cast<d32>(dt.asMilliSeconds()); | |
| const double ticksPerSecond = track->ticksPerSecond != 0.0 ? track->ticksPerSecond : 25.0; | |
| // every following time calculation happens in ticks | |
| // Every following time calculation happens in ticks | |
| time *= ticksPerSecond; | |
| const double ticksPerSecond = track->ticksPerSecond != 0.0 ? track->ticksPerSecond : 25.0; | |
| // Calculate the time (all following time calculation happens in ticks) | |
| double time = (static_cast<double>(dt.asMilliSeconds()) / 1000.0) * ticksPerSecond; |
🤖 Prompt for AI Agents
In @src/Engine/Animation/AnimatorComponent.cpp around lines 94 - 100, The time
calculation treats dt.asMilliSeconds() as seconds—fix by converting milliseconds
to seconds before applying ticksPerSecond: replace using dt.asMilliSeconds()
directly with dt.asMilliSeconds() / 1000.0 (or use dt.asSeconds() if available)
when computing the variable time in AnimatorComponent (the variable named time
and the use of track->ticksPerSecond); ensure the comment about "happens in
ticks" remains correct after the change.
| glm::mat4 &transform = mTransformArray[currentAnimationTrack]; | ||
| transform = toMat4(q); | ||
| transform *= toMat4(q); | ||
| transform = scale(transform, presenceScale); | ||
| transform = translate(transform, presentPosition); | ||
| mTransformArray[currentAnimationTrack] = transform; | ||
|
|
There was a problem hiding this comment.
Transform now accumulates every frame (likely unintended) and the write-back is redundant.
transform is a reference to mTransformArray[currentAnimationTrack], so transform *= ...; scale(...); translate(...); mutates persistent state every frame. Since presentPosition/q/presenceScale are computed as absolute values from keys, you probably want to rebuild from identity each update (and explicitly pick a consistent order like T * R * S).
Proposed fix (rebuild from identity each update)
- glm::mat4 &transform = mTransformArray[currentAnimationTrack];
- transform *= toMat4(q);
- transform = scale(transform, presenceScale);
- transform = translate(transform, presentPosition);
- mTransformArray[currentAnimationTrack] = transform;
+ glm::mat4 transform(1.0f);
+ transform = translate(transform, presentPosition);
+ transform *= toMat4(q);
+ transform = scale(transform, presenceScale);
+ mTransformArray[currentAnimationTrack] = transform;📝 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.
| glm::mat4 &transform = mTransformArray[currentAnimationTrack]; | |
| transform = toMat4(q); | |
| transform *= toMat4(q); | |
| transform = scale(transform, presenceScale); | |
| transform = translate(transform, presentPosition); | |
| mTransformArray[currentAnimationTrack] = transform; | |
| glm::mat4 transform(1.0f); | |
| transform = translate(transform, presentPosition); | |
| transform *= toMat4(q); | |
| transform = scale(transform, presenceScale); | |
| mTransformArray[currentAnimationTrack] = transform; | |
🤖 Prompt for AI Agents
In @src/Engine/Animation/AnimatorComponent.cpp around lines 203 - 208, The
current code mutates mTransformArray via a reference (glm::mat4 &transform =
mTransformArray[currentAnimationTrack]) causing transforms to accumulate each
frame; instead construct a fresh transform from identity using the absolute
values and a consistent T * R * S order: replace the reference with a local
glm::mat4 transform = glm::mat4(1.0f), then apply translate(transform,
presentPosition), then multiply by rotation (toMat4(q)), then apply
scale(presenceScale), and finally assign that single matrix back to
mTransformArray[currentAnimationTrack].
| bool AnimatorComponent::onRender(RenderBackend::RenderBackendService *renderBackendSrv) { | ||
| osre_assert(renderBackendSrv != nullptr); | ||
|
|
||
| renderBackendSrv->setMatrix(MatrixType::Model, mTransformArray[mActiveTrack]); | ||
|
|
||
| return true; | ||
| } |
There was a problem hiding this comment.
Guard mTransformArray[mActiveTrack] against “no tracks added” to avoid out-of-bounds UB.
osre_assert(renderBackendSrv != nullptr); is good, but this new indexing can crash if onRender is called before any track is created/added.
Proposed fix
bool AnimatorComponent::onRender(RenderBackend::RenderBackendService *renderBackendSrv) {
osre_assert(renderBackendSrv != nullptr);
- renderBackendSrv->setMatrix(MatrixType::Model, mTransformArray[mActiveTrack]);
+ if (mActiveTrack >= mTransformArray.size()) {
+ return true;
+ }
+ renderBackendSrv->setMatrix(MatrixType::Model, mTransformArray[mActiveTrack]);
return true;
}🤖 Prompt for AI Agents
In @src/Engine/Animation/AnimatorComponent.cpp around lines 212 - 218, The
onRender method can index mTransformArray with mActiveTrack before any tracks
are added, causing out-of-bounds UB; update AnimatorComponent::onRender to first
verify renderBackendSrv and that mTransformArray is non-empty and mActiveTrack
is within bounds (e.g., check !mTransformArray.empty() and mActiveTrack >= 0 &&
mActiveTrack < mTransformArray.size()) before calling
renderBackendSrv->setMatrix(MatrixType::Model, mTransformArray[mActiveTrack]);
if the guard fails, either skip calling setMatrix or use a safe fallback
(identity matrix) and return appropriately.
|
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/Engine/Platform/AbstractTimer.h (1)
78-96: Fix critical logic bug: modifieddiffvalue is never used in the return value.There's a logic error in
getTimeDiff():
- Line 87 creates
Time dt(diff)with the calculated difference- Lines 89-93 conditionally modify
diffto usegetRequestedTimeStep()- Line 95 returns
dt, which still contains the originaldiffvalueThe modified
diffvalue has no effect becausedtis constructed before the modification and never updated. The time-slicing logic on lines 89-93 is dead code.Additionally, the threshold check
if (diff > 1000)on line 90 now represents 1000 milliseconds (1 second) instead of the previous 1000 microseconds (1 millisecond), due to the switch fromgetMicroCurrentSeconds()togetMilliCurrentSeconds(). This is a significant 1000x change in the threshold that affects when time slicing is applied.🐛 Proposed fix for the logic bug
inline Time AbstractTimer::getTimeDiff() { const i64 currentTime = getMilliCurrentSeconds(); if (mLastTime == 0L) { mLastTime = currentTime; return 0l; } i64 diff = currentTime - mLastTime; mLastTime = currentTime; - Time dt(diff); if (mReqTimeSlice != -1) { if (diff > 1000) { diff = AbstractTimer::getRequestedTimeStep(); } } + Time dt(diff); return dt; }
🤖 Fix all issues with AI agents
In @src/Engine/Platform/AbstractTimer.h:
- Line 50: The SDL2Timer::getMilliCurrentSeconds implementation multiplies
SDL_GetTicks64() by 1000, producing microseconds instead of the expected
milliseconds; update SDL2Timer::getMilliCurrentSeconds to return the
SDL_GetTicks64() value (cast to i64) directly without multiplying by 1000 so it
matches the method name and Win32Timer behavior.
🧹 Nitpick comments (1)
src/Engine/Platform/AbstractTimer.h (1)
43-43: Remove unused constantConversion2Micro.This constant is declared on line 43 but is not referenced anywhere else in the codebase. It appears to be dead code remaining from the microsecond→millisecond refactor and should be removed.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
samples/05_Animation/Animation.cppsrc/Engine/Platform/AbstractTimer.h
🚧 Files skipped from review as they are similar to previous changes (1)
- samples/05_Animation/Animation.cpp
🧰 Additional context used
🧬 Code graph analysis (1)
src/Engine/Platform/AbstractTimer.h (4)
src/Engine/Platform/PlatformCommon.h (1)
Platform(39-63)src/Engine/Platform/sdl2/SDL2Timer.cpp (2)
getMilliCurrentSeconds(34-38)getMilliCurrentSeconds(34-34)src/Engine/Platform/win32/Win32Timer.cpp (2)
getMilliCurrentSeconds(33-40)getMilliCurrentSeconds(33-33)src/Engine/Common/Object.cpp (1)
Object(28-30)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Analyze (cpp)
- GitHub Check: build
🔇 Additional comments (3)
src/Engine/Platform/AbstractTimer.h (3)
27-27: LGTM! Modern nested namespace syntax.The C++17 nested namespace syntax improves readability and is correctly applied.
Also applies to: 98-98
66-67: LGTM! Proper use of in-class member initialization.The field initialization has been modernized using in-class initializers, and the constructor has been appropriately simplified. This improves code clarity and maintainability.
Also applies to: 70-72
61-62: LGTM! Documentation updated correctly.The comment accurately reflects the change from microseconds to milliseconds.
| /// @brief Returns the milli-seconds since starting the application. | ||
| /// @return Seconds past since starting the application. | ||
| virtual i64 getMicroCurrentSeconds() = 0; | ||
| virtual i64 getMilliCurrentSeconds() = 0; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
echo "=== Locating timer implementation files ==="
fd -e h -e cpp | grep -i timer
echo -e "\n=== SDL2Timer implementation ==="
find . -name "*SDL2Timer*" -o -name "*Win32Timer*" | head -5Repository: kimkulling/osre
Length of output: 492
🏁 Script executed:
#!/bin/bash
echo "=== SDL2Timer.cpp ==="
cat -n src/Engine/Platform/sdl2/SDL2Timer.cpp
echo -e "\n=== Win32Timer.cpp ==="
cat -n src/Engine/Platform/win32/Win32Timer.cppRepository: kimkulling/osre
Length of output: 4309
🏁 Script executed:
#!/bin/bash
echo "=== Verify SDL_GetTicks64() behavior ==="
rg -B2 -A2 'SDL_GetTicks64' src/ || echo "No additional SDL_GetTicks64 references found"
echo -e "\n=== Check if getMilliCurrentSeconds is called elsewhere ==="
rg 'getMilliCurrentSeconds|getMicroCurrentSeconds' src/ --type=cpp --type=hRepository: kimkulling/osre
Length of output: 1024
Remove the × 1000 multiplication in SDL2Timer—it returns microseconds instead of milliseconds.
SDL_GetTicks64() returns milliseconds. The current implementation multiplies this by 1000, producing microseconds, which contradicts the method name getMilliCurrentSeconds(). Win32Timer correctly returns milliseconds (dividing by frequency/1000), establishing the expected behavior. Remove the multiplication on line 36 of src/Engine/Platform/sdl2/SDL2Timer.cpp:
SDL2Timer fix
i64 SDL2Timer::getMilliCurrentSeconds() {
// There is no micro seconds tick from SDL2
i64 ticks = static_cast<i64>(SDL_GetTicks64());
return ticks;
}🤖 Prompt for AI Agents
In @src/Engine/Platform/AbstractTimer.h at line 50, The
SDL2Timer::getMilliCurrentSeconds implementation multiplies SDL_GetTicks64() by
1000, producing microseconds instead of the expected milliseconds; update
SDL2Timer::getMilliCurrentSeconds to return the SDL_GetTicks64() value (cast to
i64) directly without multiplying by 1000 so it matches the method name and
Win32Timer behavior.
Summary by CodeRabbit
New Features
Bug Fixes & Improvements
API Changes
Chores
✏️ Tip: You can customize this high-level summary in your review settings.