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

VideoCommon: add additional data types to material asset #12240

Merged

Conversation

iwubcode
Copy link
Contributor

This is another PR for adding support for custom shader uniforms to Dolphin. This time, we extend the Material asset to include additional properties (int, int2, ... float, float2,...bool) and add some helper functions to get the material data to the GPU (not used yet, will be used in a future PR).

Copy link
Contributor

@TellowKrinkle TellowKrinkle left a comment

Choose a reason for hiding this comment

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

Strongly typed variants are cool, but I also think you could get a lot less code duplication if you made the element count a runtime value
e.g. WriteAsShaderCode could be closer to

static const char* to_name(MaterialProperty::Type type)
{
  switch (type)
  {
    case MaterialProperty::Type::Type_Int:   return "int";
    case MaterialProperty::Type::Type_Float: return "float";
    case MaterialProperty::Type::Type_Bool:  return "bool";
  }
}
void MaterialProperty::WriteAsShaderCode(ShaderCode& shader_source, const MaterialProperty& property)
{
  const char* type_name = to_name(property.type);
  std::string elems = property.element_count == 1 ? "" : std::to_string(property.element_count);
  shader_source.Write("{}{} {};\n", type_name, elems, property.m_code_name);
  for (int i = property.element_count; i < 4; i++)
    shader_source.Write("{} {}_padding_{};\n", type_name, property.m_code_name, i + 1);
}

I think it would also make a future change where we start tightly packing elements (instead of padding everything to 16 bytes) to save space much easier.

Source/Core/VideoCommon/Assets/MaterialAsset.cpp Outdated Show resolved Hide resolved
Source/Core/VideoCommon/Assets/MaterialAsset.cpp Outdated Show resolved Hide resolved
@iwubcode iwubcode force-pushed the material_asset_additional_properties branch 2 times, most recently from 50d91f4 to e6110a4 Compare October 17, 2023 01:22
…as properties of materials and some helper functions to support sending the data to the GPU
@iwubcode iwubcode force-pushed the material_asset_additional_properties branch from e6110a4 to e204b3c Compare December 4, 2023 06:14
@iwubcode
Copy link
Contributor Author

iwubcode commented Dec 4, 2023

The material now uses only std::variant as opposed to also including a type. I updated the material data to also not use std::optional as data should always be present (eventually will be defaulted as laid out by shader data).

@lioncash lioncash merged commit ea30337 into dolphin-emu:master Dec 12, 2023
11 checks passed
@iwubcode iwubcode deleted the material_asset_additional_properties branch December 12, 2023 23:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
3 participants