Session 1 work#1
Conversation
pbantolas
left a comment
pbantolas
left a comment
There was a problem hiding this comment.
Great work on extending the Vulkan renderer!
Overall the approach makes sense at this stage and the code looks correct for the most part. The things I've flagged are mostly architectural and touch on ownership:
- Input tied to frame rate
- Input logic is owned by the renderer
- Camera, rectangles are owned by the renderer. New layer should exist (e.g. Scene) that can use the Renderer as a pure 'drawing machine' instead of ownership of scene state
- The camera representation has issues when world grows big, and isn't tied to world units but rather 'NDC'/screen units.
| void (*init)(VisualRuntimeState *, SurfaceDescriptor *); | ||
| void (*resize)(VisualRuntimeState *, uint32_t, uint32_t); | ||
| void (*update)(VisualRuntimeState *, float); | ||
| void (*update)(VisualRuntimeState *, Input*, float); |
There was a problem hiding this comment.
Adding input to the update API is reasonable in this design, but consider how that decision ties user input to the frame rate, and situations where we don't want these coupled.
E.g. frame rate drops, but update needs to be consumed
| struct Camera2D { | ||
| double x; | ||
| double y; | ||
| double zoom = 1.0; |
There was a problem hiding this comment.
Zoom starts at 1.0. What happens with precision when the user zooms out? what happens when they zoom in?
|
|
||
| void main() { | ||
| gl_Position = frame_uniforms.matrix * vec4(in_position, 0.0, 1.0); | ||
| gl_Position = frame_uniforms.view * frame_uniforms.matrix * vec4(in_position, 0.0, 1.0); |
There was a problem hiding this comment.
I think the matrix multiplication should be the other way around? E.g. matrix * view
(matrix is just aspect ratio, but you can think of it as eventually becoming the orthographic projection matrix)
| uint32_t render_width_ = 0; | ||
| uint32_t render_height_ = 0; | ||
| Camera2D camera_; | ||
| static const 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)
| if (upload_size > 0) { | ||
| void *data = nullptr; | ||
| vkMapMemory(context_.device(), vertex_buffer_memory_, 0, upload_size, 0, &data); | ||
| std::memcpy(data, staged_vertices_, upload_size); | ||
| vkUnmapMemory(context_.device(), vertex_buffer_memory_); |
There was a problem hiding this comment.
Existing rectangle geometry is reuploaded every frame. Also, memory mapping is not checked if it fails or not.
How can you avoid reuploading the same data each frame?
Can you think of better ways to synchronise the GPU upload?
| } | ||
| } | ||
|
|
||
| void RendererBackend::handle_input(Input* input) { |
There was a problem hiding this comment.
The renderer owns the input policy.
The problem with this approach is that any change the product side wants to make, you would have to update the code in this function.
Also, the renderer should own "rendering/drawing" stuff, and not have ownership of which input changes what.
How about introducing a new layer before the renderer, owned by the visual runtime, that could translate the input into what the renderer cares about? I.e. the view matrix, what vertex data to draw, not what a rectangle is, or how input/clicks are handled!
|
|
||
| // Handle panning only if leftclick is down | ||
| if (input->LCLICK_DOWN) { | ||
| double dx = ((2.0 * input->MOUSE_X / input->SCREEN_WIDTH) - 1.0f) - (input->LAST_MOUSE_X); |
There was a problem hiding this comment.
Because this code adds NDC deltas to camera translation, panning changes speed depending on zoom. Ideally panning should be tied to a world unit concept!
| float half_width = 0.35; | ||
| float half_height = 0.25; |
There was a problem hiding this comment.
Ok for demo, but consider how to drive size and color from product data, not renderer policy.
| uniforms.matrix[1][1] *= -1.0f; | ||
|
|
||
| // Set the view matrix based on the updated camera | ||
| uniforms.view = glm::translate(glm::mat4(1.0f), glm::vec3(camera_.x, camera_.y, 1.0)); |
There was a problem hiding this comment.
translation z should be 0
| static void visual_runtime_update_impl(VisualRuntimeState *state,Input* input, float dt) { | ||
| state->frame_count++; | ||
| state->elapsed_time += dt; | ||
| g_renderer.handle_input(input); |
There was a problem hiding this comment.
Here's where input is tied to frame rate.
This function will be called by the main frame loop, or a platform frame callback timer.
If input is handled here, then any frame stalls would stall input.
You could introduce a check that only updates at given rate, but the owner can still call this parent function from a context that might have stalled.
| uint32_t height; | ||
| }; | ||
|
|
||
| constexpr uint32_t VISUAL_RUNTIME_API_VERSION = 4; |
There was a problem hiding this comment.
API version should be bumped every time there are changes to the ABI/API.
2 participants
PR for code review