glow renderer

[jmaargh]

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, and TextureMap) 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 the 01_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.

[dbr]

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:

1. In the 01_basic.rs example, there isn't a call to imgui_context.io_mut().update_delta_time(...) so imgui incorrectly reports the framerate as a constant 60.0 FPS
2. It would be nice to have an example of a suggested approach to implementing custom textures, https://github.com/imgui-rs/imgui-rs/blob/918f79780d9ec1d36774dd19003da869124ec643/imgui-examples/examples/custom_textures.rs

[dbr]

Experimented with shoving this in an application using imgui quite heavily, and this all worked well - on Linux with Intel integrated card

..however this application on Windows (nVidia card) seems to perform very poorly with the imgui-glow-renderer (very stuttery frame rate even with a fairly simple interface)

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 🤞

[thomcc]

This is completely incorrect. It's still UB to have a null reference.

[jmaargh]

Fair. I'm assuming this means there's no way to do the equivalent of pass NULL here?

[thomcc]

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.

[thomcc]

Sorry, I haven't really looked to see if that answer makes sense. I'll be able to do a more thorough review later.

[thomcc]

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).

[jmaargh]

1. In the `01_basic.rs` example, there isn't a call to `imgui_context.io_mut().update_delta_time(...)` so imgui incorrectly reports the framerate as a constant `60.0` FPS

This was actually intentional, I was trying to keep 01_basic.rs both minimal and self-contained. In any case, I've added it now.

2. It would be nice to have an example of a suggested approach to implementing custom textures, https://github.com/imgui-rs/imgui-rs/blob/918f79780d9ec1d36774dd19003da869124ec643/imgui-examples/examples/custom_textures.rs

Done

[jmaargh]

I had to use

pub mod utils;

in examples to prevent spurious dead code warnings. I'm also getting lint errors from imgui/src/utils.rs which are not related to this PR.

[thomcc]

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:

in examples to prevent spurious dead code warnings.

It's probably find to do #![allow(dead_code)] in examples. The issue is each example is compiled as its own crate.

I'm also getting lint errors from imgui/src/utils.rs which are not related to this PR.

Should be fixed if you rebase.

[sanbox-irl]

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

[sanbox-irl]

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:

1. I'd like to remove the ShaderManager trait. It doesn't serve the purpose you have for it right now -- since it requires the related struct GenericShaderData, which has prebaked notions of uniforms and vert poses, I see no reason to not just bake the AutoShaderProvider. I don't see the scenario where someone is running a custom shader to be particularly likely to occur.
2. We should alter the texture manager. I think the trait is a fine idea, but let's invert this so that it's borrowed, just like the BorrowedRenderer, and ask that users pass in their texture manager into the render function call. Most users, I expect, will actually want to own their texture manager -- especially since ImGui is used to make Editors for game engines where textures might be created and destroyed at runtime and loaded by some asset system, it seems easier for MOST users to just give their own texture manager over. The OwnedRenderer can maintain a simpler manager -- though even in that case, I think we should remove the TrivialTextureManager entirely -- uploading other sprites is just too common.
3. We should remove the "TrivialContextManager" and simply use the "StateBackupCsm". The worst case scenario is that someone is doing unnecessary OpenGL work, and well, that's the beauty of OpenGL, isn't it. I don't think it's worth our own complexity for trivial speed gains for our most basic users -- those who are okay with giving up their entire renderer (the only category of people who would be good with the TrivialContextManager) are not going to be concerned with a minuscule amount of extra work being done.
4. If 1 and 2 and 3 are implemented, then we should be able to remove all of the generics from the Renderer, which will make it mUCH more approachable to users.
5. Let's talk about sRGB as well -- these shaders currently are just RGB, which can result in blending problems. Imgui-wgpu handles this with a #define which I think we should emulate here too: https://github.com/Yatekii/imgui-wgpu-rs/blob/master/src/imgui.frag

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!

[jmaargh]

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.

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.

Here's what I'd like to change:

Haha, looks like I'm going to be mostly reverting commits to before I got carried away 😆

4. If 1 and 2  and 3 are implemented, then we should be able to remove all of the generics from the Renderer, which will make it mUCH more approachable to users.

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 (auto_renderer) and a bit more flexible in case you wanted to customise (via the T, S, and C generics -- I can think of use cases for each, but you're right that they're probably all fairly niche). Of course, if you're going to integrate into a full engine in earnest you're probably writing your own renderer, but I was hoping to cover a little of the middle ground too.

5. Let's talk about sRGB as well -- these shaders currently are just RGB, which can result in blending problems. Imgui-wgpu handles this with a #define which I think we should emulate here too: https://github.com/Yatekii/imgui-wgpu-rs/blob/master/src/imgui.frag

Good point. Laziness on my part in copying from upstream (though also one of those use-cases for the ShaderProvider).

Do you have thoughts on getting rid of the generic over G: Gl throughout? I'm honestly not sure why glow but all of the methods in the HasContext trait, which is annoyingly generic over a bunch of types which are defined as specific integer types in the OpenGL standard. I can't think of anything we'd lose by just taking glow::Context.

[jmaargh]

I think I've addressed all of your comments.

1. Merged master in to fix lints
2. Feature-gated and version-checked glDebugMessageInsert
3. Removed ContextStateManager, replaced with GlStateBackup which is the old StateBackupCsm
4. Removed ShaderProvider, replaced with Shaders derived from the old AutoShaderProvider
5. Consolidated to two renderers: AutoRenderer and Renderer. The latter borrows the GL context and the TextureMap every time it is needed. The former AutoRenderer owns the GL context and creates a SimpleTextureMap (where the imgui texture ID == the OpenGL texture ID)
6. Put in handling for sRGB output properly. imgui outputs vertex colours in sRGB space, so these are converted to linear in the vertex shader before interpolation. imgui font textures and the lenna texture are both sRGB, so the correct OpenGL enums are now applied for the internal format. For display, I assume that either (a) you've set your framebuffer to SRGB (e.g. with glEnable(GL_FRAMEBUFFER_SRGB) if you're outputting to a display or (b) you're outputting to a texture and therefore colours should be left as linear -- in either case OpenGL should just handle it. GLES doesn't support this, so right now I just convert back to sRGB in the fragment shader (I should probably have a way to configure whether you want that to happen).

[jmaargh]

I had a rethink of sRGB support. I now no longer assume you use GL_FRAMEBUFFER_SRGB and don't switch based on whether it's GLES or not.

Now if using Renderer you can choose whether the shader will output linear colours or sRGB colours. The examples have been updated to show how to use this.

The AutoRenderer just assumes that it needs to convert to sRGB.

[sanbox-irl]

@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

[thomcc]

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 🙇

[sanbox-irl]

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)

[jmaargh]

@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

[sanbox-irl]

@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

[sanbox-irl]

@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.

1. I'd like to say that this PR is excellent, and thank you for spending the effort, and apologies that thom and I have both had a busy year and not given it the proper review it needed. I have now though, so I think we can accept it.

2. This PR uses a fairly large trait abstraction, but I don't think it does the work we need it to do. Ideally, we should be generic over G: glow::HasContext but this creates problems related to the GlStateBackup -- specifically, on WebGL, according to groves (I sent them a message about this PR):

. On WebGL we get opaque handles like WebGlTexture which are a bit difficult to work with. So internally in glow we store these in something like Map<i32, WebGlTexture> so we can use WebGL textures just like OpenGL handles (e.g. just copying them around, comparing them, storing them without worrying about lifetimes)
...
on OpenGL you'd get an i32 back, but on WebGL you'd get a WebGlTexture back (possibly a value in the map, but not necessarily if it wasn't created by glow)

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.

3. We don't need this large trait abstraction as long as we use fully qualified syntax. I will in a moment push a commit which replaces the trait with this fully qualified syntax, which should remove the need for generics. Since on every platform, glow only exposes one backend possible (ie, native on native and webgl on wasm), we do not lose any flexbility that we had.

4. Ideally, we'd be on glow 0.11, which has just come out. However, as has been noted in 2 above, we actually have a problem with our GlStateBackup with the abstraction of glow::HasContext. That prevents us from absracting over the HasContext in 0.10, and it prevents us from updating at all to 0.11 (this is somewhat on purpose -- apparently if we COULD abstract over HasContext fully in 0.10, we'd not actually compile on wasm). I'll be opening an issue on glow to fix this problem.

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)

[sanbox-irl]

Took a non-trivial amount of time to write up the functions we'd need to either fully wrap around HasContext and to update to 0.11 (which will require us to wrap around HasContext at that point, or at least might as well do so) -- grovesNL/glow#187

[jmaargh]

@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.

[sanbox-irl]

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.

[sanbox-irl]

thank you @jmaargh !

[toyboot4e]

This is great!

imgui-glow-renderer is not published on crates.io (yet). Could you do it when you have time?

[sanbox-irl]

@toyboot4e my plan was to publish when 0.8 goes live!

[jmaargh]

@toyboot4e should now be published in 0.8.0