Gui Stencil Mask Optimization #1740

ZzzhHe · 2025-01-07T15:33:10Z

Resolve #1538

Changes:

Modified Presenter::render() to use three-pass rendering:
1. First pass: Create stencil mask from GUI
2. Second pass: Render world with stencil test (skip GUI-covered pixels)
3. Third pass: Render GUI normally

heinezen · 2025-01-11T03:29:55Z

Hey thanks for contributing again :) Unfortunately, I think the solution you made clashes a bit with our design of the renderer.

In general, the renderer code is separated into two levels (see our renderer docs for details). The (low-level) level 1 interface handles the bare-metal stuff, e.g. interfacing with OpenGL to talk to the GPU, and should generally be inaccessible to other parts of the engine. Instead, the rest of the engine mainly interacts with the level 2 interface of the renderer which abstracts the low-level stuff away and implements systems that can be used for gameplay, e.g. playing sprite animations. The reason we have this level structure is mostly thread-safety, but also easier maintainance, since we can change or replace the level 1 implementaton without changing the entire engine. The latter is important for offering multiple renderer backends simultaneously to the user, e.g. OpenGL and Vulkan.

In your solution, there would be "bare-metal" OpenGL calls in the presenter, even though the presenter is technically part of the level 2 interface. If we do it like this, then the presenter does not work with a level 1 Vulkan renderer backend anymore, for example. Ideally, the presenter does not have to care what backend we initialize the renderer with, whether it's OpenGL or Vulkan.

heinezen · 2025-01-11T03:49:07Z

For solving this task properly, we should try to add stencil tests as a more general feature to the level 1 renderer first. If we want to activate it for the GUI, the stencil test write code should be moved into the GUI subsystem. To use the stencil tests in the other render passes, we could add settings to the RenderPass implementations; maybe stencil test reading could be a flag... When the render pass is rendered, we could switch on/off stencil testing based on that flag.

So in summary my thoughts would be:

Move the stencil test writing into the libopenage/renderer/gui subsystem of the level 1 renderer
Add settings to a RenderPass that allows us to configure activating/deactivating stencil tests (and maybe other settings in the future)
Based on the RenderPass settings, activate stencil testing when the render pass becomes active in

openage/libopenage/renderer/opengl/renderer.cpp

Line 170 in d6a4dd3

void GlRenderer::render(const std::shared_ptr<RenderPass> &pass) {

heinezen · 2025-01-11T03:53:52Z

Apologies btw for askng for so many redesigns 😄 For issues that are not labelled "good first issue", the issue description can sometimes be a bit vague because we are ourselves not sure of what we want as a solution yet. You can always ask me for where to look in the docs before you do another issue like this one, maybe that helps :)

ZzzhHe · 2025-01-11T15:27:02Z

Thanks a lot for the detailed explanation! It all makes sense now. I’ll ensure to separate level 1 and level 2 as suggested and adjust my solution to follow this structure. I really appreciate your guidance!

ZzzhHe · 2025-01-18T21:21:40Z

Here are updates:

Added three stencil states in render pass to control rendering behavior:WRITE_STENCIL_MASK, USE_STENCIL_TEST, DISABLE_STENCIL.
Implemented stencil state handling in GlRenderer.
Added stencil_render_pass functionality in the GUI class.
Updated the Presenter class to handle Level 2 rendering abstractly, without OpenGL details.

heinezen · 2025-01-25T13:45:42Z

Finally had time to look at this 😄 I think we are getting there, but after looking at your code, I'm pretty sure that your stencil tests currently won't work. It's hard to test since we currently have deactivated the GUI, but here are my reasons:

Stencil testing is always done per framebuffer, so writing to a specific stencil buffer does not affect stencil testing in other framebuffers. In openage, the world render stage uses a different framebuffer, so you have to actively transfer the stencil buffer state from the GUI framebuffer before you can use stencil testing (i.e. read from the stencil values written by the GUI pass).
You have to actively attach a stencil buffer to the framebuffer in OpenGL which is currently not done in your code. The same applies to depth textures btw, they don't get assigned by default. You can see how we assign a depth buffer for the world render stage framebuffer here:

openage/libopenage/renderer/stages/world/render_stage.cpp

Line 138 in 6d9d8d7

    
           this->depth_texture = renderer->add_texture(resources::Texture2dInfo(width, height, resources::pixel_format::depth24));

You have introduced a lot of redundancy (e.g. the GUI is rendered twice now) which would counteract all our potential time saves from stencil testing :D

I would recommend that you don't implement this functionality in the presenter yet. A renderer demo is much better suited and allows you to test the level 1 implementation without having to worry about level 2 abstractions logic.

heinezen · 2025-01-25T13:47:36Z

libopenage/presenter/presenter.cpp

+	// set passes indices
+	this->index_gui_stencil_pass = this->render_passes.size() - 3;
+	this->index_gui_render_pass = this->render_passes.size() - 2;
+	this->index_final_render_pass = this->render_passes.size() - 1;


Hardcoding these indices makes the code much harder to maintain because we can't add new render passes between GUI and final pass. Ideally, the solution should work without any index assignment.

heinezen · 2025-01-25T13:48:57Z

libopenage/presenter/presenter.cpp

+	auto stencil_pass = this->gui->get_stencil_pass();
+	this->render_passes.push_back(stencil_pass);


I'm not sure why we need an additional render pass just for stencil testing. If you want to write stencil values, you can use the normal GUI render pass and activate stencil writing there. But as I pointed out in my main comment, you can't even read from the stencil values unless you are using the GUI framebuffer.

If you want the world stage render pass to use stencil testing, you would first have to write to its own framebuffer's stencil buffer and then afterwards do stencil testing. As I said before, you have to transfer the stencil buffer state from one framebuffer to the other. This can probably be done by rebinding the stencil attachement from the GUI framebuffer to the other framebuffers (via GL_STENCIL_ATTACHEMENT).

heinezen · 2025-01-25T14:02:16Z

libopenage/presenter/presenter.cpp

 	this->gui->render();
+	this->renderer->render(this->render_passes[this->index_gui_stencil_pass]);

-	for (auto &pass : this->render_passes) {
-		this->renderer->render(pass);
+	for (size_t i = 0; i < this->index_gui_stencil_pass; ++i) {
+		this->renderer->render(this->render_passes[i]);
 	}
+
+	this->gui->render();
+	this->renderer->render(this->render_passes[this->index_gui_render_pass]);
+
+	this->renderer->render(this->render_passes[this->index_final_render_pass]);


The GUI is now rendered twice, which is not what we want if the goal is performance ;)

Also, code like this needs comment because otherwise it becomes very confusing to understand why the order of render passes is structured this way.

heinezen · 2025-01-25T14:03:31Z

libopenage/renderer/gui/gui.cpp

@@ -98,6 +102,10 @@ void GUI::initialize_render_pass(size_t width,
 	// TODO: Rendering into the FBO is a bit redundant right now because we
 	// just copy the GUI texture into the output texture
 	this->render_pass = renderer->add_render_pass({display_obj}, fbo);
+
+	auto stencil_pass = renderer->add_render_pass({display_obj}, fbo);
+	stencil_pass->set_stencil_state(StencilState::WRITE_STENCIL_MASK);


I think if you just set the stencil state for this->render_pass, you would effectively have the same result without the need for an additional render pass.

heinezen · 2025-01-25T14:12:54Z

libopenage/renderer/opengl/renderer.cpp

+	glColorMask(GL_FALSE, GL_FALSE, GL_FALSE, GL_FALSE);
+	glDepthMask(GL_FALSE);


Is this necessary? Running these two statement would mean that the only buffer you write to is the stencil buffer. However, I don't see a reason why we shouldn't be able to draw into the color and depth buffer simultaneously.

heinezen · 2025-01-25T14:15:33Z

libopenage/renderer/opengl/renderer.cpp

+	// ensure that an (empty) VAO is bound before rendering geometry
+	// a bound VAO is required to render bufferless quad geometries by OpenGL
+	// see https://www.khronos.org/opengl/wiki/Vertex_Rendering#Causes_of_rendering_failure
+	shared_quad_vao->bind();


This duplicates a statement + comment from further above. Please remove it.

heinezen · 2025-01-25T14:20:55Z

libopenage/renderer/opengl/renderer.cpp

+		switch (layer.stencil_state) {
+		case StencilState::WRITE_STENCIL_MASK:
+			setupStencilWrite();
+			break;
+
+		case StencilState::USE_STENCIL_TEST:
+			setupStencilTest();
+			break;
+
+		case StencilState::DISABLE_STENCIL:
+			disableStencilTest();
+			break;
+		}


Doing stencil testing per layer is a bit overkill for now IMO, but it's a good idea :)

We have to make sure that the draw order is correct and should make performance tests for this.

heinezen · 2025-01-25T14:22:28Z

libopenage/renderer/opengl/renderer.h

+	void setupStencilWrite();
+
+	void setupStencilTest();
+
+	void disableStencilTest();


These functions should use snake case naming, e.g. setup_stencil_write, to adhere to our code style conventions.

heinezen · 2025-01-25T14:23:06Z

libopenage/renderer/render_pass.h

+/**
+ * Stencil states for the render pass.
+ */
+enum class StencilState {


same here, enums should use snake case naming

heinezen · 2025-01-25T14:25:41Z

libopenage/renderer/render_pass.h

+	/// State for writing GUI elements to stencil buffer.
+	WRITE_STENCIL_MASK,
+	/// State for using the mask when rendering scene.
+	USE_STENCIL_TEST,
+	/// State for normal rendering (GUI rendering).
+	DISABLE_STENCIL


These docstrings refer to the GUI, but stencil testing should not just be used for this purpose. So it's better to make the docstrings more agnostic.

ZzzhHe · 2025-01-26T18:27:48Z

Oh.. I was too focused on implementing the stencil test functions and completely missed the fundamentals of how framebuffers work in OpenGL. I'll start fresh with a renderer demo this week to re-write and test the core functionality properly. Thanks!

heinezen added area: renderer Concerns our graphics renderer nice new thing ☺ A new feature that was not there before lang: c++ Done in C++ code labels Jan 11, 2025

ZzzhHe added 3 commits January 18, 2025 15:43

add gui stencil mask for presenter class

240a10f

refactor by add init_stencil()

2622849

implement stencil test in Level1 and integrate it in render pass

18622d4

ZzzhHe force-pushed the gui-stencil-mask-optimization branch from d63438c to 18622d4 Compare January 18, 2025 21:15

clean some format issues

7eca3b5

ZzzhHe force-pushed the gui-stencil-mask-optimization branch from eb58af1 to 7eca3b5 Compare January 19, 2025 19:21

heinezen requested changes Jan 25, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Gui Stencil Mask Optimization #1740

Gui Stencil Mask Optimization #1740

ZzzhHe commented Jan 7, 2025

heinezen commented Jan 11, 2025 •

edited

Loading

heinezen commented Jan 11, 2025

heinezen commented Jan 11, 2025

ZzzhHe commented Jan 11, 2025 •

edited

Loading

ZzzhHe commented Jan 18, 2025

heinezen commented Jan 25, 2025

heinezen Jan 25, 2025

heinezen Jan 25, 2025

heinezen Jan 25, 2025

heinezen Jan 25, 2025

heinezen Jan 25, 2025

heinezen Jan 25, 2025 •

edited

Loading

heinezen Jan 25, 2025

heinezen Jan 25, 2025

heinezen Jan 25, 2025

heinezen Jan 25, 2025

ZzzhHe commented Jan 26, 2025

		auto stencil_pass = this->gui->get_stencil_pass();
		this->render_passes.push_back(stencil_pass);

		glColorMask(GL_FALSE, GL_FALSE, GL_FALSE, GL_FALSE);
		glDepthMask(GL_FALSE);

Gui Stencil Mask Optimization #1740

Are you sure you want to change the base?

Gui Stencil Mask Optimization #1740

Conversation

ZzzhHe commented Jan 7, 2025

heinezen commented Jan 11, 2025 • edited Loading

heinezen commented Jan 11, 2025

heinezen commented Jan 11, 2025

ZzzhHe commented Jan 11, 2025 • edited Loading

ZzzhHe commented Jan 18, 2025

heinezen commented Jan 25, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

heinezen Jan 25, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ZzzhHe commented Jan 26, 2025

heinezen commented Jan 11, 2025 •

edited

Loading

ZzzhHe commented Jan 11, 2025 •

edited

Loading

heinezen Jan 25, 2025 •

edited

Loading