Implementation of UI wrapper #7
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
yanchith
requested changes
Aug 19, 2019
There was a problem hiding this comment.
Nice job!
Except for ui_capture_{keyboard,mouse} and the vulkan validation layer errors, all my comments are minor and easily fixable.
Note: either the FPS computation is broken, or vsync doesn't work, got 2000fps on debug builds and over 9000 on release builds 😄
Would explain the GPU usage.
Values in IO structure are updated after the current imgui's frame starts. Therefore our calls to `want_capture_mouse` and `want_capture_keyboard` had outdated values. This also didn't work well with `UI::create` and its callback. So I removed the whole UI wrapper and work directly with barebones calls to all involved libraries. All of this orchestration is present only in `main` and doesn't leak to other modules, so it's a good compromise.
yanchith
reviewed
Aug 21, 2019
src/imgui_renderer.rs
Outdated
| @@ -129,7 +130,8 @@ impl ImguiRenderer { | |||
| depth_stencil_state: None, | |||
| index_format: wgpu::IndexFormat::Uint16, // FIXME(yanchith): may need 32bit indices! | |||
| vertex_buffers: &[wgpu::VertexBufferDescriptor { | |||
| stride: u64::try_from(Self::DRAW_VERT_SIZE).unwrap(), | |||
| stride: u64::try_from(wgpu_size_of::<imgui::DrawVert>()) | |||
There was a problem hiding this comment.
Redundant try_from, wgpu_size_of already returns u64 (type aliased as wgpu::BufferAddress)
yanchith
reviewed
Aug 21, 2019
| @@ -105,14 +105,41 @@ fn main() { | |||
| let duration_running = now.duration_since(time_start); | |||
| time = now; | |||
|
|
|||
| let duration_last_frame_s = duration_last_frame.as_secs() as f32 | |||
There was a problem hiding this comment.
Can leave a FIXME here to change to https://doc.rust-lang.org/std/time/struct.Duration.html#method.as_secs_f32 when it stabilizes in 1.38.0
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is a convenience wrapper around imgui and all of its orchestration. Its practicality can be questioned, but it's the first iteration and I find it to be friendlier than the barebones alternative. All options are also hardcoded, which I (partly) don't expect to stay that way in the future.