Wraps the dear ImGui custom drawing API #111

malikolivier · 2018-03-30T02:36:28Z

This is an attempt at wrapping ImGui's custom drawing API.

The custom rendering example provided with ImGui has been completely implemented. The result is as below:

Each patch should be self-explanatory and can be reviewed separately.

This collection of patches can be split into 4 sections.

Implementation of window_draw_list.rs, the API itself
Add a few helper methods in Ui. These functions will be used for the examples.
Add show_example_app_custom_rendering in test_window_impl
Add another example test_drawing_channels_split, showing how channels_split can be used. => The example can be removed from this PR if you do not like it.

This is an improvement over #102. With support for safe use of channels_split and generics to allow a more convenient use of the API.

malikolivier · 2018-03-30T03:41:56Z

The build is failing as below:

---- src/lib.rs - Ui<'ui>::with_window_draw_list (line 1187) stdout ----
	error[E0432]: unresolved import `imgui`
 --> src/lib.rs:1189:5
  |
4 | use imgui::*;
  |     ^^^^^ Maybe a missing `extern crate imgui;`?

thread 'rustc' panicked at 'couldn't compile the test', librustdoc/test.rs:298:13

I don't really understand what's the issue. The build succeeds locally when I run cargo test --all.

Lymia · 2018-04-07T16:34:54Z

https://doc.rust-lang.org/stable/rustdoc/documentation-tests.html#pre-processing-examples

What's happening is that the doctest is being wrapped in a fn main(), (since they contain none), and the use doesn't work because imgui is in the scope of main, not the module.

~~Add a # fn main() { } line to the end, I guess, since you can't reasonably test this? Or a hidden main that runs the function in a imgui frame also works.~~ Edit: Actually, removing the # extern crate imgui; line should work just as well. Doctests implicitly add one.

malikolivier · 2018-04-08T00:53:25Z

@Lymia Thanks! I just fixed the examples as you suggested. Travis CI should get a successful build now.

Actually the build succeeded locally in my environment because I used the nightly compiler. On stable, it fails as in the CI.

Gekkio

Thanks! 😄

Gekkio · 2018-04-12T20:38:54Z

src/lib.rs

@@ -82,6 +84,15 @@ pub fn get_version() -> &'static str {
    }
 }

+/// Represents one of the buttons of the mouse
+pub enum ImMouseButton {


Derive at least Copy, Clone, Eq, PartialEq, Debug

Gekkio · 2018-04-12T20:42:06Z

src/window_draw_list.rs

+/// `From<[f32; 3]>`, `From<(f32, f32, f32, f32)>` and `From<(f32, f32, f32)>`
+/// for convenience. If alpha is not provided, it is assumed to be 1.0 (255).
+#[derive(Copy, Clone)]
+pub struct ImColor(ImU32);


Derive Eq, PartialEq, Debug

Gekkio · 2018-04-12T20:42:58Z

src/window_draw_list.rs

+/// All types from which ImGui's custom draw API can be used implement this
+/// trait. This trait is internal to this library and implemented by
+/// `WindowDrawList` and `ChannelsSplit`.
+pub trait DrawAPI<'ui> {


The lifetime parameter 'ui is not used at all in the trait so it's not necessary

Gekkio · 2018-04-12T20:52:16Z

src/window_draw_list.rs

+
+/// Object implementing the custom draw API.
+pub struct WindowDrawList<'ui> {
+    draw_list: *mut ImDrawList,


This needs a very thorough study, since it seems that we can have more than one WindowDrawList at a time. We won't have multiple mutable borrows in the normal Rust way, but multiple mutable raw pointers can still end up doing bad things.

Do you know how lib wrappers usually deal with this kind of issues? It seems difficult to enforce such check at compile time.

I propose to enforce the *mut ImDrawList borrow check at run time. using a static variable to store the state. This may not be a perfect solution, but I hope it is optimal for a rusty wrapper of imgui. I implement this idea in patch 586e102, please let me know your thoughts.

Gekkio · 2018-04-12T20:53:04Z

src/window_draw_list.rs

+    };
+}
+
+impl_draw_list_methods!(WindowDrawList);


Couldn't the methods be on the trait instead of manually duplicating everything with macros? I know this pattern has been used before with some input components in imgui-rs, but in retrospect I don't think it was the right way to do it.

I actually thought about doing that, so let me explain the reasoning behind my choice.

If the methods are defined on the trait, then the user of the lib will be required to import the DrawAPI trait to be able to use any of the methods. As adding a supplementary import is not convenient I chose to duplicate all method definitions with macros. With this design,DrawAPI is an internal trait used only internally for its draw_list method.

What do you think would be the best for the user's interest?

Yeah, I understand this reasoning and I've seen this pattern in some libs before (including imgui-rs itself ;) ). However, using macros to copy-paste code means that we're also ignoring potentially beneficial abstraction.

In this case, imagine that you want to write a function that draws something:

fn draw_my_amazing_thing<'ui>(dl: &WindowDrawList<'ui>) { dl.add_line(...); // ... }

This is all fine and you might be happy with this function, but now we've tied this function to WindowDrawList, so it won't work with ChannelsSplit even though it too has the add_line method. This seems strange if WindowDrawList and ChannelsSplit are supposed to have the same functionality.

What if you write a new trait to support both types? That seems very strange since we already have DrawAPI which could be used to abstract the common functionality.

What if you require that dl implements DrawAPI and use the Line::new function in the implementation instead of add_line? That works because it supports anything that implements DrawAPI. But now what's the point of having add_line?

In my opinion using macros to copy-paste code only makes sense when you want to copy some boilerplate into types that are so different that you know there's no need to abstract over them.

So, I'd prefer just having the functions in DrawAPI to avoid all these issues. Yes, the user has to import the trait, but this is just how things work in Rust. Is it a problem that you need to import std::io::Read to read from a std::fs::File?

However, don't start changing things just yet, because I think we can have a slightly different approach and abolish DrawAPI completely. I'll explain this in another comment 😃

That makes so much sense. Thank you very much for the explanation :) . I feel a better Rustacean now. Looking forward to your idea about a different approach then!

Gekkio · 2018-04-12T20:55:06Z

src/window_draw_list.rs

+    }
+}
+
+/// Represents a circle about to be drawn on the window


Gekkio · 2018-04-12T20:59:21Z

src/window_draw_list.rs

+impl<'ui> ChannelsSplit<'ui> {
+    /// Change current channel
+    pub fn channels_set_current(&self, channel_index: u32) {
+        unsafe { sys::ImDrawList_ChannelsSetCurrent(self.draw_list(), channel_index as i32) };


channel_index has an assertion in the underlying library. If we can't check the condition with a "nice" Rust panic, the abort should be documented.

Right. I will refactor to get a "nice" rust panic if the condition is unmet.

Gekkio · 2018-04-12T21:05:15Z

src/lib.rs

+    ///     });
+    /// }
+    /// ```
+    pub fn with_window_draw_list<F>(&self, f: F)


Using the with_ + closure method here feels strange, because there's no cleanup function that needs to be called. Also, this doesn't add any extra safety because it can be called recursively multiple times. WindowDrawList::new is also public, so this is not the only way to construct a WindowDrawList

Sure enough. I propose then to rename the function to get_window_draw_list and have it return a WindowDrawList object.

As you said, WindowDrawList::new should only be public within the crate.
cc878b4 fixed this issue.

malikolivier · 2018-04-15T10:32:47Z

Thanks for the review, I incorporated your comments in the PR and answered to your inquiries.

malikolivier · 2018-04-16T05:59:09Z

@Gekkio There are some conflicts now. What is your policy regarding the solving of conflicts?
Should I commit a merge or should I rebase?

Gekkio · 2018-04-16T06:03:53Z

Personally I prefer rebases for handling conflicts in short-lived pull requests, but you can choose whichever feels easier

malikolivier · 2018-04-16T06:07:48Z

Then I'll do a rebase. Rebases are better in the long term, as we get a cleaner git history. I will overwrite this PR with a rebased branch.

This patch makes the basic structure for a wrapper around Dear ImGui's drawing API. 1. Implement `with_window_draw_list` method on Ui. Call this method to get access to the `WindowDrawList` object. This object holds the methods to access ImGui's drawing API. 2. Dear ImGui uses the ImU32 (an unsigned c_int) to represent colors in the drawing API. This commit wraps this type with ImColor for convenience. Any color representation (3or4-tuples, 3or4-arrays, ImU32 or ImVec4) can be converted into ImColor for convenience. 3. Difference between WindowDrawList and ChannelsSplit: Most drawing functions can be called on WindowDrawList and ChannelsSplit objects. However for safety, some functions can only be called on WindowDrawList or ChannelsSplit instance. For example `channels_set_current` can only be called after channels have been split. To avoid code duplication, functions common to WindowDrawList and ChannelsSplit are implemented within the `impl_draw_list_methods` macro. 4. Implement drawing functions (in this commit, add_line only). Calling `add_line` returns a default representation of the line to be drawn, but does not draw it. Then parameters, such as thickness, can be set. You must call `build` to draw the line. All drawing functions will be implemented following this pattern.

This patch uses bitflags to set the whether the corners are rounded. Hence the `ImDrawCornerFlags` struct is defined, but only used internally. Externally, the valule of the flags can be changed with methods on the `Rect` structure such as `round_top_right` or `round_bot_left`. This patch wraps both ImDrawList_AddRectFilled and ImDrawList_AddRect. ImDrawList_AddRectFilled is seen as a particular case of `add_rect` where `filled` is set to `true`.

As `add_rect_filled_multicolor` does not have any option, the `build` pattern is not used and calling `add_rect_filled_multicolor` directly draws the rectangle on the window.

ImDrawList_AddCircle was missing an argument in the bindings, resulting in UB. This patches includes it and wrap the AddCircle API.

This commit adds two methods to the drawing APIs: with_clip_rect and with_clip_rect_intersect. Both wrap ImDrawList_PushClipRect and ImDrawList_PopClipRect. However, with_clip_rect_intersect sets the `intersect` argument of ImDrawList_PushClipRect to `true`.

This commit defines a new enum: `ImMouseButton`, which is used in the public-facing API of mouse-related methods. The discriminant values of the ImMouseButton enum are the value used internally by ImGui to represent the buttons of the mouse.

…endering show_example_app_custom_rendering is implemented exactly as it is in the original Dear ImGui in C++. The result should be the same. The only difference is that `DragFloat`, used to control the size of the drawings, is not implement as of now. This example demonstrates how the custom drawing API can be used.

…canvas Include drawing canvas example into show_example_app_custom_rendering. The example contains now everything included in the original C++ example provided with dear imgui.

As channels_split may be difficult to understand, this commit adds a simple example of its usage.

The test was failing because no `main` function was defined, as explained in the rustdoc documentation [1]. Add the "no_run" flag. This way, there is no attempt to run the test code. Only compile checks are done. Thus defining a `main` function is unnecessary. Moreover "export crate imgui" is implicit, so removed. [1] https://doc.rust-lang.org/stable/rustdoc/documentation-tests.html#pre-processing-examples.

For convenience.

At run time, the environment checks that at most one instance of WindowDrawList exists using a static boolean: WINDOW_DRAW_LIST_LOADED. If two WindowDrawList could exist at the same time, there would be several instances of the same `*mut ImDrawList`, which could lead to unfathomable bad things. When a WindowDrawList is created, WINDOW_DRAW_LIST_LOADED is set to true. And when it is dropped, WINDOW_DRAW_LIST_LOADED is set to false. Creating a new WindowDrawList while WINDOW_DRAW_LIST_LOADED is true causes a panic. AtomicBool could have been used instead of a bool for WINDOW_DRAW_LIST_LOADED. Though it allows to avoid the use of `unsafe { }`, the construct we are doing is already inherently unsafe. WindowDrawList and Ui are !Send and !Sync, so they cannot anyway be shared among threads. So we'd better be explicit and use an unsafe block with a normal bool.

Gekkio · 2018-04-16T06:20:27Z

👍

malikolivier · 2018-04-16T06:33:00Z

Rebase done. I found what seemed to be a bug while rebasing. The bug does not seem related to this PR. I opened a different issue: #115

Lymia · 2018-04-22T11:04:39Z

What's the status on this? This would be useful for my use case too.

Gekkio · 2018-04-22T14:09:43Z

Since there are no mutable borrows in the API, couldn't we remove all draw functions from ChannelsSplit and just use the same draw list all the time?

So, you would use it like this:

let draw_list = ui.get_window_draw_list();
draw_list.channels_split(2, |channels| {
    const RADIUS: f32 = 100.0;
    let canvas_pos = ui.get_cursor_screen_pos();
    channels.set_current(1);
    draw_list
        .add_line(
            canvas_pos,
            [canvas_pos.0 + RADIUS, canvas_pos.1 + RADIUS],
            RED,
        )
        .thickness(5.0)
        .build();

    channels.set_current(0);
    let center = (canvas_pos.0 + RADIUS, canvas_pos.1 + RADIUS);
    draw_list
        .add_circle(center, RADIUS, WHITE)
        .thickness(10.0)
        .num_segments(50)
        .build();
});

This approach makes DrawAPI pointless so it can be removed, and ChannelsSplit is reduced to a type with just one function: set_current (renamed from channels_set_current). All the XXX::new functions would then take WindowDrawList as an argument directly. Do you see any problems with this approach?

PS. I've noticed some lifetime issues that we need to think about, but let's first focus on the basic API.

malikolivier · 2018-04-22T14:24:48Z

Your proposition is a great idea! Thanks! Going into that direction tremendously reduces the complexity of the code.

What are the lifetime issues that you are hinting at?

Gekkio · 2018-04-22T14:41:08Z

It seems that a draw list is conceptually tied to a window, but 'ui in WindowDrawList<'ui> is the lifetime of the current frame. So, what happens if you use a WindowDrawList after the corresponding window is gone? I don't have time to investigate this right now, but imagine code like this (doesn't actually compile but the idea is valid):

let draw_list;
ui.window(im_str!("Uh oh"))
    .build(|| {
        draw_list = ui.get_window_draw_list();
        // lifetime says it's ok for draw_list to outlive the corresponding window
    });

draw_list.add_line(...); // ???

ui.window(im_str!("Uh oh 2"))
    .build(|| {
        draw_list.add_line(...); // ???
    });

malikolivier · 2018-04-22T15:22:03Z

Thanks. I will investigate that what happens if a WindowDrawList is used after the corresponding window is gone, so that we can come up with a solution.

If this is indeed an issue, we could imagine getting the draw list via a Window::build_with_draw_list method which could be used as follows:

ui.window(im_str!("Uh oh"))
    .build_with_draw_list(|draw_list| {
        // use draw_list

        // draw list is destroyed when it goes out of scope
    });

Ui::get_window_draw_list would then not be useful. We could delete it.

This would as well protect us from having several instances of WindowDrawList and allows us not to to have to use the dirty work-around I implemented in 454e980.

Are there cases where we could draw outside a window? I guess no.

Fancier API

Since there are no mutable borrows in the WindowDrawList API, we can remove all draw functions from ChannelsSplit and just use the same `draw_list' all the time, as shown in the `WindowDrawList::channels_split` example. This approach makes the `DrawAPI` trait pointless so it can be removed, and ChannelsSplit is reduced to a type with just one function: `set_current`.

malikolivier · 2018-04-24T04:46:57Z

I pushed the code with the approach you proposed that gets rid of DrawAPI.

Regarding the lifetime issues, I did the following experiment:

            // Get draw_list in the context of the "Debug" window:
            let draw_list = ui.get_window_draw_list();

            // Make another "Uh oh!" window
            ui.window(im_str!("Uh oh!"))
                .build(|| {
                    // use draw_list inside the context of the "Uh oh!" window
                });

As you can see, the content is drawn on the current window at the time Ui::get_window_draw_list was called (here the "Debug" window), not on the new window (here the "Uh oh!" window).

Maybe we should just ignore these life time issues and just precise in the documentation that the draw_list instance returned by Ui::get_window_draw_list is tied to the current UI window? What do you think?

parasyte · 2018-04-27T03:19:42Z

IMHO, the closure is definitely a great fit for this feature. The method name can be a bike shed, but it seems like if you want a draw_list reference, you should be able to request one on a window, and receive it in a closure.

Gekkio · 2018-04-29T08:35:16Z

Are there cases where we could draw outside a window? I guess no.

There actually is (sort of), because imgui adds a debug window by default if you don't explicitly create a window yourself. So it's ok to get a drawlist without ever creating a window. But this is a bit of a special case that we don't necessarily have to support.

Anyway...I think we can go without the closure for now, so I'll merge this pull request. If this approach turns out to be unsafe, we can change the API to the closure version later.

malikolivier force-pushed the draw-api branch from 03eb289 to 462d197 Compare April 8, 2018 00:51

Gekkio suggested changes Apr 12, 2018

View reviewed changes

malikolivier added 21 commits April 16, 2018 15:09

window_draw_list.rs: Add add_rect_filled_multicolor

d1879b2

As `add_rect_filled_multicolor` does not have any option, the `build` pattern is not used and calling `add_rect_filled_multicolor` directly draws the rectangle on the window.

window_draw_list.rs: Wrap add_circle

878de08

ImDrawList_AddCircle was missing an argument in the bindings, resulting in UB. This patches includes it and wrap the AddCircle API.

window_draw_list.rs: Wrap add_triangle

8471bd4

window_draw_list.rs: Wrap add_bezier_curve

35db5fc

ui: Wrap dummy

a60465c

ui: Wrap invisible_button

c58a93b

ui: Wrap get_content_region_avail

738a9c9

test_window_impl: show_example_app_custom_rendering: Include drawing …

c277488

…canvas Include drawing canvas example into show_example_app_custom_rendering. The example contains now everything included in the original C++ example provided with dear imgui.

test_drawing_channels_split: Add an example for channels_split

2312f41

As channels_split may be difficult to understand, this commit adds a simple example of its usage.

Add missing imports in example code

664660a

ImMouseButton: Derive Copy, Clone, Eq, PartialEq, Debug

43a2484

For convenience.

ImColor: Derive Eq, PartialEq, Debug

746bdc5

window_draw_list: Fix typo in doc

7be9f15

DrawAPI: Remove unused lifetime

3aa807b

WindowDrawList: Cannot use WindowDrawList::new outside of crate

9a65a64

malikolivier added 5 commits April 16, 2018 15:14

ui: Rename to with_window_draw_list ot get_window_draw_list

95577a0

ChannelsSplit: Panic when channel index overflows

29b0d20

imgui: Make ChannelsSplit struct visible outside the crate

734292f

window_draw_list: Fix awkward wording in doc

082d5e4

malikolivier force-pushed the draw-api branch from 586e102 to 454e980 Compare April 16, 2018 06:24

malikolivier added 2 commits April 24, 2018 13:23

ChannelsSplit: Rename channels_set_current to set_current

9a9484f

Fancier API

Gekkio merged commit 544d7de into imgui-rs:master Apr 29, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Wraps the dear ImGui custom drawing API #111

Wraps the dear ImGui custom drawing API #111

malikolivier commented Mar 30, 2018 •

edited

malikolivier commented Mar 30, 2018

Lymia commented Apr 7, 2018 •

edited

malikolivier commented Apr 8, 2018

Gekkio left a comment

Gekkio Apr 12, 2018

Gekkio Apr 12, 2018

Gekkio Apr 12, 2018

Gekkio Apr 12, 2018

malikolivier Apr 15, 2018

Gekkio Apr 12, 2018

malikolivier Apr 15, 2018

Gekkio Apr 22, 2018 •

edited

malikolivier Apr 22, 2018

Gekkio Apr 12, 2018

Gekkio Apr 12, 2018

malikolivier Apr 15, 2018

Gekkio Apr 12, 2018

malikolivier Apr 15, 2018

malikolivier commented Apr 15, 2018

malikolivier commented Apr 16, 2018

Gekkio commented Apr 16, 2018

malikolivier commented Apr 16, 2018

Gekkio commented Apr 16, 2018

malikolivier commented Apr 16, 2018

Lymia commented Apr 22, 2018

Gekkio commented Apr 22, 2018 •

edited

malikolivier commented Apr 22, 2018

Gekkio commented Apr 22, 2018

malikolivier commented Apr 22, 2018 •

edited

malikolivier commented Apr 24, 2018 •

edited

parasyte commented Apr 27, 2018

Gekkio commented Apr 29, 2018

Wraps the dear ImGui custom drawing API #111

Wraps the dear ImGui custom drawing API #111

Conversation

malikolivier commented Mar 30, 2018 • edited

malikolivier commented Mar 30, 2018

Lymia commented Apr 7, 2018 • edited

malikolivier commented Apr 8, 2018

Gekkio left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Gekkio Apr 22, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

malikolivier commented Apr 15, 2018

malikolivier commented Apr 16, 2018

Gekkio commented Apr 16, 2018

malikolivier commented Apr 16, 2018

Gekkio commented Apr 16, 2018

malikolivier commented Apr 16, 2018

Lymia commented Apr 22, 2018

Gekkio commented Apr 22, 2018 • edited

malikolivier commented Apr 22, 2018

Gekkio commented Apr 22, 2018

malikolivier commented Apr 22, 2018 • edited

malikolivier commented Apr 24, 2018 • edited

parasyte commented Apr 27, 2018

Gekkio commented Apr 29, 2018

malikolivier commented Mar 30, 2018 •

edited

Lymia commented Apr 7, 2018 •

edited

Gekkio Apr 22, 2018 •

edited

Gekkio commented Apr 22, 2018 •

edited

malikolivier commented Apr 22, 2018 •

edited

malikolivier commented Apr 24, 2018 •

edited