feat: Merge Editor into one #29
Conversation
|
Warning Rate limit exceeded@WSQS has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 16 minutes and 16 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughReplaced per-vertex NodeEditor editors with a single Editor UI that switches between three modes: Node (vertex position editing), Vertex (vertex shader editing), and Fragment (fragment shader editing). Mode changes trigger vertex buffer re-uploads or shader updates immediately. Changes
Sequence DiagramsequenceDiagram
actor User
participant UI as Editor UI
participant Buffer as Vertex Buffer
participant Pipeline as Pipeline Wrapper
User->>UI: Select "Node" Mode
UI->>UI: Render drag editors for vertices
User->>UI: Modify vertex position
UI->>Buffer: Re-upload vertex buffer
Buffer-->>UI: Buffer updated
User->>UI: Switch to "Vertex" Mode
UI->>UI: Render multiline vertex shader editor
User->>UI: Edit vertex shader
UI->>Pipeline: Update vertex_shader
Pipeline-->>UI: Shader compiled & applied
User->>UI: Switch to "Fragment" Mode
UI->>UI: Render multiline fragment shader editor
User->>UI: Edit fragment shader
UI->>Pipeline: Update fragment_shader
Pipeline-->>UI: Shader compiled & applied
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
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 |
|
Here's the code health analysis summary for commits Analysis Summary
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
main.cpp (1)
140-180: Consider adding a default case to the switch statement.While the combo box constrains
currentto valid values (0-2), adding a default case improves defensive programming and makes the code more maintainable if modes are added later.break; + default: + break; } ImGui::End();
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
main.cpp(1 hunks)
⏰ 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). (20)
- GitHub Check: build (Debug, gcc)
- GitHub Check: build (windows-latest, Release, gcc)
- GitHub Check: build (windows-latest, Release, clang)
- GitHub Check: build (windows-latest, Debug, gcc)
- GitHub Check: build (Release, clang)
- GitHub Check: build (windows-latest, Debug, cl)
- GitHub Check: build (windows-latest, Debug, clang)
- GitHub Check: build (Release, gcc)
- GitHub Check: clang-tidy-review
- GitHub Check: build (Debug, clang)
- GitHub Check: build (Release, gcc)
- GitHub Check: build (Release, clang)
- GitHub Check: build (Debug, clang)
- GitHub Check: build (Debug, gcc)
- GitHub Check: build (windows-latest, Debug, cl)
- GitHub Check: build (windows-latest, Release, clang)
- GitHub Check: build (windows-latest, Release, cl)
- GitHub Check: build (windows-latest, Release, gcc)
- GitHub Check: build (windows-latest, Debug, clang)
- GitHub Check: build (windows-latest, Debug, gcc)
🔇 Additional comments (2)
main.cpp (2)
137-139: LGTM!The static variable correctly persists the selected mode across frames, and the combo box setup is appropriate.
142-152: LGTM!The vertex position editing logic is correct. The change tracking and conditional buffer upload ensure efficient updates only when modifications occur.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
main.cpp (2)
153-164: Critical issue from previous review remains unaddressed.The shader recompilation on every keystroke issue previously flagged as critical has not been resolved.
InputTextMultilinestill triggersset_vertex_shaderfor each character typed, causing expensive recompilation and errors on incomplete syntax.
166-178: Critical issue from previous review remains unaddressed.The shader recompilation on every keystroke issue previously flagged as critical has not been resolved.
InputTextMultilinestill triggersset_fragment_shaderfor each character typed, causing the same performance and stability issues as the vertex shader case.
🧹 Nitpick comments (1)
main.cpp (1)
140-140: Add a default case for defensive coding.While the combo box constrains
currentto valid values (0-2), adding a default case improves robustness and addresses the static analysis warning.Apply this diff:
switch (current) { case 0: { auto change = ImGui::DragFloat3("##node1", vertices[0].position(), 0.01f, -1.f, 1.f); change = ImGui::DragFloat3("##node2", vertices[1].position(), 0.01f, -1.f, 1.f) || change; change = ImGui::DragFloat3("##node3", vertices[2].position(), 0.01f, -1.f, 1.f) || change; if (change) { vertex_buffer.upload(&vertices, sizeof(vertices), 0); } } break; case 1: { auto line_count = std::count(vertex_source.begin(), vertex_source.end(), '\n'); ImVec2 size = ImVec2( ImGui::GetContentRegionAvail().x, std::min(ImGui::GetTextLineHeight() * (line_count + 3), ImGui::GetContentRegionAvail().y)); if (ImGui::InputTextMultiline("##vertex editor", &vertex_source, size, ImGuiInputTextFlags_AllowTabInput)) { pipeline_wrapper.set_vertex_shader(vertex_source); } } break; case 2: { auto line_count = std::count(fragment_source.begin(), fragment_source.end(), '\n'); ImVec2 size = ImVec2( ImGui::GetContentRegionAvail().x, std::min(ImGui::GetTextLineHeight() * (line_count + 3), ImGui::GetContentRegionAvail().y)); if (ImGui::InputTextMultiline("##fragment editor", &fragment_source, size, ImGuiInputTextFlags_AllowTabInput)) { pipeline_wrapper.set_fragment_shader(fragment_source); } } break; + default: + break; }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
main.cpp(1 hunks)
🔇 Additional comments (2)
main.cpp (2)
137-139: LGTM! Good use of std::array.The mode selection combo box is well-implemented using
std::arrayinstead of C-style arrays, which addresses the previous static analysis warnings.
142-152: LGTM! Efficient vertex buffer update pattern.The node editing mode correctly tracks changes across all vertex position editors and only re-uploads the vertex buffer when modifications occur, avoiding unnecessary GPU transfers.
|
clang-tidy review says "All clean, LGTM! 👍" |
Summary by CodeRabbit