Skip to content

MeshPrimitiveUtils now contains methods for serializing triangulated and segmented indices.#2

Merged
BartSiwek merged 5 commits into
masterfrom
a-bsiwek/serialize_triangluated_and_segmented
Aug 16, 2018
Merged

MeshPrimitiveUtils now contains methods for serializing triangulated and segmented indices.#2
BartSiwek merged 5 commits into
masterfrom
a-bsiwek/serialize_triangluated_and_segmented

Conversation

@BartSiwek
Copy link
Copy Markdown
Contributor

MeshPrimitiveUtils now contains methods for serializing triangulated and segmented indices.

@msftclas
Copy link
Copy Markdown

msftclas commented Aug 14, 2018

CLA assistant check
All CLA requirements met.

@BartSiwek
Copy link
Copy Markdown
Contributor Author

Internal build 11299284 passed.
Child builds:
Windows -> 11299287 (OK)
MacOS -> 11299288 (OK)
iOS -> 11299289 (OK)
Android -> 11299290 (OK)

std::vector<uint32_t> GetJointWeights32(const Document& doc, const GLTFResourceReader& reader, const Accessor& accessor);
std::vector<uint32_t> GetJointWeights32_0(const Document& doc, const GLTFResourceReader& reader, const MeshPrimitive& meshPrimitive);

std::string SerializeTriangulatedIndices16(const uint16_t* indices, size_t indexCount, MeshMode mode, BufferBuilder& bufferBuilder);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

These utils shouldn't use BufferBuilder at all. Return a vector of indices and let the user decide what to do with it. Currently it's making too many assumptions about how the user wants to serialize their data, i.e. in it's own BufferView, not interleaved, etc....

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Sure.

Comment thread GLTFSDK/Source/MeshPrimitiveUtils.cpp Outdated
const auto resultIndexCount = 2 + triangleCount;
std::vector<T> result(resultIndexCount);

result[0] = indices[0];
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Doing multiplication and then subtraction to get the index seems really weird to me. Why not do this, which is more like the opposite of the original transform code?

        // indices:
        //     0,1,2
        //     1,3,2
        //     2,3,4

        result.push_back(indices[0]);
        result.push_back(indices[1]);
        for (size_t i = 2; i < triangleCount; i+=3)
        {
            if (i % 2 == 0)
            {
                result.push_back(indices[i]);
            }
            else
            {
                result.push_back(indices[i - 1]);
            }
        }

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

Comment thread GLTFSDK/Source/MeshPrimitiveUtils.cpp Outdated
const auto resultIndexCount = 2 + triangleCount;
std::vector<T> result(resultIndexCount);

result[0] = indices[0];
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Doing multiplication and then subtraction to get the index seems really weird to me. Why not do this, which is more like the opposite of the original transform?

        // indices:
        //     0,1,2
        //     0,2,3
        //     0,3,4

        result.push_back(indices[0]);
        result.push_back(indices[1]);
        for (size_t i = 2; i < triangleCount; i+=3)
        {
            result.push_back(indices[i]);
        }

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

Assert::AreEqual(outputIndices[262139], 0U);
}

GLTFSDK_TEST_METHOD(MeshPrimitiveUtilsTests, MeshPrimitiveUtils_Test_SerializeTriangulatedIndices16_Triangles)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

You also need to add tests which start with a fan/strip/etc, and then do a round-trip by calling both the triangulate and reverse operations and then compare input to output.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Added

@BartSiwek
Copy link
Copy Markdown
Contributor Author

Internal build 11317591 passed.

result[0] = indices[0];
result[1] = indices[1];
result[2] = indices[2];
std::vector<T> result;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

You need to call reserve on this now that we're using push_back.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done.

const size_t triangleCount = indexCount / 3;
const auto resultIndexCount = 2 + triangleCount;
std::vector<T> result(resultIndexCount);
std::vector<T> result;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

You need to call reserve on this now that we're using push_back.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done.

agrittmsft
agrittmsft previously approved these changes Aug 15, 2018
struct Accessor;
struct MeshPrimitive;
struct MorphTarget;
class BufferBuilder;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Can the BufferBuilder forward declaration be removed now?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Good point. Cleaning it up now.

throw GLTFException("Input triangulated triangle strip has fewer than 3 indices.");
}

std::vector<T> result;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Call reserve on the vector(s) to avoid reallocations?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done.

@BartSiwek
Copy link
Copy Markdown
Contributor Author

Internal build 11336263 passed.

@BartSiwek BartSiwek merged commit e978e3a into master Aug 16, 2018
@agrittmsft agrittmsft deleted the a-bsiwek/serialize_triangluated_and_segmented branch August 16, 2018 17:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants