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

Add operation with wedge to ease the representation of the topology of core mesh, with wedges to handle attribs. #589

Draft
wants to merge 37 commits into
base: release-candidate
Choose a base branch
from

Conversation

dlyr
Copy link
Contributor

@dlyr dlyr commented Jul 9, 2020

  • Please check if the PR fulfills these requirements

First part of this PR has been moved to #595 (add quad meshes for Core and Engine).
Second part of this PR has been moved to #719 (add wedge collapse).

Next steps, sorted by decreasing priority:

  • Topological test collapse on non triangular faces
  • Topological mesh for polymesh: split
  • Topological mesh add/del face/halfedge/edge/vertex which should update wedges ref count
  • Add Catmull-Clark subdivision (from previous implementation, see in code todo's)
  • Refactor UpdateGL to remove duplicated code
  • Test on real data
  • Docs have been added / updated (for bug fixes / features)
  • Tests for the changes have been added (for bug fixes / features)
  • Check picking/wireframe to show only edges corresponding to the core geometry
  • Cleanup history NO FORCE PUSH IN THE MEANTIME (other than rebase)
  • What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
    Add operation with wedge to ease the representation of the topology of core mesh, with wedges to handle attribs.

@dlyr dlyr added Core Related to Ra::Core Engine Related to Ra::Engine enhancement Type of issue/PR: enhancement Feature Request Type of issue: feature request WIP Work in Progress labels Jul 9, 2020
@dlyr dlyr marked this pull request as draft July 9, 2020 23:03
Copy link
Contributor

@nmellado nmellado left a comment

Choose a reason for hiding this comment

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

Nice !

src/Core/Geometry/TriangleMesh.hpp Outdated Show resolved Hide resolved
Copy link
Contributor

@MathiasPaulin MathiasPaulin left a comment

Choose a reason for hiding this comment

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

Nice first shot. Some improvements suggested.

src/Core/Geometry/TriangleMesh.hpp Outdated Show resolved Hide resolved
src/Engine/Renderer/Mesh/Mesh.inl Outdated Show resolved Hide resolved
@STORM-IRIT STORM-IRIT deleted a comment from nmellado Jul 10, 2020
@STORM-IRIT STORM-IRIT deleted a comment from dlyr Jul 10, 2020
@MathiasPaulin
Copy link
Contributor

@nmellado the answer to your question about impact of degenerated triangles on rendering is on the picture post by @dlyr.
Seen from here, I'm sure there is no T vertices on the polymesh and everything is well done. clap clap @dlyr. But I think there are degenerated triangles along the horizontal edge just abode the bottom. If, instead of rendering flat color, you have to interpolate attributes to compute smooth shading, you'll have interpolation artifacts along that edges. degenerated triangles will be removed by the rasterizer and you will see discontinuity in the attribute interpolation.

@dlyr
Copy link
Contributor Author

dlyr commented Jul 10, 2020

This kind of artifact.

Screenshot from 2020-07-10 22-45-52

Note that it is also present in "simple quads" and are not linked to degenerated triangles, just to thin triangles as seen here:
Screenshot from 2020-07-10 22-48-16

@MathiasPaulin
Copy link
Contributor

Thin triangles are bad for rendering. More than computation precisions that degrade as the triangles become thin, the most proeminent problem relies to interpolation along the triangles in a mesh : piecewise linear approximation of non linear function over the mesh exhibit large errors and continuity issues. That's why a good mesh for rendering is a mesh that maximise the minimum triangle area and that maximise also the minimum internal angle on the triangle mesh (Delaunay triangulation does this).

@dlyr dlyr changed the title Quad mesh Quad/poly meshes Jul 12, 2020
@STORM-IRIT STORM-IRIT deleted a comment from dlyr Jul 15, 2020
@nmellado
Copy link
Contributor

PolyMesh loading has been tested using this file: cube_quads.zip

@dlyr dlyr mentioned this pull request Jul 22, 2020
6 tasks
@dlyr dlyr force-pushed the quad-mesh branch 2 times, most recently from 7258033 to b10dcbc Compare July 22, 2020 15:46
nmellado added a commit that referenced this pull request Jul 23, 2020
First step of QuadMesh 
Remaining steps: #589
@nmellado
Copy link
Contributor

Codacy Here is an overview of what got changed by this pull request:

Issues
======
- Added 4
           

Clones added
============
- tests/unittest/Core/topomesh.cpp  3
- tests/ExampleApps/DrawPrimitivesApp/minimalradium.cpp  3
         

Clones removed
==============
+ src/Core/Geometry/TopologicalMesh.hpp  -10
+ src/Core/Geometry/TopologicalMesh.cpp  -10
         

See the complete overview on Codacy

auto quad = VectorNui( 4 );
quad << 0, 1, 2, 3;
auto hepta = VectorNui( 7 );
hepta << 3, 2, 4, 5, 6, 7, 8;
Copy link
Contributor

Choose a reason for hiding this comment

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

Codacy found an issue: Found suspicious operator ','

@MathiasPaulin
Copy link
Contributor

Is this draft PR still needed ?

@dlyr
Copy link
Contributor Author

dlyr commented Apr 1, 2021

yes, there is all the TODO's to do

@dlyr dlyr changed the base branch from master to release-candidate September 21, 2022 12:04
@dlyr dlyr changed the title Quad/poly meshes Add operation with wedge to ease the representation of the topology of core mesh, with wedges to handle attribs. Sep 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Core Related to Ra::Core Engine Related to Ra::Engine enhancement Type of issue/PR: enhancement Feature Request Type of issue: feature request WIP Work in Progress
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants