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

Refactor material. #122

Merged
merged 8 commits into from Feb 18, 2023
Merged

Refactor material. #122

merged 8 commits into from Feb 18, 2023

Conversation

Hinageshi01
Copy link
Contributor

@Hinageshi01 Hinageshi01 commented Feb 16, 2023

Refer to #112

Abstract Material Hierarchy

  • MaterialType
    • MaterialPropertyGroup
      • MaterialProperty

For example:

  • BasePBR
    • BaseColor
      • Color
      • Factor
      • Texture
    • Normal
      • Factor
      • Texture
    • ...

Actual Data Structure

  • SceneDatabase
    • Material
      • MaterialType
      • PropertyMap
    • Materila
    • ...

PropertyMap

A key-value map to store variant properties.

  • Key is a combination of MaterialPropertyGroup and MaterialProperty,
    which format is MaterialPropertyGroup_MaterialProperty.
  • Value is the real data of this MaterialType->MaterialPropertyGroup->MaterialProperty hierarchy.

TODO

Need to apply these changes and test on Engine side.

@Hinageshi01 Hinageshi01 added WIP Work In Progress API labels Feb 16, 2023
Comment on lines +29 to +45
enum class MaterialPropertyGroup
{
BaseColor = 0,
Normal,
Metalness,
Occlusion,
Roughness,
Metallic,
Normal,
Emissive,
AO,
Count
General,

Count,
};

constexpr const char* MaterialTextureTypeName[] =
// As each MaterialPropertyGroup just has one texture,
// it's fine to consider MaterialPropertyGroup as MaterialTextureType
// from the perspective of texture.
using MaterialTextureType = MaterialPropertyGroup;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Enums name changes a bit to match O3DE implementation.

Copy link
Contributor

Choose a reason for hiding this comment

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

No problem. After this change, you need to update in the engine side.

Comment on lines 106 to 116
inline std::string GetMaterialPropertyKey(MaterialPropertyGroup propertyGroup, MaterialProperty property)
{
std::stringstream ss;
ss << GetMaterialPropertyGroupName(propertyGroup) << "_" << GetMaterialPropertyName(property);

return ss.str();
}

inline const char* GetMaterialTextureTypeName(MaterialTextureType textureType)
inline std::string GetMaterialPropertyTextureKey(MaterialPropertyGroup propertyGroup)
{
return MaterialTextureTypeName[static_cast<int>(textureType)];
return GetMaterialPropertyKey(propertyGroup, MaterialProperty::Texture);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can use these util functions to easily get the key of PropertyMap

Copy link
Contributor

Choose a reason for hiding this comment

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

You can use CD_FORCEINLINE for one line function as inline keyword just suggets compiler to do inline but not force.

Comment on lines +8 to 22
MaterialImpl::MaterialImpl(MaterialID materialID, std::string materialName, MaterialType materialType)
{
Init(materialID, MoveTemp(materialName));
Init(materialID, MoveTemp(materialName), materialType);

switch (materialType)
{
case MaterialType::BasePBR:
InitBasePBR();
break;

default:
printf("Unknow material type!\n");
break;
}
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Material's constructor requires a new parameter to init default value of different material type.

Comment on lines 106 to 116
inline std::string GetMaterialPropertyKey(MaterialPropertyGroup propertyGroup, MaterialProperty property)
{
std::stringstream ss;
ss << GetMaterialPropertyGroupName(propertyGroup) << "_" << GetMaterialPropertyName(property);

return ss.str();
}

inline const char* GetMaterialTextureTypeName(MaterialTextureType textureType)
inline std::string GetMaterialPropertyTextureKey(MaterialPropertyGroup propertyGroup)
{
return MaterialTextureTypeName[static_cast<int>(textureType)];
return GetMaterialPropertyKey(propertyGroup, MaterialProperty::Texture);
Copy link
Contributor

Choose a reason for hiding this comment

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

You can use CD_FORCEINLINE for one line function as inline keyword just suggets compiler to do inline but not force.

Comment on lines +29 to +45
enum class MaterialPropertyGroup
{
BaseColor = 0,
Normal,
Metalness,
Occlusion,
Roughness,
Metallic,
Normal,
Emissive,
AO,
Count
General,

Count,
};

constexpr const char* MaterialTextureTypeName[] =
// As each MaterialPropertyGroup just has one texture,
// it's fine to consider MaterialPropertyGroup as MaterialTextureType
// from the perspective of texture.
using MaterialTextureType = MaterialPropertyGroup;
Copy link
Contributor

Choose a reason for hiding this comment

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

No problem. After this change, you need to update in the engine side.

Comment on lines 48 to 56
void AddProperty(MaterialPropertyGroup propertyGroup, MaterialProperty property, T value)
{
m_propertyGroups.Add(GetMaterialPropertyKey(propertyGroup, property), value);
}
template<typename T>
void SetProperty(MaterialPropertyGroup propertyGroup, MaterialProperty property, T value)
{
m_propertyGroups.Set(GetMaterialPropertyKey(propertyGroup, property), value);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Should avoid copy value if T is a string so you can MoveTemp(value) for all usages.

@T-rvw
Copy link
Contributor

T-rvw commented Feb 17, 2023

You can consider to serialize material type to SceneDatabase. And material type will have an ID. Then material instance will hold an ID to reference MaterialType. The advantage is that we don't limit material type by enum so everyone who wants to create a cutomized material type can add a new material type into SceneDatabase and set its ID for material instances.
Also, we should always create built-in material types in the SceneDatabase by default.

For example, I want to write a new shader about cartoon. In the high level, I need to create my cartoon material type and add to SceneDatabase. Then I will create multiple materials(instances) for multiple meshes which assigned to a cartoon material and fill their actual parameters.

For naming, it can be Effect and Material. Or MaterialType and MaterialInstance. It is two generic naming rules about this thing.

Copy link
Contributor

@T-rvw T-rvw left a comment

Choose a reason for hiding this comment

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

OK to merge as it doesn't make actual feature changes.

@Hinageshi01 Hinageshi01 merged commit 8e04650 into main Feb 18, 2023
@Hinageshi01 Hinageshi01 deleted the refactor_material branch February 18, 2023 09:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API WIP Work In Progress
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants