Skip to content

Dev 2025.11.19 - #11

Merged
caffeine0coffee merged 37 commits into
mainfrom
dev_2025.11.19
Apr 9, 2026
Merged

caffeine0coffee merged 37 commits into
mainfrom
dev_2025.11.19

Conversation

@caffeine0coffee

Copy link
Copy Markdown
Owner

No description provided.

Copilot AI left a comment

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.

Pull request overview

This PR represents a significant architectural refactoring of the Vulkan rendering initialization code. The main changes separate concerns by extracting window management into a dedicated Window class and consolidating Vulkan context management into a Renderer class with a nested Context class. The previous monolithic render_context.hpp/cpp has been removed in favor of this more modular design.

Key Changes:

  • Introduced Window class to manage GLFW window lifecycle and Vulkan surface creation
  • Refactored Vulkan context management into Renderer::Context with explicit initialization phases (instance, then device)
  • Restructured main.cpp to use the new initialization flow with explicit ordering of Vulkan instance, window, and device setup

Reviewed changes

Copilot reviewed 10 out of 16 changed files in this pull request and generated 13 comments.

Show a summary per file
File Description
src/window.hpp New header defining the Window class for GLFW window and Vulkan surface management
src/window.cpp Implementation of Window class with GLFW initialization and Vulkan surface creation
src/render_context.hpp Removed - old monolithic context class replaced by new architecture
src/render_context.cpp Removed - functionality split between Window and Renderer classes
src/main.cpp Updated to use new initialization flow with Renderer and Window classes
src/core/renderer.hpp New header defining Renderer singleton with nested Context class for Vulkan state
src/core/renderer.cpp Implementation of Renderer with separate initialization phases and atomic guards
doc/blue_print.md New design document outlining architecture and planned features (Japanese)
AGENTS.md New guidelines document for AI assistants with project conventions (Japanese)
CPPLINT.cfg Added filter to disable include_order checks

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

Comment thread src/core/renderer.cpp Outdated
Comment thread src/window.cpp
Comment on lines +23 to +24
glfwTerminate();
spdlog::trace("GLFW terminated.");

Copilot AI Nov 24, 2025

Copy link

Choose a reason for hiding this comment

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

glfwTerminate() is called in the Window destructor, but glfwInit() is called in create_glfw_window_(). This is problematic because:

  1. If multiple Window instances are created (since copy is allowed), the first destroyed window will terminate GLFW for all instances
  2. glfwTerminate() should only be called once after all GLFW windows are destroyed
  3. The current design violates RAII principles for GLFW lifecycle management

Consider either:

  • Managing GLFW initialization/termination at the application level (in main)
  • Or implementing a reference-counted GLFW context manager
  • Or ensuring Window is non-copyable and used as a singleton
Suggested change
glfwTerminate();
spdlog::trace("GLFW terminated.");

Copilot uses AI. Check for mistakes.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

I'll address this in #12

Comment thread src/core/renderer.cpp Outdated
Comment thread src/window.cpp Outdated
Comment thread src/core/renderer.cpp Outdated
Comment thread src/window.hpp Outdated
Comment thread src/window.cpp Outdated
Comment thread src/core/renderer.cpp Outdated
Comment thread src/core/renderer.cpp Outdated
Comment thread src/main.cpp
@caffeine0coffee
caffeine0coffee merged commit 6761201 into main Apr 9, 2026
2 of 4 checks passed
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.

2 participants