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

Add callbacks to the draw_list. #702

Merged
merged 3 commits into from Apr 5, 2023
Merged

Add callbacks to the draw_list. #702

merged 3 commits into from Apr 5, 2023

Conversation

rodrigorc
Copy link
Contributor

This PR adds the possibility to add callbacks to the draw_lists. Those were handled by the glow/glium backends, but we would have to add them using unsafe code and raw C callbacks.

    let draws = ui.get_window_draw_list();
    draws.add_callback(|| { do_something(); }).build();

Currently the callback must implement FnOnce() + 'static, that is the safest bet and enough for my needs.

Once the callback is run, I'm setting it to NULL, even though it is a const C pointer to avoid the risk of calling it twice, although I think that is not really needed, because an imgui draw-list can only be called once. But just in case...

Also, if the callback is added to the draw-list but then the draw-list is never run, the callback will leak, as DearImGui has no provision of a unused draw-list cleanup.

imgui/src/draw_list.rs Outdated Show resolved Hide resolved
imgui/src/draw_list.rs Outdated Show resolved Hide resolved
@dbr
Copy link
Contributor

dbr commented Jan 20, 2023

This looks good, thanks for the PR!

Added a couple of minor notes on the doc strings - I also wonder, would it be much work to add a simple example to imgui-examples/ showing how this might be used in practice?

@rodrigorc rodrigorc force-pushed the AddCallback branch 2 times, most recently from e5eab6a to 7969d1b Compare January 20, 2023 14:06
@rodrigorc
Copy link
Contributor Author

would it be much work to add a simple example to imgui-examples/ showing how this might be used in practice?

Ah, examples are tricky because they are usually used to do random OpenGL stuff that happens to be quite specific. Indeed, even DearImGui does not provide any callback example, that I know of, it just comments about using it for a render-to-texture pass and so on...

Anyway, I'm adding a commit with an example to the imgui-glow-renderer subdirectory that uses the callback to render a FramebufferObject, not so different from what I'm doing in my real project. I had to wrap the AutoRenderer::gl in an Rc because of the 'static requirements of the callback. I cannot wrap the full AutoRender into an because that is mutably borrowed when calling AutoRenderer::render().

Feel free to suggest any other way to change that and still get access to the GL context from the callback.

On a footnote, I'm quite sure that it is technically possible to get rid of that pesky 'static in the callback. But that would probably require to wrap all the UI building in a scope similar to std::thread::scope, and it is probably not worth it.

@rodrigorc
Copy link
Contributor Author

Hey, @dbr could you check my latest commits to this PR?

@dbr
Copy link
Contributor

dbr commented Mar 22, 2023

Oh sorry for missing this - I think the change to the renderer needed are fine

I fixed up conflict, shall merge when CI is happy (I think it might fail due to unrelated changes of MSRV from an upstream library - shall fix that first in main if so)

@rodrigorc
Copy link
Contributor Author

With the merge of the new glow version the new example in the PR broke. I've just pushed a fix.

@dbr: After the latest merges the git-log of this PR is a bit messy. I don't know the policy of this project, but if you like I can do a rebase/squash, before an eventual final merge?

@dbr
Copy link
Contributor

dbr commented Apr 4, 2023

Ahh good, was worried I had botched something in the merge.

Yep a rebase sounds like a good idea!

@rodrigorc
Copy link
Contributor Author

Ok, I have just rebased this. I've split the Rc<glow::Context> in AutoRenderer change from the example because I happened to need that in a project of mine, so I can just cherry-pick it from this PR 🍒.

@dbr
Copy link
Contributor

dbr commented Apr 5, 2023

Perfect, thanks!

@dbr dbr merged commit d4a1e38 into imgui-rs:main Apr 5, 2023
8 checks passed
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.

None yet

2 participants