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

Jump to bottom

Material: Appearance Updates 2 #13792

Merged
merged 1 commit into from
Jun 2, 2024
Merged

Material: Appearance Updates 2 #13792

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.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
59 changes: 59 additions & 0 deletions src/App/Material.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,10 +25,26 @@

#ifndef _PreComp_
#include <cstring>
#include <random>
#endif

#include "Application.h"
#include "Material.h"

// Helper functions to consistently convert between float and long
namespace
{
float fromPercent(long value)
{
return std::roundf(value) / 100.0F;

Check warning on line 39 in src/App/Material.cpp

View workflow job for this annotation

GitHub Actions / Lint / Lint 

narrowing conversion from 'long' to 'float' [bugprone-narrowing-conversions,cppcoreguidelines-narrowing-conversions]
}

long toPercent(float value)
{
return std::lround(100.0 * value);
}
} // namespace

using namespace App;


Expand Down Expand Up @@ -317,4 +333,47 @@
break;
}
}

App::Material Material::getDefaultAppearance()
{
ParameterGrp::handle hGrp =
App::GetApplication().GetParameterGroupByPath("User parameter:BaseApp/Preferences/View");

auto getColor = [hGrp](const char* parameter, App::Color& color) {
uint32_t packed = color.getPackedRGB();
packed = hGrp->GetUnsigned(parameter, packed);
color.setPackedRGB(packed);
};
auto intRandom = [](int min, int max) -> int {
static std::mt19937 generator;
std::uniform_int_distribution<int> distribution(min, max);
return distribution(generator);
};

App::Material mat(App::Material::DEFAULT);
mat.transparency = fromPercent(hGrp->GetInt("DefaultShapeTransparency", 0));
long shininess = toPercent(mat.shininess);
mat.shininess = fromPercent(hGrp->GetInt("DefaultShapeShininess", shininess));

// This is handled in the material code when using the object appearance
bool randomColor = hGrp->GetBool("RandomColor", false);

// diffuse color
if (randomColor) {
float red = static_cast<float>(intRandom(0, 255)) / 255.0F;
float green = static_cast<float>(intRandom(0, 255)) / 255.0F;
float blue = static_cast<float>(intRandom(0, 255)) / 255.0F;
mat.diffuseColor = App::Color(red, green, blue);
}
else {
// Color = (204, 204, 230) = 3435980543UL
getColor("DefaultShapeColor", mat.diffuseColor);
}

getColor("DefaultAmbientColor", mat.ambientColor);
getColor("DefaultEmissiveColor", mat.emissiveColor);
getColor("DefaultSpecularColor", mat.specularColor);

return mat;
}
// NOLINTEND(cppcoreguidelines-avoid-magic-numbers,readability-magic-numbers)
6 changes: 6 additions & 0 deletions src/App/Material.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -131,11 +131,13 @@
Color emissiveColor; /**< Defines the emissive color. */
float shininess;
float transparency;
std::string image;
std::string imagePath;
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the reason for adding image/texture path to the Material class? And why are there two strings?
Material and image/texture handling are two separate things and this is an example of breaking the SRP.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is to support textures as part of materials. It's not used yet, but will be once textures are supported.

It can be removed for now but will only be added back later

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Image is a base64 image, image path is a path to an image file. Only one will be used

Copy link
Contributor Author

Choose a reason for hiding this comment

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

And what is SRP?

Copy link
Member

Choose a reason for hiding this comment

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

"Single responsibility principle" -- though I'm not really sure how it's applicable here, to be honest. Maybe Werner can clarify? It seems to me that the displayed texture (and its file) are pretty central to rendering a material, at least once that feature is implemented.

Copy link
Contributor

Choose a reason for hiding this comment

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

The Material class basically is to store the four colours, transparency and shininess. This information is applied to the SoMaterial node of the OpenInventor (OI) library. The SoMaterial class doesn't know anything about images or textures.

In OI textures are basically handled by SoTexture2 and SoTexture3 and images by SoImage.
For advanced handling of textures OI has around 20 further classes to control coordinates, transformations, reflections, mapping and many more things.

Sooner or later some of the additional options will be needed to fine-tune the rendering of textures.

The result by having all texture related parameters in the Material class is that all other classes in FC that set or get them will have methods to either only handle the colour parts or the texture parts but not both. So, these methods can be divided into two parts with no cohesion which IMO can be considered as breaking the SRP.

To avoid this situation material and texture handling should be kept strictly separated.

std::string uuid;
// NOLINTEND
//@}

bool operator==(const Material& m) const

Check warning on line 140 in src/App/Material.h

View workflow job for this annotation

GitHub Actions / Lint / Lint 

parameter name 'm' is too short, expected at least 2 characters [readability-identifier-length]
{
// clang-format off
return _matType == m._matType
Expand All @@ -145,16 +147,20 @@
&& diffuseColor == m.diffuseColor
&& specularColor == m.specularColor
&& emissiveColor == m.emissiveColor
&& image == m.image
&& image == m.imagePath
&& uuid == m.uuid;
// clang-format on
}
bool operator!=(const Material& m) const

Check warning on line 155 in src/App/Material.h

View workflow job for this annotation

GitHub Actions / Lint / Lint 

parameter name 'm' is too short, expected at least 2 characters [readability-identifier-length]
{
return !operator==(m);
}
Material& operator=(const Material& other) = default;
Material& operator=(Material&& other) = default;

static Material getDefaultAppearance();

private:
MaterialType _matType;
};
Expand Down