-
Notifications
You must be signed in to change notification settings - Fork 439
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
Mesh Data rework #371
Conversation
There was a problem hiding this 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!
How about making E.g. 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. |
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.
That's what I explicitly do not want, as then the |
1dcbd35
to
f1caab3
Compare
Codecov Report
@@ 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
Continue to review full report at Codecov.
|
ad441a1
to
0281b82
Compare
efbe843
to
2813e49
Compare
98c057b
to
b4666a6
Compare
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.
Isn't it great when I discover this five minutes before merging to master?
Current state:
WIP docs: https://doc.magnum.graphics/magnum-meshdata/namespaceMagnum_1_1Trade.html (of interest is the
MeshData
class andDataChunkHeader
describing the new*.blob
file format)What will this do / things to do:
MeshData2D
andMeshData3D
into a single type, blurring the difference between these two (similarly to howAnimationData
got done before), reducing code duplicationcompile()
MeshAttributeType
VertexFormat
enum, expanding the current state with packed / normalized integer formatsGL::DynamicAttribute
PixelFormat
, restrict those in convenienceMeshData
APIsVkType
PixelFormat
AbstractImporter::meshAttributeFor(const std::string&)
MeshAttributeName
is not nearly enough considering all possible new attribs for instancing, per-face/edge/instance/meshlet attribs etc, expand to two bytesMeshlet / mesh shader supportmoved to Scene data serialization #427Make it possible to have justsimply another MeshData instance with ameshletOffsets()
w/omeshletIndices()
, thenindices()
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.MeshPrimitive::Face
,MeshPrimitive::Edge
etc primitive type to distinguish them from each otherAbstractImporter::mesh()
, similarly to Trade: support mip levels in image import #369Normal
attributes, one per-vertex, another per-meshlet. How to distinguish these?A per-attribute flag?-- by putting them into one ofMeshPrimitive::Meshlet
orMeshPrimitive::Instance
MeshData instancesattribute<UnsignedByte[]>()
returning a 2D strided view, querying the array size withattributeArraySize()
)E.g. instead of a binaryisIndexed()
have atype()
returningNonIndexed
,Indexed
,MeshletIndexed
,MeshletIndexedMeshletAttributes
,IndexedMeshletAttributes
, ...? UGH?MeshPrimitive::TrianglesMeshlet
/MeshPrimitive::LinesMeshlet
/MeshPrimitive::PointsMeshlet
instead ofMeshPrimitive::Triangles
etc.Do meshlets actually support anything else than triangles? The NV introduction didn't seem to imply that. If so, we needyes they do*Meshlet
variants of other primitive types.How does meshoptimizer fit into this design? Anything to get inspiration from?Fits nicely.Containers::Array
to avoid this being too painful for hte importersand some "pack this together, but preserve alignment" utilitynow there'sMeshTools::interleavedLayout()
which is ❤️interleave()
that eats and produces MeshData, additionally taking either bytes to reserve for each vertex or a list ofTrade::MeshAttributeData
to mix induplicate()
that takes MeshDatacompile()
working with the new APIsMeshTools::combineAttributes()
that replaces the STL-based insanity incombineIndexedArrays()
removeDuplicates()
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)flipNormals()
,tipsify()
to operate on array views insteadWould need 6 new constructors :(compressIndices()
away fromstd::vector
MeshData
(reduce the type size + add vertex offset)removeDuplicates()
(I think a better variant than the overcomplicatedcombineIndexedArrays()
?)removeDuplicates()
)MeshTools::removeDuplicates()
MeshTools::combineFaceAttributes()
removeDuplicates()
AllowpostponedMeshAttributeData
to have views with negative/zero stride to allow more flexibility in MeshTools, but ban those for MeshData itselfcopy()
was doing the right thing there? investigate & test!)