Update syntax of GlUniformBuffer to match that of GlUniformInput #1683

[EdvinLndh]

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

My first PR ever! =)

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

[EdvinLndh]

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

I have made some changes to the GlUniformBufferInput class.

[heinezen]

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]

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

[EdvinLndh]

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?

[heinezen]

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,

[heinezen]

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;

[EdvinLndh]

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?

[heinezen]

Great, I'll check it out now.

[heinezen]

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

[heinezen]

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

[heinezen]

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

• Optionally check if all uniform values have been set #1681
• Boundaries for camera movement #1682

[EdvinLndh]

Cool! I'll check them out.