Conversation
This reverts commit 3160414.
* GNU 12 compatability * cleaned header
There was a problem hiding this comment.
Pull Request Overview
This PR updates to version 0.0.2 by refactoring dependency management, adding new optional Python bindings, and enhancing the main applications with denoising and GUI improvements.
- Refactored external dependencies (glad/glfw via FetchContent) and updated submodule configuration for imgui/implot and pybind11
- Added
OVR_BUILD_PYTHON_BINDINGSoption and corresponding CMake logic - Enhanced
main_app.cppandmain_batch.cppwith denoiser support, frame swapping, and cleaned up the GUI
Reviewed Changes
Copilot reviewed 314 out of 314 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| extern/gdt/gdt/gdt.h | Added conditional prettyDouble under GDT_ENABLE_STRING_OPERATIONS |
| extern/gdt/cmake/configure_build_type.cmake | Disabled output directory settings |
| extern/gdt/CMakeLists.txt | Switched gdt to INTERFACE library and cleaned target properties |
| extern/dep_glfw.cmake | Introduced FetchContent-based GLFW dependency |
| extern/dep_glad.cmake | Converted GLAD handling to FetchContent and fixed includes |
| extern/configure.cmake | Updated to use the new dep_glfw/dep_glad scripts and added Python binding logic |
| data/configs | Added multiple new scene JSON configuration files |
| apps/main_batch.cpp | Updated camera orientation, added ren->swap() before mapping |
| apps/main_app.cpp | Refactored GUI, integrated denoiser, cleaned up frame handling |
| apps/CMakeLists.txt | Removed implot from render app linking |
| CMakeLists.txt | Enhanced CMake policies, added Python bindings and submodule updates |
| .gitmodules | Adjusted submodule list to imgui/implot and pybind11 |
Comments suppressed due to low confidence (2)
apps/CMakeLists.txt:12
- Removing 'implot' from the linked libraries breaks ImPlot usage in the main application GUI. Consider re-adding 'implot' to the target_link_libraries to avoid missing symbols at link time.
- implot
.gitmodules:3
- The removal of the 'extern/glfw' and 'extern/glad' submodule entries may lead to missing submodule checkouts for those dependencies. Ensure dependencies are managed consistently (e.g., via FetchContent) or restore the submodule entries if needed.
-[submodule "extern/glfw"]
| add_subdirectory(ovr) | ||
| add_subdirectory(apps) | ||
| add_subdirectory(projects) | ||
| add_subdirectory(python) |
There was a problem hiding this comment.
The 'add_subdirectory(python)' call is unconditional and will fail if the 'python' directory does not exist. It should be wrapped in an 'if(OVR_BUILD_PYTHON_BINDINGS)' guard to only include it when the option is enabled.
| add_subdirectory(python) | |
| if(OVR_BUILD_PYTHON_BINDINGS) | |
| add_subdirectory(python) | |
| endif() |
| #include "common/vidi_fps_counter.h" | ||
| #include "common/vidi_highperformance_timer.h" | ||
| #include "renderer.h" | ||
| #include "serializer/serializer.h" |
There was a problem hiding this comment.
[nitpick] The 'serializer/serializer.h' header is included but not used in this file. Consider removing this include to reduce unnecessary dependencies.
| #include "serializer/serializer.h" |
| #include "serializer/serializer.h" | ||
|
|
There was a problem hiding this comment.
[nitpick] The 'serializer/serializer.h' header is included but not referenced in the code. Removing unused includes can improve compile times and reduce coupling.
| #include "serializer/serializer.h" |
| @@ -43,6 +43,6 @@ if(NOT SET_UP_CONFIGURATIONS_DONE) | |||
| # endif() | |||
| endif() | |||
|
|
|||
There was a problem hiding this comment.
[nitpick] The commented-out CMake output directory settings remain in the file. Consider removing or documenting why these lines are disabled to keep the build script clean.
| # The following output directory settings are commented out because the default | |
| # behavior of CMake is sufficient for this project. Uncomment and modify these | |
| # lines if custom output directories are required in the future. |
| cmake/FindOptiX.cmake | ||
| gdt/gdt.h | ||
| gdt/gdt.cpp) | ||
| # gdt/gdt.cpp |
There was a problem hiding this comment.
[nitpick] The commented reference to 'gdt/gdt.cpp' in the target definition could be removed if that source file is no longer intended to be part of this target.
| # gdt/gdt.cpp |
On-going developments
Propogate Internal Changes
So many changes has been added, maybe let's do some testing and merge them 🙏