Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement get_*_draw_list and DrawList drawing functions #242

Closed
wants to merge 2 commits into from

Conversation

tangmi
Copy link
Contributor

@tangmi tangmi commented Jul 23, 2019

Closes #220

  • Merge 1imgui::render::DrawList1 with 1imgui::WindowDrawList1
  • Rename WindowDrawList to DrawList and make [Window|Foreground|Background]DrawList into a wrapper for DrawList
  • Add 'ui lifetime to DrawData and *DrawList
  • Make all DrawList methods take &mut self instead of &self

Breaks DrawData/DrawList API?

  • Rename window_draw_list to draw_list, but for now, this keeps the diff sane.
  • Expose DrawList, ImColor publicly
  • Path API?

- Rename WindowDrawList to DrawList and make [Window|Foreground|Background]DrawList into a wrapper for DrawList
- Add 'ui lifetime to DrawData and *DrawList
- Make all DrawList methods take &mut self instead of &self
src/render/draw_data.rs Outdated Show resolved Hide resolved
@tangmi
Copy link
Contributor Author

tangmi commented Jul 23, 2019

@Gekkio I believe this PR should be a safe implementation of the drawlist apis.

Notably:

  • All DrawList functions take &mut self, as they mutate the underlying draw list
  • DrawList is bound to 'ui now
  • add_circle, etc return a builder with a mutable reference to the DrawList with a new lifetime pub fn add_circle<'a, ...>(&'a mut self, ...) -> Circle<'ui, 'a>

Edit: looks like methods like with_clip_rect and channels_split have borrow issues, as they require &mut DrawList, but also take a delegate that requires &mut DrawList as well. A workaround is forcing people to call Push and Pop individually, but that doesn't follow the "safe"-ness pattern of this library. Another option would be to pass the &mut DrawList to the delegate as a parameter, but that may be confusing to callers seeing a borrow of a DrawList they otherwise already have. I could also make a type that is a ChannelSplitToken-like thing that DerefMuts into DrawList, so people could do something like:

let draw_list = ...;
{
    let draw_list_channels = draw_list.channel_split(2);
    draw_list_channels.add_line(...);
    draw_list_channels.set_current(0);
    ....
    // channel split is popped here
}

thoughts?

@saucesaft
Copy link

What can be done to implement this functions into imgui-rs? Anything i can help with?

@tangmi
Copy link
Contributor Author

tangmi commented Jan 31, 2020

@saucesaft Aside from the merge conflict and failing CI checks, the bit in my last comment about lifetimes regarding with_clip_rect and channels_split I think is still true. Another thing I recall when using this branch is that while there is WindowDrawList, ForegroundDrawList, and BackgroundDrawList, there is no easy way to write code abstracted across them (this might be as simple as making DrawList pub.

I think @Gekkio is pretty busy (I am too!) and I believe neither of us have looked at this since the summer! Feel free to clean this up and re-propose the changes, I think some test cases that sanity check the API would catch things like in the second half of my last comment.

@Boscop
Copy link
Contributor

Boscop commented Sep 22, 2020

Any update on this? :)

In my use case I need access to the drawlist..

@tangmi
Copy link
Contributor Author

tangmi commented Feb 15, 2021

Closing, as I believe this was addressed in #414!

@tangmi tangmi closed this Feb 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

GetForegroundDrawList and GetBackgroundDrawList not exposed (upgrade Dear Imgui to 1.69)
3 participants