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

Drawlists callbacks questions with OpenGL #7665

Open
astotrzynascie opened this issue Jun 6, 2024 · 8 comments
Open

Drawlists callbacks questions with OpenGL #7665

astotrzynascie opened this issue Jun 6, 2024 · 8 comments

Comments

@astotrzynascie
Copy link

astotrzynascie commented Jun 6, 2024

Version/Branch of Dear ImGui:

Version 1.90.3, Branch: master

Back-ends:

imgui_impl_sdl2.cpp + imgui_impl_opengl3.cpp

Compiler, OS:

Windows 10 + Clang-cl 18.1.6

Full config/build information:

No response

Details:

Hello.

I'm using drawlists callbacks the usual(?) way:

if (ImGui::Begin("test", NULL, flags)					// flags can make a difference
	{
	ImDrawList *draw_list { ImGui::GetWindowDrawList() };		// or GetBackgroundDrawList()
	draw_list->PushClipRect(position_min, position_max);		// or PushClipRectFullScreen();
	draw_list->AddDrawCmd();
	draw_list->PopClipRect();

	TCallbackData *callback_data = new TCallbackData(...);

	draw_list->AddCallback(callback_function, callback_data);
	draw_list->AddCallback(ImDrawCallback_ResetRenderState, nullptr);
	}
ImGui::End();

In callback_function I juggle textures, bind to units, use glsl programs, the whole zoo is involved.
That's why I add ImDrawCallback_ResetRenderState.

At first I didn't add AddDrawCmd(); to redraw the rectangle that I will be processing in callback,
but I noticed that I have to do it in more and more cases.

First question: is there any clue about when I should or shouldn't refresh / force redraw this part of image?

Second question is about different draw lists in this context, maybe this will help me understand better the ImGui flow.

If I have a single callback in the frame (or all callbacks are in the same draw lists) skipping redraw (AddDrawCmd) results in messed up rectangle(s) that was processed in the callback(s). And now the interesting observation which I mention because it was quite hard to debug/trace it: if callbacks are from different draw lists (GetWindowDrawList/GetBackgroundDrawList) and one is lacking the redraw, then weird and terrible things are happening. In particular, the fonts looks like someone throw a grenade into textures store (font atlas gets textures from random locations?). The whole ImGui goes loco.
Additionally, changing window properties, like making window invisible (no background, titlebar, borders) causes the mess looks slightly different.

Is there a short explanation for this?

Screenshots/Video:

No response

Minimal, Complete and Verifiable Example code:

No response

@GamingMinds-DanielC
Copy link
Contributor

AddDrawCmd() does not issue any kind of redraw, you should see it more as a pipeline flush. It prevents submitted primitives from before and after the call being merged into the same command. State changes like pushing or popping a clip rect will flush internally, there should be no need to explicitly call this function there. Adding a callback will also call it internally. If explicitly calling or not calling AddDrawCmd() makes a tangible difference, that is an indicator that something is wrong with your callbacks regardless of the call, but the problem may manifest in different ways.

You can process geometry in callbacks, but you shouldn't process geometry that you submitted to ImGui. In that case, you should use the callback to just set up render state, then submit the geometry to ImGui, then add another callback to reset or restore the render state to something that is expected. If you forget to do that, changed render state will affect everything that comes after, even other draw lists that are executed afterwards. So if you are not careful, you can mess up all rendering down the line, and you don't have direct control over the order in which draw lists are executed. But they are executed sequentially, so basically you just need to make sure to fix the render state before the end of the draw list. This is not a bug of the library, just an advanced feature that puts the responsibility of proper use on the user.

If you need to identify your callbacks, you can put identifying information into the user data pointer you are prividing to your callback. If you want to analyze draw calls and render state to get a better understanding and see where it goes wrong, RenderDoc is a great tool to do so.

@astotrzynascie
Copy link
Author

astotrzynascie commented Jun 7, 2024

Thank you for the response.

AddDrawCmd() does not issue any kind of redraw (...)

Ok, got it. For a test I commented out the code and in one project everything looks broken and in second only if I change window draw list to background. Looks like I need to rework the callbacks. I'm not sure what you mean by 'submitting geometry to ImGui', at the moment callbacks look more or less like this:

void callback_example(const ImDrawList *_parent_list, const ImDrawCmd *_cmd)
	{
	// one VAO for the project
	glBindVertexArray(gl::VAO);

	// glActiveTexture & glBindTexture, the texture I use to render ImGui on
	// (bind it's framebuffer before ImGui::Render())
	rebind_texture(ETexture::imgui);  

	TCallbackData *callback_data = static_cast<TCallbackData*>(_cmd->UserCallbackData);

	ImVec2 &position_min = callback_data->position_min;
	ImVec2 &position_max = callback_data->position_max;

	some_shader.use();

	bind_framebuffer(ETexture::imgui);

	set_some_shader_stuff();
	
	// gl::vertices_set/reset is used set the vertices to draw only position_min - position_max
	// at the end of these are (only one VBO at the project):
	// glBindBuffer(GL_ARRAY_BUFFER, VBO);
	// glBufferData(GL_ARRAY_BUFFER, sizeof(vertices), vertices, GL_STATIC_DRAW);

	gl::vertices_set(position_min.x, position_min.y, position_max.x, position_max.y);
	glDrawElements(GL_TRIANGLES, 6, GL_UNSIGNED_INT, 0);
	generate_mipmap(ETexture::imgui);
	gl::vertices_reset();

	delete callback_data;
	}

I suspect I shouldn't use something (glDrawElements?) or render to a completely different texture and after the callback use draw_list->AddImage or something. In the later case, is it ok (also performance-wise) to create/delete textures for each window's callback every frame or just make one and use it for all callbacks and just manipulate corners?
Regarding AddImage, if the texture is premultiplied, is it ok to use glBlendFunc before/after?

Oh, and in some callbacks I use more shaders, giving some ETexture::imgui as an additional texture (for blurring the background for example). In these cases I render to a helper texture of course and then render it back on ETexture::imgui.

Adding ImDrawCallback_ResetRenderState after each callback.

Any hints appreciated.

@GamingMinds-DanielC
Copy link
Contributor

I'm not sure what you mean by 'submitting geometry to ImGui', at the moment callbacks look more or less like this:

What I meant was using a callback to set up shaders and other state without drawing anything, then calling other drawing functions like f.e. AddRectFilled() before finally cleaning up with another callback. That way you can submit geometry to ImGui that is rendered with the shaders you supplied. But that doesn't apply to the code you just posted.

So now you need to find out what state is wrong, maybe ImDrawCallback_ResetRenderState doesn't handle every eventuality. To do that I recommend RenderDoc, that tool is made for rendering analysis like this. Draw something after your callback/reset combo and replace these callbacks with a single AddDrawCmd() to make sure the following draw doesn't get merged into the previous one. Make captures with your callback and with the inhibited merge, then compare states between those versions. There you will see what state is different than it should be.

@astotrzynascie
Copy link
Author

astotrzynascie commented Jun 8, 2024

I prepared the data for examination:

First callback:

	ImDrawList *draw_list { ImGui::GetWindowDrawList() };
	TCallback1Data *callback_1_data = new TCallback1Data(...);
	draw_list->AddCallback(callback_1_function, callback_1_data);
	draw_list->AddCallback(ImDrawCallback_ResetRenderState, nullptr);

Second callback

	ImDrawList *draw_list { ImGui::GetBackgroundDrawList() };
	
	draw_list->AddDrawCmd(); // commented out in 'broken' version
	
	TCallback2Data *callback_2_data = new TCallback2Data(...);
	draw_list->AddCallback(callback_2_function, callback_2_data);
	draw_list->AddCallback(ImDrawCallback_ResetRenderState, nullptr);

RenderDoc showed me:
RenderDoc_difference
I checked, only difference is in these two positions (glScissors full screen and glBindTexture), the rest is continued in next position.
Number 204 didn't tell me much, was empty in texture inspector and had dimensions 2048x4096, like none of my textures.
So 204 must be an ImGui texture.
To be sure, I debugged draw_list->AddDrawCmd();, it led me to

	void ImDrawList::AddDrawCmd()
	{
	    ImDrawCmd draw_cmd;
	    draw_cmd.ClipRect = _CmdHeader.ClipRect;    // Same as calling ImDrawCmd_HeaderCopy()
	    draw_cmd.TextureId = _CmdHeader.TextureId;
	    draw_cmd.VtxOffset = _CmdHeader.VtxOffset;
	    draw_cmd.IdxOffset = IdxBuffer.Size;

	    IM_ASSERT(draw_cmd.ClipRect.x <= draw_cmd.ClipRect.z && draw_cmd.ClipRect.y <= draw_cmd.ClipRect.w);
	    CmdBuffer.push_back(draw_cmd);
	}

...and TextureId was number 9.
So I checked the whole project for glGenTextures and found it in imgui_impl_opengl3.cpp:
GL_CALL(glGenTextures(1, &bd->FontTexture));
when debugged, it showed also 9.
Anyways, there aren't many textures as I cut down the project to the working minimum with two callbacks.

So, it looks like font texture wasn't bound anywhere(?).
I'm always rebinding textures with

		glActiveTexture(GL_TEXTURE0 + _texture.unit);
		glBindTexture(GL_TEXTURE_2D, _texture.tex);

but when I looked up for glActiveTexture in ImGui I found only:

void ImGui_ImplOpenGL3_RenderDrawData(ImDrawData* draw_data)
{
(...)
// Backup GL state
GLenum last_active_texture; glGetIntegerv(GL_ACTIVE_TEXTURE, (GLint*)&last_active_texture);
glActiveTexture(GL_TEXTURE0);
(...)
    // Restore modified GL state
(...)
#ifdef IMGUI_IMPL_OPENGL_MAY_HAVE_BIND_SAMPLER
    if (bd->GlVersion >= 330 || bd->GlProfileIsES3)
        glBindSampler(0, last_sampler);
#endif
    glActiveTexture(last_active_texture);

At this point, I'm a little confused, should I assume that ImGui is using GL_TEXTURE0 ('+0') unit only?
I'm not using it at all, my textures start from GL_TEXTURE0 + 1.

So, somehow just glBindTexture is enough, without setting glActiveTexture first ? Looks risky, what if someone just set and used glActiveTexture for his texture and this bind will overwrite it? Someone should set glActiveTexture on unused unit just for ImGui after any operations?
Edit: that was a little hasty

And why this binding (if it was bound to GL_TEXTURE0 + 0) was no more?

As an extra exercise I set ImGui::GetWindowDrawList() in both callbacks (so it's also fine, everything looks ok, especially fonts).
In this case RenderDoc reported only one Colour Pass completely skipping glDrawElements, glGenerateTextureMipmap and glDrawElementsBaseVertex.

It's my second OpenGL project, first one with ImGui and I really appreciate the help.

The only additional info in Errors and Warnings in RenderDoc were API Info Miscellaneous (...) Buffer detailed info: Buffer object (...) will use VIDEO memory as the source for buffer object operations., so I assume it's fine since it has Severity set to info.

@astotrzynascie
Copy link
Author

I just did an extra check and in 'full' project I removed all the

	draw_list->PushClipRect(..);
	draw_list->AddDrawCmd();
	draw_list->PopClipRect();

lines and checked that callbacks use the same drawlist (ImGui::GetWindowDrawList()).
Results are very similar, same broken font.
So, in 'reduced' project I need to switch draw list in one callback to break fonts, and in 'full' project all callbacks are using the same lists and the font is broken anyway (unless I add draw_list->AddDrawCmd();).

@astotrzynascie
Copy link
Author

astotrzynascie commented Jun 8, 2024

If, instead of draw_list->AddDrawCmd(); I put
glBindTexture(GL_TEXTURE_2D, (GLuint)(intptr_t)ImGui::GetIO().Fonts->TexID);
then fonts look correct.
Still, the question is, what breaks it?

Edit: there is just one glActiveTexture (with GL_TEXTURE0, at the beginning of frame/1st color pass) before the 'cutting point', no suspicious texture binding, or something

Edit2: After analyzing all 3 saved cases, it may look like sometimes ImGui is adding glScissors + glBindTexture(GL_TEXTURE_2D, Texture_with_font) before glDrawElementsBaseVertex(number_other_than_font_id) and sometimes not. Sometimes it's just the separated group with glDrawElementsBaseVertex(font_id_number). My experience is still too low to say anything more.

@astotrzynascie
Copy link
Author

astotrzynascie commented Jun 13, 2024

Investigation, part 2.

The case can be narrowed to one callback.
Callback should be added to background drawlist.
This list is in ImGui_ImplOpenGL3_RenderDrawData drawed first, it may be the difference.
The foreground and window list callbacks don't spoil rendering fonts.

If we take a ImGui-OpenGL project containing only one window with one callback,
then we get situation in which in the ImGui_ImplOpenGL3_RenderDrawData loop
all but one draw list have CmdBuffer.Size == 1.
If this list with callback is background or foreground, then it's buffer has 3 commands:
callback, reset, and 'finishing command' scissors, bind fonts texture, draw.
If this list is the window's one, then we got 4 commands, with the same finishing command
at the very beginning (because of PushClipRect(window->InnerClipRect.Min, window->InnerClipRect.Max, true);
line in window begin(...) ).
Anyways, as all the lists command buffers contain at least one command,
before window/foreground lists there are scissord, bind fonts texture, draw commands.
In case of background list there isn't any.

Callback could contain only the binding of the texture that later on the ImGui will be rendered on
(binding it shouldn't change anything, right?).

	glBindVertexArray(one_and_only_VAO);
	glActiveTexture(GL_TEXTURE0 + n); // n > 0
	glBindTexture(GL_TEXTURE_2D, texture_which_framebuffer_will_be_bound_before_ImGui_ImplOpenGL3_RenderDrawData);

and reset command should be submitted after the callback

	draw_list->AddCallback(callback_function, doesnt_matter);
	draw_list->AddCallback(ImDrawCallback_ResetRenderState, nullptr);

From the RenderDoc point of view it looks like if we add
draw_list->AddDrawCmd(); before the callback then additional pre-binding of fonts texture is submited.
And this is what matters (as I checked exchanging draw command with
glBindTexture(GL_TEXTURE_2D, (GLuint)(intptr_t)ImGui::GetIO().Fonts->TexID);),
but I still don't know why, as it seems like binding it just before
glDrawElementsBaseVertex / glDrawElements should be enough.

@ocornut
Copy link
Owner

ocornut commented Oct 9, 2024

Hello,

In callback_function I juggle textures, bind to units, use glsl programs, the whole zoo is involved.
That's why I add ImDrawCallback_ResetRenderState.

That's correct and expected.

At first I didn't add AddDrawCmd(); to redraw the rectangle that I will be processing in callback,
but I noticed that I have to do it in more and more cases.

I am not sure I understand what you are saying or suggesting here. AFAIK you should never have to call AddDrawCmd(). If you do there is either a misunderstanding or a problem.

Some comments on further replies:

(Daniel) AddDrawCmd() does not issue any kind of redraw, you should see it more as a pipeline flush. It prevents submitted primitives from before and after the call being merged into the same command

This is what is currently happening defacto but I don't expect to guarantee that draw calls won't be merged in the future. In particular, once we add AABB information in ImDrawCmd there will be opportunities to merge more draw calls and even reorder some. My expectation is that user should never have to use AddDrawCmd(). If it's useful I would like to know the details.

(Daniel) You can process geometry in callbacks, but you shouldn't process geometry that you submitted to ImGui. [...]

This is all correct. Additionally, you can use Tools->Metrics/Debugger to visualize the draw lists submitted to your backend.
Basically ImDrawList are submitted back to front.


At this point, I'm a little confused, should I assume that ImGui is using GL_TEXTURE0 ('+0') unit only?

Yes, the imgui_impl_opengl3 backend currently only uses GL_TEXTURE0.

OpenGL is a giant intricate state machine, lots of implicit state etc.
For your render code inside a callback to work and not break subsequent imgui rendering, you would need to satisfy two conditions:

  • Your own rendering in the callback manages to override/disable/correctly set every state that the imgui backend has been modifying (else your rendering may be botched).
  • Either your code and imgui backend handling of ImDrawCallback_ResetRenderState manages to reset everything correctly, include possible unknown/expected state that your callback modified.

I think you pretty much understood that at this point. Honestly, in OpenGL it is rather difficult and confusing to achieve.

There's lots of contents in your messages but it's difficult to make sense of it or draw conclusions. Narrowing it down would be good.

I suspect I shouldn't use something (glDrawElements?) or render to a completely different texture and after the callback use draw_list->AddImage or something. In the later case, is it ok (also performance-wise) to create/delete textures for each window's callback every frame or just make one and use it for all callbacks and just manipulate corners?

Indeed. My advice with OpenGL would be to render all your contents into textures and then just emitting ImGui::Image()/AddImage() calls. I don't know about OpenGL performances specifically but it's typical for games to have dozens layers of textures/buffers and hundreds of render to textures in a frame.

Regarding AddImage, if the texture is premultiplied, is it ok to use glBlendFunc before/after?

Yes.

@ocornut ocornut added the opengl label Oct 9, 2024
@ocornut ocornut changed the title Drawlists callbacks questions Drawlists callbacks questions with OpenGL Oct 9, 2024
ocornut added a commit that referenced this issue Oct 11, 2024
…copy and store any amount of user data for usage by callbacks: (#6969, #4770, #7665)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants