Session 1 work#2
Conversation
…er. Exposed abstractions to the host via the API.
refactor: Background Color is now just Color, because it can be used by other scene elements. fix: Vulkan Host printf specifiers, GetWindowSize instead of GetFrameBufferSize for window accurate shape placement and panning.
There was a problem hiding this comment.
Overall good shape and the new API looks good!
The main issues:
- Rectangles are drawn in 'immediate' mode, by being resent to the visual runtime each frame. This creates unnecessary GPU writes each frame. Think of a way to 'cache' the vertices on the visual runtime side (not inside the renderer) so that the buffers can remain stable if there are no changes
- The arbitrary amount of rectangles that can be drawn
- Input coordinates and units need work, without exposing renderer internals. Consider what happens at extreme cases and how precision is lost in an 'infinite' canvas.
| float pan_x; | ||
| float pan_y; | ||
| float zoom; | ||
| }; |
There was a problem hiding this comment.
This stores the "world/canvas" space coordinates of the view. How does storing float affect precision especially on very large X/Y/zoom values?
What can happen at very low zoom values?
| float pan_y_ = 0.0f; | ||
| float zoom_ = 1.0f; | ||
|
|
||
| static constexpr uint32_t MAX_RECTS = 128; |
There was a problem hiding this comment.
Probably ok for a demo, but what happens if user needs has a canvas with more than 128 rectangles?
How would you handle it?
(it's a fair thing to not want to RENDER more than N rectangles at a time, but consider how you could update this policy to cull the correct/invisible rectangles)
| const VkDeviceSize upload_size = sizeof(Vertex) * staged_vertex_count_; | ||
| if (upload_size > 0) { | ||
| void *data = nullptr; | ||
| vkMapMemory(context_.device(), vertex_buffer_memory_, 0, upload_size, 0, &data); |
There was a problem hiding this comment.
vkMapMemory needs to be checked for its return value before writing to memory.
Can you think of better ways to synchronise the GPU upload?
| void shutdown(); | ||
| void set_background_color(float r, float g, float b); | ||
| void set_view(float pan_x, float pan_y, float zoom); | ||
| void draw_rect(float top_left_x, float top_left_y, float width, float height, float r, float g, float b); |
There was a problem hiding this comment.
Are there any ways to reduce the argument count for this function? Or how would you make it more generic? (other shapes for example).
| const float ref = static_cast<float>(std::min(w, h)); | ||
|
|
||
| // Convert pixel delta → canvas units: NDC range is 2 (-1..1), ref is the | ||
| // shorter dimension (matches aspect-correction in the shader). | ||
| const double dx = x - state->last_mouse_x; | ||
| const double dy = y - state->last_mouse_y; | ||
| state->view.pan_x -= static_cast<float>(2.0 * dx / ref) / state->view.zoom; | ||
| state->view.pan_y += static_cast<float>(2.0 * dy / ref) / state->view.zoom; |
There was a problem hiding this comment.
This logic converts pixels to world/canvass units
Dividing by very low zoom values can explode the pan amounts.
Also, what happens if the framebuffer (pixel coordinates) and window (logical units, high density displays or compositors) mismatch?
| state->last_mouse_y = y; | ||
| } | ||
|
|
||
| void on_key(GLFWwindow *window, int key, int /*scancode*/, int action, int /*mods*/){ |
There was a problem hiding this comment.
The mouse to world conversion duplicates the renderer's projection logic here. aspect, sx/sy, the y flip, zoom order and pan convention all have to match update_frame_uniforms (wrong coupling). Changing the renderer matrix can silently break rectangle placement.
| uniforms.matrix = glm::scale(glm::mat4{1.0f}, glm::vec3(sx, sy, 1.0f)); | ||
| uniforms.matrix = glm::scale(uniforms.matrix, glm::vec3(zoom_, zoom_, 1.0f)); | ||
| uniforms.matrix = glm::translate(uniforms.matrix, glm::vec3(-pan_x_, -pan_y_, 0.0f)); |
There was a problem hiding this comment.
I believe you should split into 2 matrix uniforms, one projection (contains the aspect ratio) and a view matrix (translation and scale).
2 participants
Code review