Kimkulling/use own UI #227
Conversation
WalkthroughThe changes involve significant modifications across multiple files, focusing on restructuring the project’s build configuration, particularly regarding the ImGui library. Updates include the removal of SDL and OpenGL initialization in the application, alterations in resource management for textures, and a consistent renaming of member variables in the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant AssetManager
participant Renderer
User->>AssetManager: Select Asset
AssetManager->>Renderer: Load Asset
Renderer->>Renderer: Initialize Rendering
Renderer->>User: Display Asset
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
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: 0
Outside diff range, codebase verification and nitpick comments (2)
src/Editor_imgui/CMakeLists.txt (1)
Remaining ImGui References Detected
The recent changes in
src/Editor_imgui/CMakeLists.txthave not fully removed all ImGui-related references. The following references were found:
${CMAKE_HOME_DIRECTORY}/contrib/imguiinsrc/Editor_imgui/CMakeLists.txtADD_EXECUTABLE( osre_ed_imgui ... )insrc/Editor_imgui/CMakeLists.txtosre_ed_imguireferences in the rootCMakeLists.txtPlease ensure that these references are either removed or properly replaced to reflect the shift away from using ImGui. Verify that all dependencies and configurations are updated accordingly to prevent any potential issues with the application's build process or functionality.
Analysis chain
Line range hint
1-88: Verify the impact of removing ImGui dependencies.The changes in this file indicate a significant shift away from using ImGui for the graphical components of the project. This is evident from the removal of ImGui-related variables (
platform_backendandimgui_src), theSOURCE_GROUPdirective forcontrib\Imgui, and the${imgui_src}variable from theADD_EXECUTABLEcommand.Please ensure that:
- The necessary changes have been made in other parts of the codebase to accommodate this shift away from ImGui.
- The functionality and appearance of the application's user interface are not adversely affected by this change.
- Any new dependencies or libraries introduced as a replacement for ImGui are properly integrated and configured.
You can use the following script to search for any remaining ImGui references in the codebase:
If there are any remaining references to ImGui, please ensure that they are properly removed or replaced.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Search for any remaining ImGui references in the codebase. # Test: Search for ImGui references in C++ files. Expect: No occurrences. rg --type cpp -i 'imgui' # Test: Search for ImGui references in CMake files. Expect: No occurrences. rg --type cmake -i 'imgui'Length of output: 558
src/Editor_imgui/src/main.cpp (1)
39-39: Cross-platform compatibility likely impacted by Windows-specific codeThe inclusion of the Windows-specific header
Platform/win32/Win32Window.hinsrc/Editor_imgui/src/main.cppand other files suggests a reliance on Windows-specific functionality. The presence of conditional compilation directives and platform-specific code paths indicates that the project's cross-platform compatibility may be affected. Further investigation or testing on other platforms is recommended to assess the full impact.
src/Editor_imgui/src/main.cpp: IncludesPlatform/win32/Win32Window.hsrc/Editor_imgui/src/ProgressReporter.cpp: Includes and usesPlatform/win32/Win32Window.hsrc/Engine/Threading/TaskThread.h: Contains#ifdef WIN32and similar directivesAnalysis chain
Verify the impact of including the Windows-specific header on cross-platform compatibility.
The inclusion of the Windows-specific header file
Platform/win32/Win32Window.hsuggests a potential shift towards Windows-specific functionality. This change may impact the project's cross-platform compatibility.Please ensure that the inclusion of this header does not introduce any platform-specific dependencies or break the build on other supported platforms.
Run the following script to verify the project's cross-platform compatibility:
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the project's cross-platform compatibility. # Test: Search for platform-specific code. Expect: No platform-specific code outside of the designated platform directories. rg --type cpp -i -g '!Platform' -e 'win32|linux|macos'Length of output: 22408
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (9)
- contrib/cppcore (1 hunks)
- src/Editor_imgui/CMakeLists.txt (3 hunks)
- src/Editor_imgui/src/main.cpp (3 hunks)
- src/Engine/App/AssimpWrapper.cpp (2 hunks)
- src/Engine/App/AssimpWrapper.h (2 hunks)
- src/Engine/RenderBackend/OGLRenderer/OGLRenderBackend.cpp (1 hunks)
- src/Engine/RenderBackend/OGLRenderer/OGLRenderCommands.cpp (1 hunks)
- src/Engine/RenderBackend/RenderCommon.cpp (5 hunks)
- src/Engine/RenderBackend/RenderCommon.h (1 hunks)
Files skipped from review due to trivial changes (3)
- contrib/cppcore
- src/Engine/App/AssimpWrapper.h
- src/Engine/RenderBackend/OGLRenderer/OGLRenderCommands.cpp
Additional comments not posted (15)
src/Editor_imgui/src/main.cpp (3)
60-63: LGTM!The changes to streamline the asset loading process by directly invoking a file dialog and loading the selected asset are approved. This modification aligns with the goal of focusing more on asset handling and less on the graphical setup, as mentioned in the AI-generated summary.
74-76: Clarify the intended usage of the retrieved stage and world pointers.The code segment retrieves the active stage and world from the
osreAppobject, but the retrieved pointers are not utilized in the provided context. This may indicate incomplete or placeholder code.Please provide more information about the intended usage of the retrieved stage and world pointers. If this code segment is a work in progress, consider adding TODO comments to clarify the planned functionality.
82-84: LGTM!The added function calls to
handleEvents(),update(), andrequestNextFrame()ensure that the application can handle events, update its state, and request the next frame, which aligns with the modified main loop structure mentioned in the AI-generated summary. The changes are approved.src/Engine/RenderBackend/RenderCommon.cpp (7)
299-308: LGTM!The member variable renaming improves code readability and maintainability.
313-314: LGTM!The changes to the destructor and the addition of the
clear()method improve resource management by ensuring proper cleanup of the texture data.Also applies to: 316-318
323-323: LGTM!The changes are consistent with the member variable renaming in the
Textureclass.Also applies to: 336-337, 341-343, 350-352
323-323: Verify the return type change.The function now returns
0linstead of0when the texture pointer isnullptr. Ensure that this change is intentional and does not affect the caller's behavior.
366-385: LGTM!The changes improve the function's efficiency by avoiding unnecessary texture creation and provide a more structured approach to initializing the default texture. The initialization logic is consistent with the member variable renaming in the
Textureclass.
396-400: LGTM!The changes are consistent with the member variable renaming in the
Textureclass, ensuring proper cleanup of texture resources.
438-439: LGTM!The changes are consistent with the member variable renaming in the
Textureclass, ensuring proper initialization of texture resources.Also applies to: 446-446
src/Engine/App/AssimpWrapper.cpp (2)
99-99: LGTM!The code changes are approved.
99-99: LGTM!The code changes are approved.
src/Engine/RenderBackend/RenderCommon.h (2)
416-425: Approve the renaming of member variables to PascalCase.The change from snake_case to PascalCase for member variable names improves readability and aligns with common C++ naming conventions for public members. This is a good refactoring.
429-429: Verify the implementation of the newclear()method.The addition of the
clear()method indicates new functionality to manage or reset the texture data.Please provide the implementation of the
clear()method. I'll review it to ensure correctness and suggest any necessary improvements.src/Engine/RenderBackend/OGLRenderer/OGLRenderBackend.cpp (1)
805-806: LGTM!The changes to the texture property access naming convention improve consistency and readability without altering functionality.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| @@ -328,23 +333,23 @@ | |||
|
|
|||
| i32 width = 0, height = 0, channels = 0; | |||
|
|
|||
| tex->m_data = stbi_load(path.c_str(), &width, &height, &channels, 0); | |||
| if (nullptr == tex->m_data) { | |||
| tex->Data = stbi_load(path.c_str(), &width, &height, &channels, 0); | |||
Check failure
Code scanning / CodeQL
Uncontrolled data used in path expression High
Fix finding
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- src/Engine/RenderBackend/RenderCommon.cpp (5 hunks)
Files skipped from review as they are similar to previous changes (1)
- src/Engine/RenderBackend/RenderCommon.cpp
Summary by CodeRabbit
New Features
Bug Fixes
Refactor