STORM-IRIT / Radium-Engine

Research 3D Engine for rendering, animation and processing
https://storm-irit.github.io/Radium-Engine/
Apache License 2.0
100 stars 50 forks source link

Attribute handling for duplicated vertices #242

Closed nmellado closed 3 years ago

nmellado commented 6 years ago

Context

When loading meshes, Radium may merge (or not) duplicated vertices. For instance, a mesh composed by two triangles shared by an edge can be represented either with

  1. Duplicate vertices handling: 4 vertices: one edge is implicitely shared between the two triangles. Vertices of that edges are stored once, and referenced twice, one time by connected face.
  2. NoDuplicate handling: 2x3 vertices: no connectivity information, the vertices are stored two times.

If there is no problem with the second case, the attribute management is misleading in the first case.

To handle duplicated vertices, a duplicate table of size n stores the indices of the vertices defining the mesh. When a vertex is duplicated, it appears 2 times or more in the duplicate table. Note that the number of vertices m (size of the vertex container) always satisfies m <= n, with n-m being the number of duplications.

Why is this a problem

The main problem is related to the attribute management of duplicated vertices:

Impact on loops

For the sake of clarity, here is an example of two different ways to loop over the Mesh vertices.

First, I want to iterate over the original mesh vertices (assuming I don't care about the loading options), which leads to:

for (uint id : *( component->getDuplicateTableOutput())) {
   auto & v = component->getVerticesRw()[id];
   // do stuff
}

Second, If I want to skip the duplicates, I need to do:

for ( auto & v : *(component->getVerticesRw())) {
   // do stuff
}

This can be ok, however we need to provide access to the attributes whatever the loop is, and in a consistent way. This is not the case today, and only the first loop provide access to the normals of the vertex v in the loop (even if we use index-based loop in the second case).

Suggestions and fixes

dlyr commented 6 years ago

I see a solution using OpenMesh : Triangle/FancyMesh are meshes for rendering (I mean with "duplicated point", a vertex is a unique combination of point, normal, uv, ...) and TopologicalMeshes are for mesh manipulation. On TopologicalMesh, every attributes (point, normal, uv ...) are stored on halfedge ... it solves the pbs.

The conversion should be quite fast, but maybe not fast enough to do it every frame. If needed we could have a map (like duplicate table) between one TriangleMesh and one TopologicalMesh, then the update will be fast.

This would be easier to achieve by aslo moving FancyMesh from Plugin to Core

dlyr commented 6 years ago

Is anyone use splitEdge function ? The "ASCII scheme" is wrong since OpenMesh splitEdge do not create faces ! (see doc).

nmellado commented 6 years ago

Currently, nobody uses the topological mesh. But when it is stabilized, yes, we would need such a thing.

Can you, either fix it, or at least add a flag, such as [[deprecated("reason")]] ?

dlyr commented 6 years ago

I will fix it ! but it will take some time

nmellado commented 6 years ago

Sure, no problem, that's great :)

dlyr commented 6 years ago

So my current view is :

One solution to get thing controllable and extensible, is to template Mesh with a VertexData struct, with a default one handling point, normals, (tex coords?) And wrapper for DisplayMesh to send the data to the GPU (wrapper are specialized for each VertexData struct we have). One question arise : what is the purpose of VectorArray ?. do I need to use it for VertexData ?

nmellado commented 6 years ago

TopologicalMeshes have been introduced in PRs #305 and are now stable. Part of this issue has been solved by the new attribute management system introduced by merged PRs #345 and #359. WIP PR #350 adds attribute support for subdivision schemes.

MathiasPaulin commented 5 years ago

Seems that this issue is solved now.

nmellado commented 5 years ago

We still have a duplicate table in the Animation package. I'm keeping the issue open to track this.

stale[bot] commented 4 years ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed in 14 days if no further activity occurs. Thank you for your contributions.

hoshiryu commented 4 years ago

Re-opening since we still have the "duplicate table" for the computation of the mesh normals in the Geometric Skinning pipeline.

dlyr commented 4 years ago

I'm working on wedge to handle vertices attributes instead of storing them on halfedges. One question is do we need to handle attributes add/remove on TopoMesh or could we say that attributes are from CoreMesh, propagated to TopoMesh back and forth, but one could not have attrib managed directly on TopoMesh.

nmellado commented 4 years ago

From my point of view, it is totally fine if we have a first version that does not allow to add/remove attributes in the topomesh. This must be clearly stated in the documentation. Even with this limitation, one can create the attribute with dummy values before generating the topomesh, and use the topomesh to compute the actual values (possible use case would be: curvatures estimation).

If we need this functionality later on, well, we will add it.

stale[bot] commented 4 years ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed in 14 days if no further activity occurs. Thank you for your contributions.

nmellado commented 4 years ago

Note sure this issue is totally solved, we may still have the duplicate table. Reopen.

stale[bot] commented 4 years ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed in 14 days if no further activity occurs. Thank you for your contributions.

dlyr commented 3 years ago

I think this one is fixed, isn't it ?

dlyr commented 3 years ago

at least there isn't any reference to duplicate table in Radium-Engine src.

dlyr commented 3 years ago

I close the issue due ton inactivities, feel free to reopen with a reference to problematic source code.

MathiasPaulin commented 3 years ago

I close the issue due ton inactivities, feel free to reopen with a reference to problematic source code.

There is still duplicate table management in the skinning plugin ... but I'm fine to close this and, as I planned to integrate the skinning into Radium libs, I will try to use wedges instead of duplicate tables ...

nmellado commented 3 years ago

If I may, I would suggest that you open a PR with the current version of the plugin (the one that use the duplicate table) so we can discuss how to switch to wedges, and we can also contribute (you don't have to do it alone).

dlyr commented 3 years ago

yes, skinning plugin is not part (yet) or radium-engine, so the issue is not relevant here anymore.