-
Notifications
You must be signed in to change notification settings - Fork 359
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
glow renderer #495
glow renderer #495
Conversation
This is great! Experimented with shoving this in an application using imgui quite heavily, and this all worked well - on Linux with Intel integrated card, and with a fork of imgui which including support for the docking branch Two minor comments:
|
..however this application on Windows (nVidia card) seems to perform very poorly with the Not too sure what is the problem (it very well may be caused by something unrelated), I can maybe look into this further.. unless someone more experienced with profiling on Windows can test and see if they get the same result with a different code base 🤞 |
} | ||
|
||
let ui = imgui_context.frame(); | ||
// Safety: internally, this reference just gets passed as a |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is completely incorrect. It's still UB to have a null reference.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fair. I'm assuming this means there's no way to do the equivalent of pass NULL
here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, you can make it take an option and pass None
, since Option<&T>
/Option<&mut T>
is guaranteed to have the same representation as a nullable pointer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I haven't really looked to see if that answer makes sense. I'll be able to do a more thorough review later.
On a skim it looks okay. Please fix the lint and I'll take a closer look in a week or two when my schedule frees up a bit more (sorry for delays). |
This was actually intentional, I was trying to keep
Done |
I had to use pub mod utils; in examples to prevent spurious dead code warnings. I'm also getting lint errors from |
Still not going to have time to really dig through for another week (it's a lot of unsafe code so I don't really want to halfass the review), but just a couple notes:
It's probably find to do
Should be fixed if you rebase. |
Took a look through it -- I'll verify most of the calls, but to start with, you're using glDebugMessageInsert fairly extensively -- I think that's fine, just we should have a feature gate around that, as macOS doesn't support it, since it's locked still to 3.3. Working through it still generally though |
Hiya, I have quite a lot of thoughts looking over this. In general, I think your doc comments are correct: this is over engineered in its current form, but also is a promising start. I haven't verified the OpenGL itself in depth yet, but looking over it, it looks very fine and runs on a variety of devices without issue, so although we should vet it more thoroughly, I'm not too worried about it. Here's what I'd like to change:
I'd like to see some of these changes discussed first and then let's walk through the rest. Overall, this looks like it will be an EXCELLENT addition to the project. Thanks so much for the work! |
Obviously it's always good to manually check, but in case it wasn't obvious the vast majority of the OpenGL logic was ripped straight from the extremely well-tested upstream example, filtered through my own understanding of what's going on. Hopefully that can ease your analysis burden a bit.
Haha, looks like I'm going to be mostly reverting commits to before I got carried away 😆
These are all fair and I'll make these changes. FWIW though, my idea was to provide a solution that was both plug-and-play for people who want to get going quickly (
Good point. Laziness on my part in copying from upstream (though also one of those use-cases for the Do you have thoughts on getting rid of the generic over |
I think I've addressed all of your comments.
|
I had a rethink of sRGB support. I now no longer assume you use Now if using The |
@thomcc can you take a check over this? I spent the day working on a similar code area, and looking over this PR. I think it is acceptable in its current form! Sorry for the large delay, but with the recent edits, I'm comfortable taking it |
Hey, I'll try to get to it in two weeks. Unfortunately, at the moment I'm in the middle of something of a vacation, and non-relaxation time is being taken up with a series of job interviews (... I know how to schedule things well 😅). So I don't have a ton of time for open source until at least one of those is done, in two weeks. Sorry for the delay though 🙇 |
hey @jmaargh what's the status of the glow-renderer linter errors? I'd love to accept this if those errors are spurious (and they look spurious) |
@sanbox-irl I wasn't seeing any linter errors from CI, but a few spurious-looking test failures. I've merged master back in and will fix any errors when CI comes back |
@dbr sorry for the hilariously late response, but using this on a nvidia windows laptop (a pretty rough gpu -- mx150), i'm not getting any notable stutters |
@jmaargh Okay, I've done a final review, and would like to propose one final change, and additionally note further work to be done in, presumably, glow 0.12.
Currently, however, there's no generic way to get the backing texture for the GlStateBackup. But of course, imgui theoretically has support for webgl, but in reality, we fail to compile anyway, so let's not lose our sleep over that today. I'll make a tracking issue, however, since getting wasm support should be useful to us.
For now, can you review the commit I'll push, make sure everything is okay with you, and then ping me back? At that point, I'll accept the PR -- I'm anxious to have it in the project, even if it will require some minor updates to get to 0.11/0.12 glow in the future! (hit the close button accidentally! Sorry about that) |
Took a non-trivial amount of time to write up the functions we'd need to either fully wrap around |
@sanbox-irl Thanks for the thorough review. Your commit looks fine to me. I just pushed another tidying the fully-qualified syntax with some type aliases, feel, free to revert 404841f if you'd prefer the more explicit syntax. I'm happy for this to be committed either way. Interesting to hear about the WebGL complications. I'm afraid I don' t now enough about WebGL right now to opine, but your suggestions for the future look sensible to me. |
didn't actually know you could type alias that! When the CI goes green, I'll add a thanks to you in the CHANGELOG and then merge it in. Thank you for all the work! It is so appreciated -- stuff like this is required to keep the library alive. |
Previous `prepare_font_atlas` silently assumed a TrivialTextureMap. Also adds an impl of `TextureMap` for `imgui::Textures` as a further example.
Instead use `GlStateBackup` every time
Instead, always use the previous AutoShaderProvider
There are now only two renderers (only generic over the GL context) - `AutoRenderer` which is both the old `OwningRenderer` and also sets up a `SimpleTextureMap` automatically - `Renderer` which borrows both the GL context and the texture map on every call to Render This means the `RendererBuilder` can be entirely removed. Also `TrivialTextureMap` renamed to `SimpleTextureMap`.
Fix sRGB support, now when initialising a Renderer you can explicitly choose whether to output colors in linear or sRGB color spaces. Fix examples to show how to render these properly. Fix comments in examples
Ready for merge
404841f
to
7d06d42
Compare
thank you @jmaargh ! |
This is great! imgui-glow-renderer is not published on crates.io (yet). Could you do it when you have time? |
@toyboot4e my plan was to publish when 0.8 goes live! |
@toyboot4e should now be published in 0.8.0 |
Apologies if this is a bit over-complicated, it got away from me a bit. However, the vast majority of the OpenGL logic is based on the upstream example, so I hope it is mostly correct.
Where it differs are in three traits (
ContextStateManager
,ShaderProvider
, andTextureMap
) which users can implement to customise how the renderer behaves. It also supports either owning the OpenGL context, or borrowing it when needed. But, as the01_basic.rs
example shows, basic usage remains simple.Hopefully the examples and code are relatively self-explanatory. I started writing docs, but then I got a compiler crash while running
cargo doc
, so I just sort of stopped.Happy to change/fix as needed. Any testing would be great -- I've only tested on Linux with nvidia drivers.