diff --git a/doc/changelog.dox b/doc/changelog.dox index 4119ac766..428575820 100644 --- a/doc/changelog.dox +++ b/doc/changelog.dox @@ -453,8 +453,8 @@ See also: common texture coordinate set as a complement to a per-texture property added in 2020.06 - @ref Trade::MeshData now allows zero and negative strides for better - data layout flexibility. This is however not commonly supported in GPU - APIs. + data layout flexibility. This is however not commonly supported by GPU APIs + and tools like @ref MeshTools::compile() assert in that case. - Added @ref Trade::MeshData::findAttributeId() for an ability to check that an attribute exists and retrieve its ID in a single step, avoiding a double lookup compared to @relativeref{Trade::MeshData,hasAttribute()} + diff --git a/src/Magnum/MeshTools/Compile.cpp b/src/Magnum/MeshTools/Compile.cpp index b6cc272fa..9ceee82f0 100644 --- a/src/Magnum/MeshTools/Compile.cpp +++ b/src/Magnum/MeshTools/Compile.cpp @@ -144,13 +144,19 @@ GL::Mesh compileInternal(const Trade::MeshData& meshData, GL::Buffer&& indices, continue; boundAttributes.set(attribute->location(), true); + /* Negative strides are not supported by GL, zero strides are + understood as tightly packed instead of all attributes having the + same value */ + const Int stride = meshData.attributeStride(i); + CORRADE_ASSERT(stride > 0, + "MeshTools::compile():" << meshData.attributeName(i) << "stride of" << stride << "bytes isn't supported by OpenGL", GL::Mesh{}); + /* For the first attribute move the buffer in, for all others use the reference */ if(vertices.id()) mesh.addVertexBuffer(std::move(vertices), - meshData.attributeOffset(i), meshData.attributeStride(i), - *attribute); + meshData.attributeOffset(i), stride, *attribute); else mesh.addVertexBuffer(verticesRef, meshData.attributeOffset(i), - meshData.attributeStride(i), *attribute); + stride, *attribute); } if(meshData.isIndexed()) { diff --git a/src/Magnum/MeshTools/Compile.h b/src/Magnum/MeshTools/Compile.h index 42dc8f0fb..fac98b79f 100644 --- a/src/Magnum/MeshTools/Compile.h +++ b/src/Magnum/MeshTools/Compile.h @@ -120,6 +120,8 @@ possibly also an index buffer, if the mesh is indexed. - Implementation-specific @ref Magnum::MeshPrimitive values are passed as-is with @ref meshPrimitiveUnwrap(). It's the user responsibility to ensure an implementation-specific value is valid in this context. +- Stride of all attributes is expected to be positive. OpenGL doesn't support + zero and negative strides. If normal generation is not requested, @ref Trade::MeshData::indexData() and @ref Trade::MeshData::vertexData() are uploaded as-is without any further diff --git a/src/Magnum/MeshTools/Test/CompileGLTest.cpp b/src/Magnum/MeshTools/Test/CompileGLTest.cpp index 2c8e04de4..554283253 100644 --- a/src/Magnum/MeshTools/Test/CompileGLTest.cpp +++ b/src/Magnum/MeshTools/Test/CompileGLTest.cpp @@ -106,6 +106,7 @@ struct CompileGLTest: GL::OpenGLTester { void customAttribute(); void unsupportedAttribute(); + void unsupportedAttributeStride(); void implementationSpecificAttributeFormat(); void generateNormalsNoPosition(); @@ -256,8 +257,12 @@ CompileGLTest::CompileGLTest() { &CompileGLTest::renderTeardown); addInstancedTests({&CompileGLTest::customAttribute, - &CompileGLTest::unsupportedAttribute, - &CompileGLTest::implementationSpecificAttributeFormat}, + &CompileGLTest::unsupportedAttribute}, + Containers::arraySize(CustomAttributeWarningData)); + + addTests({&CompileGLTest::unsupportedAttributeStride}); + + addInstancedTests({&CompileGLTest::implementationSpecificAttributeFormat}, Containers::arraySize(CustomAttributeWarningData)); addTests({&CompileGLTest::generateNormalsNoPosition, @@ -1188,6 +1193,28 @@ void CompileGLTest::unsupportedAttribute() { #endif } +void CompileGLTest::unsupportedAttributeStride() { + #ifdef CORRADE_NO_ASSERT + CORRADE_SKIP("CORRADE_NO_ASSERT defined, can't test assertions"); + #endif + + Vector3 data[2]{}; + Trade::MeshData zero{MeshPrimitive::Points, {}, data, { + Trade::MeshAttributeData{Trade::MeshAttribute::Position, Containers::stridedArrayView(data, 1).broadcasted<0>(2)} + }}; + Trade::MeshData negative{MeshPrimitive::Points, {}, data, { + Trade::MeshAttributeData{Trade::MeshAttribute::Normal, Containers::stridedArrayView(data).flipped<0>()} + }}; + + std::ostringstream out; + Error redirectError{&out}; + compile(zero); + compile(negative); + CORRADE_COMPARE(out.str(), + "MeshTools::compile(): Trade::MeshAttribute::Position stride of 0 bytes isn't supported by OpenGL\n" + "MeshTools::compile(): Trade::MeshAttribute::Normal stride of -12 bytes isn't supported by OpenGL\n"); +} + void CompileGLTest::implementationSpecificAttributeFormat() { auto&& instanceData = CustomAttributeWarningData[testCaseInstanceId()]; setTestCaseDescription(instanceData.name);