Update syntax of GlUniformBuffer to match that of GlUniformInput #1683 #1690

EdvinLndh · 2024-09-18T12:30:16Z

Made some similar changes as to what was done on #1683. Hope this is somewhat what you had in mind.

My first PR ever! =)

…Input

heinezen

Hey thanks a lot! Also great that it runs when I tested it because that's not a given for contributions :D

There's a few things that still need to be added in the PR:

The changes in the GlUniformInput input class should be reflected in the GlUniformBufferInput class, i.e. in uniform_input.h both classes should look the same apart from the constructor. (This is part of the first task in the issue)
You need to add your info to the table in copying.md file in our repo. That's the reason why the CI fails currently.

…Input

…into uniform_syntax_change

EdvinLndh · 2024-09-19T12:11:44Z

Ah, I see now that it says so quite clearly in the contribution page 😅.

I have made some changes to the GlUniformBufferInput class.

heinezen · 2024-09-20T12:28:53Z

Nicely done! I'm currently testing your changes locally and will report back later.

heinezen

Very good! I found one potential problem but once you have fixed that, it should be good to go.

heinezen · 2024-09-20T14:08:55Z

libopenage/renderer/opengl/uniform_buffer.cpp

+	const auto &update_offs = glunif_in->update_offs;
 	uint8_t const *data = glunif_in->update_data.data();
-	for (auto const &pair : glunif_in->update_offs) {
-		uint8_t const *ptr = data + pair.second;
-		auto unif_def = this->uniforms[pair.first];
+	for (auto const &pair : this->uniforms_by_name) {
+		auto id = pair.second;
+		auto offset = update_offs[id];
+		uint8_t const *ptr = data + offset.offset;
+		auto unif_def = this->uniforms.find(pair.first)->second;


You should not iterate through this->uniforms_by_name here and instead use glunif_in->used_uniforms. Reasons for this are:

UniformBufferInput can be partial updates where only a few uniforms in the buffer are updated and not all. If you iterate on this->uniforms_by_name, this could then read some uninitialized data into the buffer.

Iterating on a std::unordered_map like this->uniforms_by_name will be very slow compared to iterating on a std::vector (because of their memory layout). This is part of the reason why we want to "vectorize" these parameters: The vectors are much faster in our renderer.

You can check out the update for the regular UniformInput in this code sample for inspiration:

openage/libopenage/renderer/opengl/shader_program.cpp

Lines 352 to 365 in 380d2e9

const auto &update_offs = unif_in->update_offs;

const auto &used_uniforms = unif_in->used_uniforms;

const auto &uniforms = this->uniforms;

uint8_t const *data = unif_in->update_data.data();

size_t unif_count = used_uniforms.size();

for (size_t i = 0; i < unif_count; ++i) {

uniform_id_t unif_id = used_uniforms[i];

const auto &update_off = update_offs[unif_id];

uint8_t const *ptr = data + update_off.offset;

const auto &unif = uniforms[unif_id];

auto loc = unif.location;

The syntax of your code can be roughly the same I think :)

I see, I think one of the reasons I went with that approach was in order to be able to lookup the GlBlockInUniform in the uniforms map. When iterating over the id's I'm not really sure how I should retrieve the correct value from the uniforms map. Do you think the uniforms attribute should be kept as a std::unordered_map?

Yeah I think this might be a good opportunity to change the type of GlUniformBuffer::uniforms to std::vector. That's already something we do for the uniforms in shader_program.h for uniforms outside of blocks:

openage/libopenage/renderer/opengl/shader_program.h

Lines 184 to 189 in 380d2e9

/// Uniforms in the shader program. Contains only

/// uniforms in the default block, i.e. not within named blocks.

std::vector<GlUniform> uniforms;

/// Maps uniform names to their ID (the index in the uniform vector).

std::unordered_map<std::string, uniform_id_t> uniforms_by_name;

The change should be pretty straight-forward I hope. Apart from the type of uniforms, you'll have to change the constructor of GlUniformBuffer and the Renderer::add_uniform_buffer methods:

openage/libopenage/renderer/opengl/renderer.cpp

Line 87 in 380d2e9

std::shared_ptr<UniformBuffer> GlRenderer::add_uniform_buffer(resources::UniformBufferInfo const &info) {

openage/libopenage/renderer/opengl/renderer.cpp

Line 115 in 380d2e9

std::shared_ptr<UniformBuffer> GlRenderer::add_uniform_buffer(std::shared_ptr<ShaderProgram> const &prog,

shader_data.h also needs to be changed for the GlInBlockUniform:

openage/libopenage/renderer/opengl/shader_data.h

Lines 78 to 81 in 380d2e9

/**

* Maps uniform names within this block to their descriptions.

*/

std::unordered_map<std::string, GlInBlockUniform> uniforms;

I have now changed from std::unordered_map to std::vector. I did add a name attribute in the GlInBlockUniform. the name is used instead of the the key that was stored with the map previously, mostly debugging and for creating the uniforms_by_name map. I'm not sure if this is how it should be, but I suppose some performance was gained, what do you think?

Great, I'll check it out now.

heinezen · 2024-09-22T20:49:36Z

The code already looks good. I'll run a few performance tests now.

heinezen · 2024-09-22T20:58:35Z

Very nice, it works like a charm and is also faster than before :)

heinezen · 2024-09-22T21:08:32Z

@EdvinLndh If you want, there are other renderer issues that should be easily doable now that you worked with part of the code already 😄

EdvinLndh · 2024-09-23T07:13:27Z

Cool! I'll check them out.

renderer: Update syntax of GlUniformBuffer to match that of GlUniform…

c6db4fb

…Input

heinezen linked an issue Sep 18, 2024 that may be closed by this pull request

Use same syntax for updating uniform inputs and uniform buffer inputs #1683

Closed

3 tasks

heinezen added improvement Enhancement of an existing component area: renderer Concerns our graphics renderer lang: c++ Done in C++ code labels Sep 18, 2024

heinezen requested changes Sep 18, 2024

View reviewed changes

EdvinLndh added 3 commits September 19, 2024 14:02

renderer: Update syntax of GlUniformBuffer to match that of GlUniform…

6256c66

…Input

Added syntax changes to GlUniformBufferInput class.

623554e

Merge branch 'uniform_syntax_change' of github.com:EdvinLndh/openage …

0c1a368

…into uniform_syntax_change

heinezen requested changes Sep 20, 2024

View reviewed changes

Changed uniforms attribute from unordered_map to vector

de212cb

heinezen approved these changes Sep 22, 2024

View reviewed changes

heinezen merged commit 66098ce into SFTtech:master Sep 22, 2024

EdvinLndh deleted the uniform_syntax_change branch September 23, 2024 07:13

	const auto &update_offs = unif_in->update_offs;
	const auto &used_uniforms = unif_in->used_uniforms;
	const auto &uniforms = this->uniforms;
	uint8_t const *data = unif_in->update_data.data();

	size_t unif_count = used_uniforms.size();
	for (size_t i = 0; i < unif_count; ++i) {
	uniform_id_t unif_id = used_uniforms[i];

	const auto &update_off = update_offs[unif_id];
	uint8_t const *ptr = data + update_off.offset;

	const auto &unif = uniforms[unif_id];
	auto loc = unif.location;

	/// Uniforms in the shader program. Contains only
	/// uniforms in the default block, i.e. not within named blocks.
	std::vector<GlUniform> uniforms;

	/// Maps uniform names to their ID (the index in the uniform vector).
	std::unordered_map<std::string, uniform_id_t> uniforms_by_name;

	/**
	* Maps uniform names within this block to their descriptions.
	*/
	std::unordered_map<std::string, GlInBlockUniform> uniforms;

Uh oh!

Update syntax of GlUniformBuffer to match that of GlUniformInput #1683 #1690

Update syntax of GlUniformBuffer to match that of GlUniformInput #1683 #1690

Uh oh!

Conversation

EdvinLndh commented Sep 18, 2024

Uh oh!

heinezen left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

EdvinLndh commented Sep 19, 2024

Uh oh!

heinezen commented Sep 20, 2024

Uh oh!

heinezen left a comment

Choose a reason for hiding this comment

Uh oh!

heinezen Sep 20, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

EdvinLndh Sep 20, 2024

Choose a reason for hiding this comment

Uh oh!

heinezen Sep 21, 2024

Choose a reason for hiding this comment

Uh oh!

heinezen Sep 21, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

EdvinLndh Sep 22, 2024

Choose a reason for hiding this comment

Uh oh!

heinezen Sep 22, 2024

Choose a reason for hiding this comment

Uh oh!

heinezen commented Sep 22, 2024

Uh oh!

heinezen commented Sep 22, 2024

Uh oh!

heinezen commented Sep 22, 2024

Uh oh!

EdvinLndh commented Sep 23, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

heinezen left a comment •

edited

Loading

heinezen Sep 20, 2024 •

edited

Loading

heinezen Sep 21, 2024 •

edited

Loading