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.

Sign up for GitHub

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

Separate linear and sRGB uniform buffers in RD rendering backends #92444

Merged
merged 1 commit into from
May 31, 2024
Merged

Separate linear and sRGB uniform buffers in RD rendering backends #92444

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 16 additions & 14 deletions servers/rendering/renderer_rd/storage_rd/material_storage.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -746,8 +746,10 @@ MaterialStorage::MaterialData::~MaterialData() {
material_storage->global_shader_uniforms.materials_using_texture.erase(global_texture_E);
}

if (uniform_buffer.is_valid()) {
RD::get_singleton()->free(uniform_buffer);
for (int i = 0; i < 2; i++) {
if (uniform_buffer[i].is_valid()) {
RD::get_singleton()->free(uniform_buffer[i]);
}
}
}

Expand Down Expand Up @@ -987,17 +989,17 @@ void MaterialStorage::MaterialData::free_parameters_uniform_set(RID p_uniform_se
}

bool MaterialStorage::MaterialData::update_parameters_uniform_set(const HashMap<StringName, Variant> &p_parameters, bool p_uniform_dirty, bool p_textures_dirty, const HashMap<StringName, ShaderLanguage::ShaderNode::Uniform> &p_uniforms, const uint32_t *p_uniform_offsets, const Vector<ShaderCompiler::GeneratedCode::Texture> &p_texture_uniforms, const HashMap<StringName, HashMap<int, RID>> &p_default_texture_params, uint32_t p_ubo_size, RID &uniform_set, RID p_shader, uint32_t p_shader_uniform_set, bool p_use_linear_color, bool p_3d_material) {
if ((uint32_t)ubo_data.size() != p_ubo_size) {
if ((uint32_t)ubo_data[p_use_linear_color].size() != p_ubo_size) {
Copy link
Contributor

@Dowsley Dowsley Jun 2, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@clayjohn I know the feature works because you reproduced so I'm just trying to learn here.

If ubo_data[0] is linear and ubo_data[1] is sRGB, why is the boolean p_use_linear_color used like this? When it's true, it evaluates to 1 and accesses the sRGB data. Which seems counter-intuitive given the name of the variable.

p_uniform_dirty = true;
if (uniform_buffer.is_valid()) {
RD::get_singleton()->free(uniform_buffer);
uniform_buffer = RID();
if (uniform_buffer[p_use_linear_color].is_valid()) {
RD::get_singleton()->free(uniform_buffer[p_use_linear_color]);
uniform_buffer[p_use_linear_color] = RID();
}

ubo_data.resize(p_ubo_size);
if (ubo_data.size()) {
uniform_buffer = RD::get_singleton()->uniform_buffer_create(ubo_data.size());
memset(ubo_data.ptrw(), 0, ubo_data.size()); //clear
ubo_data[p_use_linear_color].resize(p_ubo_size);
if (ubo_data[p_use_linear_color].size()) {
uniform_buffer[p_use_linear_color] = RD::get_singleton()->uniform_buffer_create(ubo_data[p_use_linear_color].size());
memset(ubo_data[p_use_linear_color].ptrw(), 0, ubo_data[p_use_linear_color].size()); //clear
}

//clear previous uniform set
Expand All @@ -1009,9 +1011,9 @@ bool MaterialStorage::MaterialData::update_parameters_uniform_set(const HashMap<
}

//check whether buffer changed
if (p_uniform_dirty && ubo_data.size()) {
update_uniform_buffer(p_uniforms, p_uniform_offsets, p_parameters, ubo_data.ptrw(), ubo_data.size(), p_use_linear_color);
RD::get_singleton()->buffer_update(uniform_buffer, 0, ubo_data.size(), ubo_data.ptrw());
if (p_uniform_dirty && ubo_data[p_use_linear_color].size()) {
update_uniform_buffer(p_uniforms, p_uniform_offsets, p_parameters, ubo_data[p_use_linear_color].ptrw(), ubo_data[p_use_linear_color].size(), p_use_linear_color);
RD::get_singleton()->buffer_update(uniform_buffer[p_use_linear_color], 0, ubo_data[p_use_linear_color].size(), ubo_data[p_use_linear_color].ptrw());
}

uint32_t tex_uniform_count = 0U;
Expand Down Expand Up @@ -1053,7 +1055,7 @@ bool MaterialStorage::MaterialData::update_parameters_uniform_set(const HashMap<
RD::Uniform u;
u.uniform_type = RD::UNIFORM_TYPE_UNIFORM_BUFFER;
u.binding = 0;
u.append_id(uniform_buffer);
u.append_id(uniform_buffer[p_use_linear_color]);
uniforms.push_back(u);
}

Expand Down
4 changes: 2 additions & 2 deletions servers/rendering/renderer_rd/storage_rd/material_storage.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,8 +100,8 @@ class MaterialStorage : public RendererMaterialStorage {
HashMap<StringName, uint64_t> used_global_textures;

//internally by update_parameters_uniform_set
Vector<uint8_t> ubo_data;
RID uniform_buffer;
Vector<uint8_t> ubo_data[2]; // 0: linear buffer; 1: sRGB buffer.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would use an enum for the buffer, its just clearer why we have two. Might be over kill.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you clarify where the Enum would go?

Do you mean you would create an enum just to establish the size of the Array?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe what he means is making it clearer that 0 is Linear and 1 is sRGB through an Enum. Declared at the top of material.h like this:

enum BufferType {
	LINEAR = 0,
	SRGB = 1
};

Used as such (without the need of comments):

Vector<uint8_t> ubo_data[BufferType.LINEAR];
RID uniform_buffer[BufferType.SRGB];

I don't find it overkill, might save some headache for someone trying to guess which one is which in a later feature. But I think this ship has sailed since it has already been merged.

@clayjohn if this is welcome, let me know if I should make another PR with this minor change. It might be a good start for me to get used with contributions.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think it would be beneficial. Using enums hides what is actually happening. For example, your code will fail to compile (an array of size 0 doesn't work for the UBO array, the array needs to have 2 elements). Also, the uniform buffer RID array is also wrong since it also needs two elements.

It's really easy to make this kind of mistake when you hide the important values behind an enum.

That being said, enums can be really handy when you use them both to initialize the array and to index into the array. Doing something like:

LINEAR = 0 // use to index into the array
SRGB = 1 // use to index into the array
MAX = 2 // use to initialize the array

But in this case we index into the array with a boolean. So enums won't help.

IMO using enums here adds more complexity for no gain

RID uniform_buffer[2]; // 0: linear buffer; 1: sRGB buffer.
Vector<RID> texture_cache;
};

Expand Down
Loading