Skip to content

Merge graphics library into main library with untested functions#50

Open
YuanSang0512 wants to merge 16 commits intogkit-org:mainfrom
YuanSang0512:feature/merge-render-lib
Open

Merge graphics library into main library with untested functions#50
YuanSang0512 wants to merge 16 commits intogkit-org:mainfrom
YuanSang0512:feature/merge-render-lib

Conversation

@YuanSang0512
Copy link
Copy Markdown
Contributor

No description provided.

Copy link
Copy Markdown
Contributor

@CoraBlack CoraBlack left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

我只审核完 include 目录下的东西,但依旧有很多东西需要更改:

  • 主库中函数方法均使用蛇形命名法
  • 其余注释和代码等风格请参考主库其他代码,库需要统一的代码风格才能支撑维护性

Note

  • 多审阅自己的代码,避免意义不明的改动
  • 多使用git进行小提交,大量的代码更改会让我我无法快速追踪代码的更改内容和理解修改的意义。此外,频繁细小的提交有利于代码回退和追踪,尤其使用AI工具的情况下,这是好的开发习惯

Comment thread test/CMakeLists.txt Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

应当使用git-lfs追踪二进制资源,避免仓库膨胀和困于解决合并冲突

Comment on lines +34 to +39
- name: Install Python dependencies
shell: cmd
run: |
echo "📦 Installing Windows dependencies..."

echo "✅ Windows dependencies installed successfully"
echo "📦 Installing Python dependencies..."
C:\hostedtoolcache\windows\Python\3.14.4\x64\python3.exe -m pip install jinja2
echo "✅ Python dependencies installed successfully"
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hyw? 为什么要把Windows dependents 改成 Python dependents

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

不知道,全是claude写的

Comment on lines -66 to -74
echo "Job status: ${{ job.status }}"

# # List built files
# echo "📁 Built files:"
# find ${{ github.workspace }}/build -name "*.so" -o -name "*.dll" -o -name "*.a" -o -name "*.lib" 2>/dev/null || echo "No library files found"

# # Show disk usage
# echo "💾 Disk usage:"
# du -sh ${{ github.workspace }}/build 2>/dev/null || echo "Build directory size info not available"
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

同上

Comment thread include/gkit/graphic/opengl/config.hpp Outdated
Comment thread include/gkit/graphic/opengl/StateManager.hpp Outdated
Comment on lines +110 to +111
/// @brief Apply all dirty states to OpenGL
auto Apply() -> void;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

考虑添加 const 修饰

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

考虑添加 const 修饰

why

Comment thread include/gkit/graphic/opengl/Texture.hpp
Comment on lines +57 to +66
/**
* @brief Add an element to the layout (unsupported type - compile-time error)
* @tparam T Data type
* @param count Number of components
*/
template<typename T>
void Push(uint32_t count)
{
static_assert(sizeof(T) == 0, "Unsupported type for VertexBufferLayout::Push");
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

未完成?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

未完成?

没看懂,还缺什么

Comment thread include/gkit/graphic/opengl/VertexBufferLayout.hpp Outdated
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR integrates a new OpenGL-based graphics module into gkit, adds GLAD as a third-party dependency, and introduces a graphics “test” executable plus shader assets to exercise the new rendering path.

Changes:

  • Add GLAD submodule/build integration and link GLAD/SDL dependencies into the main libraries/tests.
  • Introduce a new src/graphic module (Renderer, Shader, OpenGL buffer/state wrappers) and corresponding public headers.
  • Add a test/graphic/test_window.cpp demo plus shader files/resources wiring.

Reviewed changes

Copilot reviewed 32 out of 34 changed files in this pull request and generated 20 comments.

Show a summary per file
File Description
third_party/CMakeLists.txt Adds GLAD generation/build and exposes generated include dir.
src/CMakeLists.txt Adds graphic/ subdir and links GLAD to gkit libs.
src/graphic/CMakeLists.txt Defines gkit_graphic object library with OpenGL sources.
src/graphic/Renderer.cpp Implements basic clear/draw calls via OpenGL.
src/graphic/Shader.cpp Implements shader parsing/compile/link and uniform setters.
src/graphic/opengl/config.cpp Adds viewport helper wrappers around glViewport.
src/graphic/opengl/VertexBuffer.cpp Adds VBO wrapper implementation.
src/graphic/opengl/IndexBuffer.cpp Adds IBO wrapper implementation.
src/graphic/opengl/VertexArray.cpp Adds VAO wrapper + instancing attribute setup.
src/graphic/opengl/Texture.cpp Adds texture wrapper skeleton (bind/unbind + lifetime).
src/graphic/opengl/FrameBuffer.cpp Adds FBO wrapper implementation and attachment helpers.
src/graphic/opengl/RenderBuffer.cpp Adds RBO wrapper implementation.
src/graphic/opengl/StateManager.cpp Adds dirty-flag OpenGL state manager implementation.
include/gkit/graphic/Renderer.hpp Public Renderer API.
include/gkit/graphic/Shader.hpp Public Shader API and shader source struct.
include/gkit/graphic/opengl/config.hpp Public OpenGL enums/constants + viewport API.
include/gkit/graphic/opengl/VertexBuffer.hpp Public VBO wrapper.
include/gkit/graphic/opengl/IndexBuffer.hpp Public IBO wrapper.
include/gkit/graphic/opengl/VertexBufferLayout.hpp Public vertex layout helper.
include/gkit/graphic/opengl/VertexArray.hpp Public VAO wrapper.
include/gkit/graphic/opengl/Texture.hpp Public texture wrapper API.
include/gkit/graphic/opengl/FrameBuffer.hpp Public framebuffer wrapper API.
include/gkit/graphic/opengl/RenderBuffer.hpp Public renderbuffer wrapper API.
include/gkit/graphic/opengl/StateManager.hpp Public OpenGL state manager API.
test/CMakeLists.txt Adds graphic/*.cpp tests and links SDL/GLAD for test executables.
test/graphic/test_window.cpp Adds interactive SDL/OpenGL window demo using new module.
test/graphic/basic.shader Adds basic shader asset.
test/graphic/texture.shader Adds textured quad shader asset.
test/graphic/post_process.shader Adds post-processing shader asset.
.gitmodules Adds GLAD submodule.
.gitignore Ignores generated GLAD output directory.
.github/workflows/windows-debug-build-test.yml Updates Windows CI steps (Python/Jinja2 install).

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread src/graphic/opengl/VertexBuffer.cpp
Comment thread include/gkit/graphic/opengl/VertexBuffer.hpp
Comment thread include/gkit/graphic/opengl/IndexBuffer.hpp
Comment thread test/graphic/test_window.cpp
Comment thread src/graphic/Shader.cpp Outdated
Comment thread include/gkit/graphic/opengl/Texture.hpp
Comment thread include/gkit/graphic/Shader.hpp
Comment thread src/graphic/opengl/Texture.cpp
Comment thread src/graphic/Renderer.cpp Outdated
Comment on lines +6 to +10
glad_add_library(glad_gl STATIC
API "gl:core=4.6"
REPRODUCIBLE
LOCATION "${CMAKE_SOURCE_DIR}/third_party/glad_generated"
)
Copy link

Copilot AI Apr 29, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

glad_add_library(... LOCATION "${CMAKE_SOURCE_DIR}/third_party/glad_generated" ) generates code into the source tree. This breaks read-only source checkouts and leaves untracked/generated files behind after builds. Prefer generating into the build directory (e.g., under ${CMAKE_BINARY_DIR}) and point include dirs at that location, or make the location configurable via a cache variable.

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@CoraBlack 怎么说,是否采纳

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants