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

Improve stream handling in Viewer and GraphEditor #1517

Closed
wants to merge 23 commits into from
Closed
Show file tree
Hide file tree
Changes from 16 commits
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
9c8b357
Modify glTF loader to load in multiple uv streams.
kwokcb Sep 6, 2023
5f3ab58
Change MSL to call getStream with fallback option turned on.
kwokcb Sep 6, 2023
2581bf3
Fix return value for some streams. Fix docs.
kwokcb Sep 7, 2023
1ea6908
Merge branch 'main' into multi_stream
jstone-lucasfilm Sep 11, 2023
ebbb765
Clarify input name
jstone-lucasfilm Sep 12, 2023
cf23227
Clarify input name
jstone-lucasfilm Sep 12, 2023
2510769
Remove extra newlines
jstone-lucasfilm Sep 12, 2023
8170c20
Cleanup code. Note this is not required for MSL as this check does no…
kwokcb Sep 14, 2023
1a4d2cd
Merge branch 'main' into multi_stream
jstone-lucasfilm Oct 7, 2023
1caad7a
Minor simplification
jstone-lucasfilm Oct 7, 2023
003379e
Merge branch 'main' into multi_stream
jstone-lucasfilm Oct 24, 2023
1511967
Merge branch 'main' into multi_stream
jstone-lucasfilm Dec 2, 2023
fdef59e
Merge branch 'AcademySoftwareFoundation:main' into multi_stream
kwokcb Dec 5, 2023
a72fb91
Review update to remove unneeded call with default arguments.
kwokcb Dec 5, 2023
5183320
Merge branch 'main' into multi_stream
jstone-lucasfilm Dec 12, 2023
4c73377
Merge branch 'main' into multi_stream
jstone-lucasfilm Jan 5, 2024
9cfb948
Change to perform existence test outside of the "generate" functions …
kwokcb Jan 30, 2024
9a0d7f0
Merge (#50)
kwokcb Jan 30, 2024
32a6f3a
Merge branch 'main' into multi_stream
jstone-lucasfilm Feb 2, 2024
14c723e
Remove default arguments
jstone-lucasfilm Feb 2, 2024
cfaec6d
Remove extra newline
jstone-lucasfilm Feb 2, 2024
c21337d
Remove unused temporary
jstone-lucasfilm Feb 2, 2024
92439dd
Merge branch 'dev_1.39' into multi_stream
kwokcb Mar 16, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 0 additions & 15 deletions source/MaterialXGraphEditor/RenderView.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -81,21 +81,6 @@ void applyModifiers(mx::DocumentPtr doc, const DocumentModifiers& modifiers)
}
}
}

// Remap unsupported texture coordinate indices.
for (mx::ElementPtr elem : doc->traverseTree())
{
mx::NodePtr node = elem->asA<mx::Node>();
if (node && node->getCategory() == "texcoord")
{
mx::InputPtr index = node->getInput("index");
mx::ValuePtr value = index ? index->getValue() : nullptr;
if (value && value->isA<int>() && value->asA<int>() != 0)
{
index->setValue(0);
}
}
}
}

void RenderView::setDocument(mx::DocumentPtr document)
Expand Down
9 changes: 3 additions & 6 deletions source/MaterialXRender/CgltfLoader.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -267,6 +267,7 @@ bool CgltfLoader::load(const FilePath& filePath, MeshList& meshList, bool texcoo
MeshStreamPtr texcoordStream = nullptr;
MeshStreamPtr vec4TangentStream = nullptr;
int colorAttrIndex = 0;
int texcoordIndex = 0;

// Read in vertex streams
for (cgltf_size prim = 0; prim < primitive->attributes_count; prim++)
Expand All @@ -277,12 +278,7 @@ bool CgltfLoader::load(const FilePath& filePath, MeshList& meshList, bool texcoo
{
continue;
}
// Only load one stream of each type for now.
cgltf_int streamIndex = attribute->index;
if (streamIndex != 0)
{
continue;
}

// Get data as floats
cgltf_size floatCount = cgltf_accessor_unpack_floats(accessor, NULL, 0);
Expand Down Expand Up @@ -335,14 +331,15 @@ bool CgltfLoader::load(const FilePath& filePath, MeshList& meshList, bool texcoo
}
else if (isTexCoordStream)
{
texcoordStream = MeshStream::create("i_" + MeshStream::TEXCOORD_ATTRIBUTE + "_0", MeshStream::TEXCOORD_ATTRIBUTE, 0);
texcoordStream = MeshStream::create("i_" + MeshStream::TEXCOORD_ATTRIBUTE + "_" + std::to_string(texcoordIndex), MeshStream::TEXCOORD_ATTRIBUTE, texcoordIndex);
mesh->addStream(texcoordStream);
if (vectorSize == 2)
{
texcoordStream->setStride(MeshStream::STRIDE_2D);
desiredVectorSize = 2;
}
geomStream = texcoordStream;
texcoordIndex++;
}
else
{
Expand Down
152 changes: 147 additions & 5 deletions source/MaterialXRender/Mesh.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
//

#include <MaterialXRender/Mesh.h>
#include <MaterialXCore/Util.h>

#include <limits>
#include <map>
Expand Down Expand Up @@ -42,8 +43,17 @@ Mesh::Mesh(const string& name) :

MeshStreamPtr Mesh::generateNormals(MeshStreamPtr positionStream)
{
const string normalStreamName = "i_" + MeshStream::NORMAL_ATTRIBUTE;

// Return if already exists
MeshStreamPtr normalStream = getStream(normalStreamName);
if (normalStream)
{
return normalStream;
}
jstone-lucasfilm marked this conversation as resolved.
Show resolved Hide resolved

// Create the normal stream.
MeshStreamPtr normalStream = MeshStream::create("i_" + MeshStream::NORMAL_ATTRIBUTE, MeshStream::NORMAL_ATTRIBUTE, 0);
normalStream = MeshStream::create(normalStreamName, MeshStream::NORMAL_ATTRIBUTE, 0);
normalStream->resize(positionStream->getSize());

// Iterate through partitions.
Expand Down Expand Up @@ -78,8 +88,18 @@ MeshStreamPtr Mesh::generateNormals(MeshStreamPtr positionStream)

MeshStreamPtr Mesh::generateTextureCoordinates(MeshStreamPtr positionStream)
{
const string texcoordStreamName = "i_" + MeshStream::TEXCOORD_ATTRIBUTE + "_0";

// Return if already exists
MeshStreamPtr texcoordStream = getStream(texcoordStreamName);
if (texcoordStream)
{
return texcoordStream;
}

// Create stream from x,y of position stream
texcoordStream = MeshStream::create(texcoordStreamName, MeshStream::TEXCOORD_ATTRIBUTE, 0);
size_t vertexCount = positionStream->getData().size() / MeshStream::STRIDE_3D;
MeshStreamPtr texcoordStream = MeshStream::create("i_" + MeshStream::TEXCOORD_ATTRIBUTE + "_0", MeshStream::TEXCOORD_ATTRIBUTE, 0);
texcoordStream->setStride(MeshStream::STRIDE_2D);
texcoordStream->resize(vertexCount);
std::fill(texcoordStream->getData().begin(), texcoordStream->getData().end(), 0.0f);
Expand All @@ -89,6 +109,15 @@ MeshStreamPtr Mesh::generateTextureCoordinates(MeshStreamPtr positionStream)

MeshStreamPtr Mesh::generateTangents(MeshStreamPtr positionStream, MeshStreamPtr normalStream, MeshStreamPtr texcoordStream)
{
const string tangentStreamName = "i_" + MeshStream::TANGENT_ATTRIBUTE;

// Return if already exists
MeshStreamPtr tangentStream = getStream(tangentStreamName);
if (tangentStream)
{
return tangentStream;
}

size_t vertexCount = positionStream->getData().size() / positionStream->getStride();
size_t normalCount = normalStream->getData().size() / normalStream->getStride();
size_t texcoordCount = texcoordStream->getData().size() / texcoordStream->getStride();
Expand All @@ -98,7 +127,7 @@ MeshStreamPtr Mesh::generateTangents(MeshStreamPtr positionStream, MeshStreamPtr
}

// Create the tangent stream.
MeshStreamPtr tangentStream = MeshStream::create("i_" + MeshStream::TANGENT_ATTRIBUTE, MeshStream::TANGENT_ATTRIBUTE, 0);
tangentStream = MeshStream::create(tangentStreamName, MeshStream::TANGENT_ATTRIBUTE, 0);
tangentStream->resize(positionStream->getSize());
std::fill(tangentStream->getData().begin(), tangentStream->getData().end(), 0.0f);

Expand Down Expand Up @@ -173,12 +202,21 @@ MeshStreamPtr Mesh::generateTangents(MeshStreamPtr positionStream, MeshStreamPtr

MeshStreamPtr Mesh::generateBitangents(MeshStreamPtr normalStream, MeshStreamPtr tangentStream)
{
const string bitangentStreamName = "i_" + MeshStream::BITANGENT_ATTRIBUTE;

// Return if already exists
MeshStreamPtr bitangentStream = getStream(bitangentStreamName);
if (bitangentStream)
{
return bitangentStream;
}

if (normalStream->getSize() != tangentStream->getSize())
{
return nullptr;
}

MeshStreamPtr bitangentStream = MeshStream::create("i_" + MeshStream::BITANGENT_ATTRIBUTE, MeshStream::BITANGENT_ATTRIBUTE, 0);
bitangentStream = MeshStream::create(bitangentStreamName, MeshStream::BITANGENT_ATTRIBUTE, 0);
bitangentStream->resize(normalStream->getSize());

for (size_t i = 0; i < normalStream->getSize(); i++)
Expand Down Expand Up @@ -218,7 +256,8 @@ void Mesh::mergePartitions()

void Mesh::splitByUdims()
{
MeshStreamPtr texcoords = getStream(MeshStream::TEXCOORD_ATTRIBUTE, 0);
const unsigned int texcoord_stream = 0;
MeshStreamPtr texcoords = getStream(MeshStream::TEXCOORD_ATTRIBUTE, texcoord_stream);
if (!texcoords)
{
return;
Expand Down Expand Up @@ -312,4 +351,107 @@ void MeshStream::transform(const Matrix44& matrix)
}
}

MeshStreamPtr Mesh::getStream(const string& name, bool allowFallback)
{
for (const auto& stream : _streams)
{
if (stream->getName() == name)
{
return stream;
}
}
if (allowFallback)
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure about this design choice, where the formerly const method Mesh::getStream will silently create new streams when data is missing.

As an alternative design, what if we were to leave Mesh::getStream as a const method, and move the stream construction logic into a new Mesh::createStream method. In the two locations where we need fallbacks for missing streams, we would simply check for a null result from Mesh::getStream and then call Mesh::createStream explicitly.

{
const std::string POSITION_NAME("i_" + MeshStream::POSITION_ATTRIBUTE);
MeshStreamPtr positionStream = getStream(POSITION_NAME);
// Fail if there are no positions.
if (!positionStream)
{
return MeshStreamPtr();
}

MeshStreamPtr returnStream = nullptr;
bool needTexCoords = stringStartsWith(name, "i_" + MeshStream::TEXCOORD_ATTRIBUTE);
bool needNormals = stringStartsWith(name, "i_" + MeshStream::NORMAL_ATTRIBUTE);
bool needTangents = stringStartsWith(name, "i_" + MeshStream::TANGENT_ATTRIBUTE);
if (needTangents)
{
needNormals = true;
needTexCoords = true;
}
bool needBiTangents = stringStartsWith(name, "i_" + MeshStream::BITANGENT_ATTRIBUTE);
if (needBiTangents)
{
needNormals = true;
needTangents = true;
needTexCoords = true;
}

// Return texcoord 0. If it does not exist then create it.
MeshStreamPtr texcoordStream = getStream("i_" + MeshStream::TEXCOORD_ATTRIBUTE + "_0");
if (needTexCoords)
{
if (!texcoordStream)
{
texcoordStream = generateTextureCoordinates(positionStream);
if (texcoordStream)
{
addStream(texcoordStream);
}
}
returnStream = texcoordStream;
}

MeshStreamPtr normalStream = nullptr;
if (needNormals)
{
normalStream = generateNormals(positionStream);
if (normalStream)
{
addStream(normalStream);
returnStream = normalStream;
}
}

MeshStreamPtr tangentStream = nullptr;
if (needTangents)
{
tangentStream = generateTangents(positionStream, normalStream, texcoordStream);
if (tangentStream)
{
addStream(tangentStream);
returnStream = tangentStream;
}
}

if (needBiTangents)
{
MeshStreamPtr bitangentStream = generateBitangents(normalStream, tangentStream);
if (bitangentStream)
{
addStream(bitangentStream);
returnStream = bitangentStream;
}
}

// Return position as fallback color stream
if (stringStartsWith(name, "i_" + MeshStream::COLOR_ATTRIBUTE))
{
returnStream = positionStream;
}

return returnStream;
}

return MeshStreamPtr();
}

MeshStreamPtr Mesh::getStream(const string& type, unsigned int index, bool allowFallback)
{
const string streamName = "i_" + type + "_" + std::to_string(index);
MeshStreamPtr foundStream = getStream(streamName, allowFallback);
return foundStream;
}

MATERIALX_NAMESPACE_END

32 changes: 9 additions & 23 deletions source/MaterialXRender/Mesh.h
Original file line number Diff line number Diff line change
Expand Up @@ -275,39 +275,25 @@ class MX_RENDER_API Mesh

/// Get a mesh stream by name
/// @param name Name of stream
/// @param allowFallback If true will attempt to return fallback stream if it does not exist. Default is false.
/// @return Reference to a mesh stream if found
MeshStreamPtr getStream(const string& name) const
{
for (const auto& stream : _streams)
{
if (stream->getName() == name)
{
return stream;
}
}
return MeshStreamPtr();
}
MeshStreamPtr getStream(const string& name, bool allowFallback = false);

/// Get a mesh stream by type and index
/// @param type Type of stream
/// @param index Index of stream
/// @param allowFallback If true will attempt to return a fallback stream if it does not exist. Default is false.
/// @return Reference to a mesh stream if found
MeshStreamPtr getStream(const string& type, unsigned int index) const
{
for (const auto& stream : _streams)
{
if (stream->getType() == type &&
stream->getIndex() == index)
{
return stream;
}
}
return MeshStreamPtr();
}
MeshStreamPtr getStream(const string& type, unsigned int index, bool allowFallback = false);

/// Add a mesh stream
void addStream(MeshStreamPtr stream)
{
for (MeshStreamPtr s : _streams)
Copy link
Member

Choose a reason for hiding this comment

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

This seems like a surprising behavior for Mesh::addStream, where it will silently skip the requested addition when a stream by the same name already exists. In this situation, the caller has no way to know that the mesh stream that they have constructed is not actually referenced or needed by the mesh.

Would it instead be better to thrown an exception in this situation, so that the logic bug leading to this situation can be addressed?

{
if (s->getName() == stream->getName())
return;
}
_streams.push_back(stream);
}

Expand Down
7 changes: 2 additions & 5 deletions source/MaterialXRenderGlsl/GlslProgram.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -255,7 +255,7 @@ void GlslProgram::bindAttribute(const GlslProgram::InputMap& inputs, MeshPtr mes
unsigned int index = input.second->value ? input.second->value->asA<int>() : 0;

unsigned int stride = 0;
MeshStreamPtr stream = mesh->getStream(input.first);
MeshStreamPtr stream = mesh->getStream(input.first, true);
if (!stream)
{
throw ExceptionRenderError("Geometry buffer could not be retrieved for binding: " + input.first + ". Index: " + std::to_string(index));
Expand Down Expand Up @@ -1065,10 +1065,7 @@ int GlslProgram::mapTypeToOpenGLType(const TypeDesc* type)

const GlslProgram::InputMap& GlslProgram::updateAttributesList()
{
if (_attributeList.size() > 0)
{
return _attributeList;
}
_attributeList.clear();

if (_programId == UNDEFINED_OPENGL_RESOURCE_ID)
{
Expand Down
2 changes: 1 addition & 1 deletion source/MaterialXRenderMsl/MslPipelineStateObject.mm
Original file line number Diff line number Diff line change
Expand Up @@ -372,7 +372,7 @@ int GetStrideOfMetalType(MTLDataType type)
unsigned int index = input.second->value ? input.second->value->asA<int>() : 0;

unsigned int stride = 0;
MeshStreamPtr stream = mesh->getStream(input.first);
MeshStreamPtr stream = mesh->getStream(input.first, true);
if (!stream)
{
errors.push_back("Geometry buffer could not be retrieved for binding: " + input.first + ". Index: " + std::to_string(index));
Expand Down
4 changes: 2 additions & 2 deletions source/MaterialXTest/MaterialXRender/RenderUtil.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -478,7 +478,7 @@ void ShaderRenderTester::addAdditionalTestStreams(mx::MeshPtr mesh)

const std::string GEOM_VECTOR2_STREAM_NAME("i_" + mx::MeshStream::GEOMETRY_PROPERTY_ATTRIBUTE + "_geompropvalue_vector2");
mx::MeshFloatBuffer* geomVector2Data = nullptr;
if (!mesh->getStream(GEOM_VECTOR2_STREAM_NAME))
if (!mesh->getStream(GEOM_VECTOR2_STREAM_NAME, false))
{
mx::MeshStreamPtr geomVector2Stream = mx::MeshStream::create(GEOM_VECTOR2_STREAM_NAME, mx::MeshStream::GEOMETRY_PROPERTY_ATTRIBUTE, 1);
geomVector2Data = &(geomVector2Stream->getData());
Expand Down Expand Up @@ -533,7 +533,7 @@ void ShaderRenderTester::addAdditionalTestStreams(mx::MeshPtr mesh)

const std::string GEOM_COLOR4_STREAM_NAME("i_" + mx::MeshStream::GEOMETRY_PROPERTY_ATTRIBUTE + "_geompropvalue_color4");
mx::MeshFloatBuffer* geomColor4Data = nullptr;
if (!mesh->getStream(GEOM_COLOR4_STREAM_NAME))
if (!mesh->getStream(GEOM_COLOR4_STREAM_NAME, false))
{
mx::MeshStreamPtr geomColor4Stream = mx::MeshStream::create(GEOM_COLOR4_STREAM_NAME, mx::MeshStream::GEOMETRY_PROPERTY_ATTRIBUTE, 1);
geomColor4Data = &(geomColor4Stream->getData());
Expand Down
15 changes: 0 additions & 15 deletions source/MaterialXView/Viewer.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -120,21 +120,6 @@ void applyModifiers(mx::DocumentPtr doc, const DocumentModifiers& modifiers)
}
}
}

// Remap unsupported texture coordinate indices.
for (mx::ElementPtr elem : doc->traverseTree())
{
mx::NodePtr node = elem->asA<mx::Node>();
if (node && node->getCategory() == "texcoord")
{
mx::InputPtr index = node->getInput("index");
mx::ValuePtr value = index ? index->getValue() : nullptr;
if (value && value->isA<int>() && value->asA<int>() != 0)
{
index->setValue(0);
}
}
}
}

} // anonymous namespace
Expand Down
Loading