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

Mesh Data rework #371

Merged
merged 107 commits into from
Mar 11, 2020
Merged

Mesh Data rework #371

merged 107 commits into from
Mar 11, 2020

Conversation

mosra
Copy link
Owner

@mosra mosra commented Sep 13, 2019

Current state:

image

WIP docs: https://doc.magnum.graphics/magnum-meshdata/namespaceMagnum_1_1Trade.html (of interest is the MeshData class and DataChunkHeader describing the new *.blob file format)

What will this do / things to do:

  • Combine MeshData2D and MeshData3D into a single type, blurring the difference between these two (similarly to how AnimationData got done before), reducing code duplication
    • Need to deprecate the two original types and the old compile()
    • Adapt all code that consumes those to new APIs
    • Add implicit conversion for seamless backward compatibility
  • Expose new unified Importer API for those
    • Need to deprecate related Importer APIs (and make use of the implicit conversion for seamless backward compatibility)
  • Create a MeshAttributeType VertexFormat enum, expanding the current state with packed / normalized integer formats
    • Provide mapping to GL::DynamicAttribute
    • Provide an ability to store implementation-specific values in it, like with PixelFormat, restrict those in convenience MeshData APIs
    • Provide mapping to VkType
    • Document mapping to D3D and Metal formats, like with PixelFormat
  • Better support for generic vertex attributes (reserved IDs in the enum for those)
    • AbstractImporter::meshAttributeFor(const std::string&)
    • One-byte MeshAttributeName is not nearly enough considering all possible new attribs for instancing, per-face/edge/instance/meshlet attribs etc, expand to two bytes
  • Meshlet / mesh shader support moved to Scene data serialization #427
    • Make it possible to have just meshletOffsets() w/o meshletIndices(), then indices() is global again; this can also be abused to group triangles into N-gons (but can't have both because a N-gon could be split across two meshlets and that would complicate things), preserve PLY/OBJ per-face attribs etc. simply another MeshData instance with a MeshPrimitive::Face, MeshPrimitive::Edge etc primitive type to distinguish them from each other
    • Ability to import these "additional mesh data layers" via a second parameter to AbstractImporter::mesh(), similarly to Trade: support mip levels in image import #369
    • Ability to have per-meshlet / per-Ngon / per-instance attributes (for example a normal / base texel offset for each, allowing the per-vertex ones to be less precise; a per-meshlet bounding box etc.) -- this would mean there's e.g. two Normal attributes, one per-vertex, another per-meshlet. How to distinguish these? A per-attribute flag? -- by putting them into one of MeshPrimitive::Meshlet or MeshPrimitive::Instance MeshData instances
    • Since some per-meshlet attributes coming from meshoptimizer are actually arrays, provide a way to store and retrieve those (e.g. via attribute<UnsignedByte[]>() returning a 2D strided view, querying the array size with attributeArraySize())
    • Since meshlet meshes will be very different from normal ones, these need to be designed to prevent a meshlet one being used as a normal one (and then using a wrong local index array, wrong local attributes etc.). E.g. instead of a binary isIndexed() have a type() returning NonIndexed, Indexed, MeshletIndexed, MeshletIndexedMeshletAttributes, IndexedMeshletAttributes, ...? UGH? MeshPrimitive::TrianglesMeshlet / MeshPrimitive::LinesMeshlet / MeshPrimitive::PointsMeshlet instead of MeshPrimitive::Triangles etc.
    • Do meshlets actually support anything else than triangles? The NV introduction didn't seem to imply that. If so, we need *Meshlet variants of other primitive types. yes they do
    • How does meshoptimizer fit into this design? Anything to get inspiration from? Fits nicely.
  • Might need to implement growable Containers::Array to avoid this being too painful for hte importers
    • and some "pack this together, but preserve alignment" utility now there's MeshTools::interleavedLayout() which is ❤️
  • interleave() that eats and produces MeshData, additionally taking either bytes to reserve for each vertex or a list of Trade::MeshAttributeData to mix in
  • duplicate() that takes MeshData
  • compile() working with the new APIs
    • Also the normal generation
    • Ability to pass it a "global" index/vertex buffer for the case where multiple meshes share one data buffer
  • MeshTools::combineAttributes() that replaces the STL-based insanity in combineIndexedArrays()
    • Needs an allocation-less variant of removeDuplicates()
    • Deprecate combineIndex*Arrays()
  • MeshTools::concatenate() that takes a list of MeshData, assumes the same primitive, and combines them together (picking attributes of the first mesh, leaving zeros when the others don't have them, ignoring additional ones)
  • Make STL-less alternatives of all MeshTools
    • deprecate the originals
    • Change flipNormals(), tipsify() to operate on array views instead
  • an option for the MeshAttributeData / MeshIndexData to store relative offsets to help with serialization and cases where layout is known but pointer not
    • Ability to omit vertex count in there also? Would need 6 new constructors :(
    • Make all attribute layouts in Primitives done at compile-time
  • Store the StridedArrayView unpacked to save 10 bytes (by using less bits for stride and vertex count)
  • Do the essential updates needed in MeshTools so we can avoid vectors
  • Port Primitives to it
    • Adapt tests
    • Adapt the primitive generator utility in doc/generated
    • MSVC 2015 has some weirdness where it duplicates constexpr data leading to runtime asserts about views being contained in the data
    • MSVC asserts in MeshDataTest where it should be a graceful assert
  • Port compressIndices() away from std::vector
    • Add a version operating on MeshData (reduce the type size + add vertex offset)
  • Port plugins
    • ObjImporter
      • Needs an integer version of removeDuplicates() (I think a better variant than the overcomplicated combineIndexedArrays()?)
        • And shouldn't compress the indices, leave that on the user (needs non-allocating removeDuplicates())
      • Adapt tests
    • AssimpImporter (we'll need to interleave outselves, slow ugh)
    • OgexImporter (just floats, none of the extra types, it's a text file so who cares),
    • StanfordImporter
      • needs Vector2us etc types, need to add them all, ugh
      • Adapt test data, test for new restrictions
      • Per-face attribute import, optionally turning into per-vertex
        • Needs an integer version of MeshTools::removeDuplicates()
        • Needs MeshTools::combineFaceAttributes()
          • ... which needs an allocation-less not-in-place variant of removeDuplicates()
    • TinyGltfImporter (should be pretty straightforward)
  • Allow MeshAttributeData to have views with negative/zero stride to allow more flexibility in MeshTools, but ban those for MeshData itself postponed
  • Adapt examples
  • Adapt Python bindings
  • Adapt DART integration
  • Proofread docs
    • Ensure the old types are not referenced anymore
  • Interleave fails for non-aligned data on Emscripten even though it shouldn't (I thought copy() was doing the right thing there? investigate & test!)

@mosra mosra added this to the 2019.0c milestone Sep 13, 2019
Copy link
Contributor

@Squareys Squareys left a comment

Choose a reason for hiding this comment

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

I added a couple of comments, just in case they are helpful. Really excited about this!

src/Magnum/Trade/MeshData.cpp Outdated Show resolved Hide resolved
src/Magnum/Trade/MeshData.cpp Outdated Show resolved Hide resolved
src/Magnum/Trade/MeshData.h Outdated Show resolved Hide resolved
@mosra mosra mentioned this pull request Sep 13, 2019
60 tasks
@Squareys
Copy link
Contributor

How about making DataType a char[4] so that it can be used as "magic"?

E.g. "Mg1m" could be "Magnum, version 1, mesh", or "Mg1a" etc. Would a, make the blobs distinguishable and easily identify magnum files as such to help with detection in Any*Importer.

There could also be a file-header in addition to the blob header which suggests number of chunks making detection of broken chungs easier etc.

@mosra
Copy link
Owner Author

mosra commented Sep 13, 2019

How about making DataType a char[4] so that it can be used as "magic"?

Ah, forgot to list the "magic" field in there. Updated. Putting all that into just four bytes seems a bit too crowded, would prevent extensibility with 3rd party chunk types for example.

There could also be a file-header in addition to the blob header which suggests number of chunks

That's what I explicitly do not want, as then the cat would be impossible and you'd need some tool to put them together, extract, or traverse. With the way above (and analogously to RIFF), a file is correct if each chunk points to a (non-OOB) position of the next chunk with correct magic or EOF and I think that could be enough :) Error detection could be further improved by having a CRC field for every chunk for example ... but then why not just zip all that anyway :)

@mosra mosra mentioned this pull request Oct 2, 2019
45 tasks
@mosra mosra force-pushed the meshdata branch 2 times, most recently from 1dcbd35 to f1caab3 Compare November 10, 2019 23:16
@codecov-io
Copy link

codecov-io commented Nov 10, 2019

Codecov Report

Merging #371 into master will increase coverage by 2.34%.
The diff coverage is 98.04%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #371      +/-   ##
==========================================
+ Coverage   72.54%   74.88%   +2.34%     
==========================================
  Files         368      384      +16     
  Lines       19339    21293    +1954     
==========================================
+ Hits        14030    15946    +1916     
- Misses       5309     5347      +38
Impacted Files Coverage Δ
src/Magnum/PixelFormat.h 100% <ø> (ø) ⬆️
src/Magnum/Trade/AbstractImporter.h 100% <ø> (ø) ⬆️
src/Magnum/Trade/MeshData.h 100% <ø> (ø)
src/Magnum/Primitives/Cylinder.h 100% <ø> (ø) ⬆️
src/Magnum/MeshTools/CombineIndexedArrays.cpp 96.49% <ø> (-3.51%) ⬇️
...agnumPlugins/AnySceneImporter/AnySceneImporter.cpp 45.32% <ø> (-0.55%) ⬇️
...gnum/Primitives/Implementation/WireframeSpheroid.h 100% <ø> (ø) ⬆️
src/Magnum/Primitives/Cone.h 100% <ø> (ø) ⬆️
src/Magnum/Trade/MeshData2D.h 100% <ø> (ø) ⬆️
src/MagnumPlugins/ObjImporter/ObjImporter.cpp 93.54% <ø> (+0.21%) ⬆️
... and 101 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 881003d...eaa59cc. Read the comment docs.

mosra added 17 commits March 11, 2020 13:56
Replaces the STL-heavy combineIndexedArrays(), but in a less extremely
horrendous way.
The combineIndexArrays() and combineIndexedArrays() API is replaced with
a more generic combineIndexedAttributes(), and thanks to that we also
don't need STL-based duplicate() and removeDuplicates().
Those work without modifying the input data. The *Indexed and fuzzy
variants are missing as I don't need those right now, but might get
added later.
Less code and complexity than first creating a StridedArrayView in
data() and then extracting offset/stride from there.
Until now, except for an attribute-less index-less mesh, the vertex
count was only implicitly taken from passed attributes, but it was
severely limiting:

 - There was no way to set vertex count for an attribute-less
   indexed mesh, which didn't make sense
 - All code that made non-owning MeshData instances referencing another
   MeshData had to explicitly handle the attribute-less corner case to
   avoid vertex count getting lost
 - Offset-only attributes couldn't be used to specify static layout of
   meshes with dynamic vertex count, causing unnecessary extra
   allocations especially in the Primitives library.
Now possible in all cases, except for grid, where the combination count
is too large to be practical, even more so with the introduction of
tangents in the future.
No functional change.
@mosra mosra marked this pull request as ready for review March 11, 2020 16:45
Isn't it great when I discover this five minutes before merging to
master?
@mosra mosra merged commit 236954a into master Mar 11, 2020
@mosra mosra deleted the meshdata branch March 11, 2020 19:23
@mosra mosra restored the meshdata branch March 11, 2020 19:32
@mosra mosra deleted the meshdata branch March 11, 2020 19:33
@mosra mosra mentioned this pull request Mar 11, 2020
61 tasks
@mosra
Copy link
Owner Author

mosra commented Mar 11, 2020

Merged in current state as the design is stable enough. Zero-copy import, mesh serialization, meshlet support and scene conversion plugin APIs are continued in #240 and #427.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

Successfully merging this pull request may close these issues.

3 participants