Skip to content

Commit

Permalink
--reviewer comments
Browse files Browse the repository at this point in the history
  • Loading branch information
@jturner65
jturner65 committed Apr 26, 2023
1 parent 158112d commit 026dfb1
Show file tree
Hide file tree
Showing 6 changed files with 103 additions and 114 deletions.
2 changes: 2 additions & 0 deletions src/cmake/dependencies.cmake
View file
Original file line number Diff line number Diff line change
Expand Up @@ -199,6 +199,8 @@ if(NOT USE_SYSTEM_MAGNUM)

# These are enabled by default but we don't need them for anything yet
set(MAGNUM_WITH_SHADERTOOLS OFF CACHE BOOL "" FORCE)
# These used to be disabled here but now aren't, explicitly enable them to
# update options in existing builds
set(MAGNUM_WITH_MATERIALTOOLS ON CACHE BOOL "" FORCE)

# These are enabled by default but we don't need them if not building GUI
Expand Down
72 changes: 36 additions & 36 deletions src/esp/assets/ResourceManager.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2358,11 +2358,8 @@ Mn::Trade::MaterialData ResourceManager::setDefaultMaterialUserAttributes(
Cr::Containers::Array<Mn::Trade::MaterialAttributeData> newAttributes;
arrayAppend(newAttributes, Cr::InPlaceInit, "hasPerVertexObjectId",
hasVertObjID);
arrayAppend(newAttributes, Cr::InPlaceInit, "hasTextureObjectId",
hasTxtrObjID);

if (hasTxtrObjID) {
arrayAppend(newAttributes, Cr::InPlaceInit, "objectIdTexturePtr",
arrayAppend(newAttributes, Cr::InPlaceInit, "objectIdTexture",
textures_.at(txtrIdx).get());
}
arrayAppend(newAttributes, Cr::InPlaceInit, "shaderTypeToUse",
Expand Down Expand Up @@ -2569,12 +2566,10 @@ void ResourceManager::loadMaterials(Importer& importer,
// Merge all custom attribute except remapped texture pointers with
// original material for final material. custMaterialData should never be
// Cr::Containers::NullOpt since every non-error branch is covered.
// custMaterialData needs to have fewer or equal layers than materialData
// or the rest will be
Cr::Containers::Optional<Mn::Trade::MaterialData> mergedCustomMaterial =
Mn::MaterialTools::merge(
*custMaterialData, *materialData,
Mn::MaterialTools::MergeConflicts::KeepFirstIgnoreType);
Mn::MaterialTools::MergeConflicts::KeepFirstIfSameType);

// Now build texture pointer array, with appropriate layers based on
// number of layers in original material
Expand All @@ -2591,35 +2586,36 @@ void ResourceManager::loadMaterials(Importer& importer,
// Returns nullptr if has no attributes
if (materialAttributes != nullptr) {
// Idxs in materialAttributes corresponding to each layer
const Cr::Containers::ArrayView<const Mn::UnsignedInt> layerIdxs =
const Cr::Containers::ArrayView<const Mn::UnsignedInt> layersFromMat =
materialData->layerData();

int layerIdx = 0;
// Add all material texture pointers into new attributes
for (int i = 0; i < materialAttributes.size(); ++i) {
for (int mIdx = 0; mIdx < materialAttributes.size(); ++mIdx) {
const Mn::Trade::MaterialAttributeData& materialAttribute =
materialAttributes[i];
auto matName = materialAttribute.name();
materialAttributes[mIdx];
auto attrName = materialAttribute.name();
const auto matType = materialAttribute.type();
// Find textures and add them to newAttributes
// bool found = (std::string::npos != key.find(strToLookFor));
if ((matType == Mn::Trade::MaterialAttributeType::UnsignedInt) &&
matName.contains("Texture") && !matName.contains("Scale")) {
attrName.contains("Texture") && !attrName.contains("Scale")) {
// texture index, copy texture pointer into newAttributes
const Mn::UnsignedInt txtrIdx =
materialAttribute.value<Mn::UnsignedInt>();
// copy texture into new attributes tagged with lowercase material
// name
auto newMatName = Cr::Utility::formatString(
"{}{}Ptr", Cr::Utility::String::lowercase(matName.slice(0, 1)),
matName.slice(1, matName.size()));

auto newAttrName = Cr::Utility::formatString(
"{}{}", Cr::Utility::String::lowercase(attrName.slice(0, 1)),
attrName.slice(1, attrName.size()));
// Temporary debug message
Cr::Utility::formatInto(debugStr, debugStr.size(),
"| txtr ptr name:{} | idx :{} ", newMatName,
(textureBaseIndex + txtrIdx));

"| txtr ptr name:{} | idx :{} Layer {}",
newAttrName, (textureBaseIndex + txtrIdx),
layerIdx);
arrayAppend(
newAttributes,
{newMatName, textures_.at(textureBaseIndex + txtrIdx).get()});
{newAttrName, textures_.at(textureBaseIndex + txtrIdx).get()});
}
}
} // if has attributes
Expand Down Expand Up @@ -2657,29 +2653,33 @@ Mn::Trade::MaterialData ResourceManager::buildCustomAttributeFlatMaterial(
const auto& flatMat = materialData.as<Mn::Trade::FlatMaterialData>();
// Populate base/diffuse color and texture (if present) into flat
// material array
arrayAppend(custAttributes,
{Mn::Trade::MaterialAttribute::AmbientColor, flatMat.color()});
// Clear out diffuse and specular colors for flat material
arrayAppend(custAttributes,
{Mn::Trade::MaterialAttribute::DiffuseColor, 0x00000000_rgbaf});
// No default shininess in Magnum materials
arrayAppend(custAttributes, {Mn::Trade::MaterialAttribute::Shininess, 80.0f});
arrayAppend(custAttributes,
{Mn::Trade::MaterialAttribute::SpecularColor, 0x00000000_rgbaf});
// Only populate into ambient texture if present in original material
arrayAppend(
custAttributes,
{// Set ambient color from flat material's base/diffuse color
{Mn::Trade::MaterialAttribute::AmbientColor, flatMat.color()},
// Clear out diffuse and specular colors for flat material
{Mn::Trade::MaterialAttribute::DiffuseColor, 0x00000000_rgbaf},
// No default shininess in Magnum materials
{Mn::Trade::MaterialAttribute::Shininess, 80.0f},

{Mn::Trade::MaterialAttribute::SpecularColor, 0x00000000_rgbaf}});
// Only populate into ambient texture if present in original
// material
if (flatMat.hasTexture()) {
arrayAppend(custAttributes,
{"ambientTexturePtr",
{"ambientTexture",
textures_.at(textureBaseIndex + flatMat.texture()).get()});
}
// Merge new attributes with those specified in original material
// overridding original ambient, diffuse and specular colors
// Using owning MaterialData constructor to handle potential layers
// Using owning MaterialData constructor to handle potential
// layers
auto finalMaterial = Mn::MaterialTools::merge(
Mn::Trade::MaterialData{{}, std::move(custAttributes), {}}, materialData,
Mn::MaterialTools::MergeConflicts::KeepFirstIgnoreType);
Mn::MaterialTools::MergeConflicts::KeepFirstIfSameType);

// Set default, expected user attributes for the final material and return
// Set default, expected user attributes for the final material
// and return
return setDefaultMaterialUserAttributes(*finalMaterial,
ObjectInstanceShaderType::Flat);
} // ResourceManager::buildFlatShadedMaterialData
Expand All @@ -2700,7 +2700,7 @@ Mn::Trade::MaterialData ResourceManager::buildCustomAttributePhongMaterial(
// Using owning MaterialData constructor to handle potential layers
auto finalMaterial = Mn::MaterialTools::merge(
Mn::Trade::MaterialData{{}, std::move(custAttributes), {}},
materialData, Mn::MaterialTools::MergeConflicts::KeepFirstIgnoreType);
materialData, Mn::MaterialTools::MergeConflicts::KeepFirstIfSameType);

// Set default, expected user attributes for the final material and return
return setDefaultMaterialUserAttributes(*finalMaterial,
Expand Down Expand Up @@ -2728,7 +2728,7 @@ Mn::Trade::MaterialData ResourceManager::buildCustomAttributePbrMaterial(
// Using owning MaterialData constructor to handle potential layers
auto finalMaterial = Mn::MaterialTools::merge(
Mn::Trade::MaterialData{{}, std::move(custAttributes), {}},
materialData, Mn::MaterialTools::MergeConflicts::KeepFirstIgnoreType);
materialData, Mn::MaterialTools::MergeConflicts::KeepFirstIfSameType);

// Set default, expected user attributes for the final material and return
return setDefaultMaterialUserAttributes(*finalMaterial,
Expand Down
41 changes: 16 additions & 25 deletions src/esp/gfx/GenericDrawable.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -45,35 +45,28 @@ GenericDrawable::GenericDrawable(
/* If texture transformation is specified, enable it only if the material is
actually textured -- it's an error otherwise */
if (materialData_->commonTextureMatrix() != Mn::Matrix3{} &&
(materialData_->hasAttribute("ambientTexturePtr") ||
materialData_->hasAttribute("baseColorTexturePtr") ||
materialData_->hasAttribute("diffuseTexturePtr") ||
materialData_->hasAttribute("specularTexturePtr") ||
materialData_->attribute<bool>("hasTextureObjectId"))) {
ESP_WARNING() << "Drawable has texture transformation";
(materialData_->hasAttribute("ambientTexture") ||
materialData_->hasAttribute("baseColorTexture") ||
materialData_->hasAttribute("diffuseTexture") ||
materialData_->hasAttribute("specularTexture") ||
materialData_->hasAttribute("objectIdTexture"))) {
flags_ |= Mn::Shaders::PhongGL::Flag::TextureTransformation;
}
if ((materialData_->hasAttribute("ambientTexturePtr")) ||
(materialData_->hasAttribute("baseColorTexturePtr"))) {
ESP_WARNING() << "Drawable has ambientTexture/baseColorTexturePtr";
if ((materialData_->hasAttribute("ambientTexture")) ||
(materialData_->hasAttribute("baseColorTexture"))) {
flags_ |= Mn::Shaders::PhongGL::Flag::AmbientTexture;
}
if (materialData_->hasAttribute("diffuseTexturePtr")) {
ESP_WARNING() << "Drawable has diffuseTexturePtr";
if (materialData_->hasAttribute("diffuseTexture")) {
flags_ |= Mn::Shaders::PhongGL::Flag::DiffuseTexture;
}
if (materialData_->hasAttribute("specularTexturePtr")) {
ESP_WARNING() << "Drawable has specularTexturePtr";
if (materialData_->hasAttribute("specularTexture")) {
flags_ |= Mn::Shaders::PhongGL::Flag::SpecularTexture;
}

if (materialData_->hasAttribute("normalTexturePtr")) {
ESP_WARNING() << "Drawable has normalTexturePtr";
if (materialData_->hasAttribute("normalTexture")) {
if (meshAttributeFlags & Drawable::Flag::HasTangent) {
ESP_WARNING() << "Drawable has HasTangent";
flags_ |= Mn::Shaders::PhongGL::Flag::NormalTexture;
if (meshAttributeFlags & Drawable::Flag::HasSeparateBitangent) {
ESP_WARNING() << "Drawable has HasSeparateBitangent";
flags_ |= Mn::Shaders::PhongGL::Flag::Bitangent;
}
} else {
Expand All @@ -82,11 +75,9 @@ GenericDrawable::GenericDrawable(
}
}
if (materialData_->attribute<bool>("hasPerVertexObjectId")) {
ESP_WARNING() << "Drawable has hasPerVertexObjectId";
flags_ |= Mn::Shaders::PhongGL::Flag::InstancedObjectId;
}
if (materialData_->attribute<bool>("hasTextureObjectId")) {
ESP_WARNING() << "Drawable has hasTextureObjectId";
if (materialData_->hasAttribute("objectIdTexture")) {
flags_ |= Mn::Shaders::PhongGL::Flag::ObjectIdTexture;
}
if (meshAttributeFlags & Drawable::Flag::HasVertexColor) {
Expand Down Expand Up @@ -194,23 +185,23 @@ void GenericDrawable::draw(const Mn::Matrix4& transformationMatrix,

if (flags_ & Mn::Shaders::PhongGL::Flag::AmbientTexture) {
shader_->bindAmbientTexture(
*(materialData_->attribute<Mn::GL::Texture2D*>("ambientTexturePtr")));
*(materialData_->attribute<Mn::GL::Texture2D*>("ambientTexture")));
}
if (flags_ & Mn::Shaders::PhongGL::Flag::DiffuseTexture) {
shader_->bindDiffuseTexture(
*(materialData_->attribute<Mn::GL::Texture2D*>("diffuseTexturePtr")));
*(materialData_->attribute<Mn::GL::Texture2D*>("diffuseTexture")));
}
if (flags_ & Mn::Shaders::PhongGL::Flag::SpecularTexture) {
shader_->bindSpecularTexture(
*(materialData_->attribute<Mn::GL::Texture2D*>("specularTexturePtr")));
*(materialData_->attribute<Mn::GL::Texture2D*>("specularTexture")));
}
if (flags_ & Mn::Shaders::PhongGL::Flag::NormalTexture) {
shader_->bindNormalTexture(
*(materialData_->attribute<Mn::GL::Texture2D*>("normalTexturePtr")));
*(materialData_->attribute<Mn::GL::Texture2D*>("normalTexture")));
}
if (flags_ >= Mn::Shaders::PhongGL::Flag::ObjectIdTexture) {
shader_->bindObjectIdTexture(
*(materialData_->attribute<Mn::GL::Texture2D*>("objectIdTexturePtr")));
*(materialData_->attribute<Mn::GL::Texture2D*>("objectIdTexture")));
}

if (skinData_) {
Expand Down
33 changes: 15 additions & 18 deletions src/esp/gfx/GenericDrawable.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,40 +22,37 @@ class GenericDrawable : public Drawable {
//! color for textured buffer and color shader output respectively
explicit GenericDrawable(
scene::SceneNode& node,
Magnum::GL::Mesh* mesh,
Mn::GL::Mesh* mesh,
Drawable::Flags& meshAttributeFlags,
ShaderManager& shaderManager,
const Magnum::ResourceKey& lightSetupKey,
const Magnum::ResourceKey& materialDataKey,
const Mn::ResourceKey& lightSetupKey,
const Mn::ResourceKey& materialDataKey,
DrawableGroup* group = nullptr,
const std::shared_ptr<InstanceSkinData>& skinData = nullptr);

void setLightSetup(const Magnum::ResourceKey& lightSetupKey) override;
void setLightSetup(const Mn::ResourceKey& lightSetupKey) override;
static constexpr const char* SHADER_KEY_TEMPLATE =
"Phong-lights={}-flags={}-joints={}";

protected:
void draw(const Magnum::Matrix4& transformationMatrix,
Magnum::SceneGraph::Camera3D& camera) override;
void draw(const Mn::Matrix4& transformationMatrix,
Mn::SceneGraph::Camera3D& camera) override;

void updateShader();
void updateShaderLightingParameters(
const Magnum::Matrix4& transformationMatrix,
Magnum::SceneGraph::Camera3D& camera);
void updateShaderLightingParameters(const Mn::Matrix4& transformationMatrix,
Mn::SceneGraph::Camera3D& camera);

Magnum::ResourceKey getShaderKey(Magnum::UnsignedInt lightCount,
Magnum::Shaders::PhongGL::Flags flags,
Mn::UnsignedInt jointCount) const;
Mn::ResourceKey getShaderKey(Mn::UnsignedInt lightCount,
Mn::Shaders::PhongGL::Flags flags,
Mn::UnsignedInt jointCount) const;

// shader parameters
ShaderManager& shaderManager_;
Magnum::Resource<Magnum::GL::AbstractShaderProgram, Magnum::Shaders::PhongGL>
shader_;
Magnum::Resource<Magnum::Trade::MaterialData,
Magnum::Trade::PhongMaterialData>
Mn::Resource<Mn::GL::AbstractShaderProgram, Mn::Shaders::PhongGL> shader_;
Mn::Resource<Mn::Trade::MaterialData, Mn::Trade::PhongMaterialData>
materialData_;
Magnum::Resource<LightSetup> lightSetup_;
Magnum::Shaders::PhongGL::Flags flags_;
Mn::Resource<LightSetup> lightSetup_;
Mn::Shaders::PhongGL::Flags flags_;
std::shared_ptr<InstanceSkinData> skinData_;
Cr::Containers::Array<Mn::Matrix4> jointTransformations_;
};
Expand Down
Loading

0 comments on commit 026dfb1

Please sign in to comment.