Skip to content

Commit

Permalink
Browse files Browse the repository at this point in the history
#5571: Add ITableDefinition interface for table decls to make them un…
…it-testable. Add more test coverage (tests are failing).
  • Loading branch information
codereader committed Mar 23, 2021
1 parent 11d8e0c commit d7161cb
Show file tree
Hide file tree
Showing 10 changed files with 63 additions and 32 deletions.
18 changes: 18 additions & 0 deletions include/ishaders.h
Expand Up @@ -433,6 +433,21 @@ inline std::ostream& operator<< (std::ostream& os, const Material* m)

typedef std::function<void(const std::string&)> ShaderNameCallback;

// Represents a table declaration in the .mtr files
class ITableDefinition
{
public:
using Ptr = std::shared_ptr<ITableDefinition>;

virtual ~ITableDefinition() {}

// The name of this table
virtual const std::string& getName() const = 0;

// Retrieve a value from this table, respecting the clamp and snap flags
virtual float getValue(float index) = 0;
};

const char* const MODULE_SHADERSYSTEM = "MaterialManager";

/**
Expand Down Expand Up @@ -541,6 +556,9 @@ class MaterialManager
// Creates a named, internal material for debug/testing etc.
// Used by shaders without corresponding material declaration, like entity wireframe shaders
virtual MaterialPtr createDefaultMaterial(const std::string& name) = 0;

// Tries to find the named table, returns an empty reference if nothing found
virtual ITableDefinition::Ptr getTable(const std::string& name) = 0;
};

inline MaterialManager& GlobalMaterialManager()
Expand Down
2 changes: 1 addition & 1 deletion radiantcore/shaders/Doom3ShaderSystem.cpp
Expand Up @@ -306,7 +306,7 @@ MaterialPtr Doom3ShaderSystem::createDefaultMaterial(const std::string& name)
return std::make_shared<CShader>(name, _library->getEmptyDefinition(), true);
}

TableDefinitionPtr Doom3ShaderSystem::getTableForName(const std::string& name)
ITableDefinition::Ptr Doom3ShaderSystem::getTable(const std::string& name)
{
ensureDefsLoaded();

Expand Down
2 changes: 1 addition & 1 deletion radiantcore/shaders/Doom3ShaderSystem.h
Expand Up @@ -111,7 +111,7 @@ class Doom3ShaderSystem :
MaterialPtr createDefaultMaterial(const std::string& name) override;

// Look up a table def, return NULL if not found
TableDefinitionPtr getTableForName(const std::string& name);
ITableDefinition::Ptr getTable(const std::string& name);

public:
sigc::signal<void> signal_activeShadersChanged() const override;
Expand Down
8 changes: 4 additions & 4 deletions radiantcore/shaders/ShaderExpression.cpp
Expand Up @@ -314,9 +314,9 @@ class ShaderExpressionParser
else
{
// Check if this keyword is a material lookup table
TableDefinitionPtr table = GetShaderSystem()->getTableForName(token);
auto table = GetShaderSystem()->getTable(token);

if (table != NULL)
if (table)
{
// Got it, this is a table name
_tokeniser.nextToken(); // valid token, exhaust, this hasn't been done by the caller yet
Expand All @@ -327,7 +327,7 @@ class ShaderExpressionParser
// The lookup expression itself has to be parsed afresh, enter recursion
auto lookupValue = getExpression();

if (lookupValue == NULL)
if (!lookupValue)
{
throw new parser::ParseException("Missing or invalid expression in table lookup operator[]");
}
Expand Down Expand Up @@ -455,7 +455,7 @@ IShaderExpression::Ptr ShaderExpression::createMultiplication(const IShaderExpre
return std::make_shared<expressions::MultiplyExpression>(a, b);
}

IShaderExpression::Ptr ShaderExpression::createTableLookup(const TableDefinitionPtr& table, const IShaderExpression::Ptr& lookup)
IShaderExpression::Ptr ShaderExpression::createTableLookup(const ITableDefinition::Ptr& table, const IShaderExpression::Ptr& lookup)
{
return std::make_shared<expressions::TableLookupExpression>(table, lookup);
}
Expand Down
6 changes: 3 additions & 3 deletions radiantcore/shaders/ShaderExpression.h
Expand Up @@ -105,7 +105,7 @@ class ShaderExpression :
static IShaderExpression::Ptr createConstant(float constantValue);
static IShaderExpression::Ptr createAddition(const IShaderExpression::Ptr& a, const IShaderExpression::Ptr& b);
static IShaderExpression::Ptr createMultiplication(const IShaderExpression::Ptr& a, const IShaderExpression::Ptr& b);
static IShaderExpression::Ptr createTableLookup(const TableDefinitionPtr& table, const IShaderExpression::Ptr& lookup);
static IShaderExpression::Ptr createTableLookup(const ITableDefinition::Ptr& table, const IShaderExpression::Ptr& lookup);

virtual std::string getExpressionString() override
{
Expand Down Expand Up @@ -285,12 +285,12 @@ class TableLookupExpression :
public ShaderExpression
{
private:
TableDefinitionPtr _tableDef;
ITableDefinition::Ptr _tableDef;
IShaderExpression::Ptr _lookupExpr;

public:
// Pass the table and the expression used to perform the lookup
TableLookupExpression(const TableDefinitionPtr& tableDef,
TableLookupExpression(const ITableDefinition::Ptr& tableDef,
const IShaderExpression::Ptr& lookupExpr) :
ShaderExpression(),
_tableDef(tableDef),
Expand Down
9 changes: 4 additions & 5 deletions radiantcore/shaders/ShaderLibrary.cpp
Expand Up @@ -149,17 +149,16 @@ void ShaderLibrary::foreachShader(const std::function<void(const CShaderPtr&)>&
}
}

TableDefinitionPtr ShaderLibrary::getTableForName(const std::string& name)
ITableDefinition::Ptr ShaderLibrary::getTableForName(const std::string& name)
{
TableDefinitions::const_iterator i = _tables.find(name);
auto i = _tables.find(name);

return i != _tables.end() ? i->second : TableDefinitionPtr();
return i != _tables.end() ? i->second : ITableDefinition::Ptr();
}

bool ShaderLibrary::addTableDefinition(const TableDefinitionPtr& def)
{
std::pair<TableDefinitions::iterator, bool> result = _tables.insert(
TableDefinitions::value_type(def->getName(), def));
auto result = _tables.emplace(def->getName(), def);

return result.second;
}
Expand Down
2 changes: 1 addition & 1 deletion radiantcore/shaders/ShaderLibrary.h
Expand Up @@ -64,7 +64,7 @@ class ShaderLibrary
void foreachShader(const std::function<void(const CShaderPtr&)>& func);

// Look up a table def, return NULL if not found
TableDefinitionPtr getTableForName(const std::string& name);
ITableDefinition::Ptr getTableForName(const std::string& name);

// Method for adding tables, returns FALSE if a def with the same name already exists
bool addTableDefinition(const TableDefinitionPtr& def);
Expand Down
3 changes: 2 additions & 1 deletion radiantcore/shaders/TableDefinition.h
Expand Up @@ -10,7 +10,8 @@ namespace shaders
// Syntax:
// table <tablename> { [snap] [clamp] { <data>, <data>, ... } }

class TableDefinition
class TableDefinition :
public ITableDefinition
{
private:
// The block name
Expand Down
4 changes: 2 additions & 2 deletions radiantcore/shaders/TextureMatrix.cpp
Expand Up @@ -111,8 +111,8 @@ void TextureMatrix::applyTransformation(const IShaderLayer::Transformation& tran
break;
case IShaderLayer::TransformType::Rotate:
{
auto sinTable = GetShaderSystem()->getTableForName("sinTable");
auto cosTable = GetShaderSystem()->getTableForName("cosTable");
auto sinTable = GetShaderSystem()->getTable("sinTable");
auto cosTable = GetShaderSystem()->getTable("cosTable");

if (!sinTable || !cosTable)
{
Expand Down
41 changes: 27 additions & 14 deletions test/Materials.cpp
Expand Up @@ -112,21 +112,34 @@ TEST_F(MaterialsTest, IdentifyAmbientLight)

TEST_F(MaterialsTest, MaterialTableLookup)
{
auto material = GlobalMaterialManager().getMaterial("textures/parsertest/expressions/sinTableLookup");
auto table = GlobalMaterialManager().getTable("sinTable");

auto stage = material->getAllLayers().front();

// Set time to 5008 seconds, this is the value I happened to run into when debugging this in the engine
stage->evaluateExpressions(5008);

EXPECT_FLOAT_EQ(stage->getAlphaTest(), -0.00502608204f);

material = GlobalMaterialManager().getMaterial("textures/parsertest/expressions/cosTableLookup");

stage = material->getAllLayers().front();
stage->evaluateExpressions(1000);

EXPECT_FLOAT_EQ(stage->getAlphaTest(), 0.999998093f);
constexpr std::pair<float, float> testCases[]
{
{ -9.400000f, -0.587745f },
{ -1.000000f, 0.000000f },
{ -0.355500f, -0.788223f },
{ -0.000025f, -0.000157f },
{ 0.000000f, 0.000000f },
{ 0.000025f, 0.000157f },
{ 0.050000f, 0.309003f },
{ 0.250000f, 1.000000f },
{ 0.332200f, 0.869553f },
{ 0.700020f, -0.951048f },
{ 0.999980f, -0.000126f },
{ 1.000000f, 0.000000f },
{ 1.002000f, 0.012565f },
{ 1.800000f, -0.951010f },
{ 2.300000f, 0.951010f },
{ 60.500000f, 0.000000f },
{ 100.230003f, 0.992086f }
};

for (auto testcase : testCases)
{
EXPECT_NEAR(table->getValue(testcase.first), testcase.second, TestEpsilon) << "Lookup failed: "
<< table->getName() << "[" << testcase.first << "] = " << table->getValue(testcase.first) << ", but should be " << testcase.second;
}
}

TEST_F(MaterialsTest, MaterialRotationEvaluation)
Expand Down

0 comments on commit d7161cb

Please sign in to comment.