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

MeshTools: adding generateSmoothNormals() and some more things #229

Merged
merged 9 commits into from May 28, 2019

Conversation

3 participants
@mosra
Copy link
Owner

commented Feb 19, 2018

Stashing my brain here so I don't forget the particulars.

Started out as a discussion on the DART integration PR (mosra/magnum-integration#29), with the following code snippet, extracted from DART itself, with a goal to make this testable:

static Eigen::Vector3d normalFromVertex(const dart::dynamics::SoftBodyNode* bn,
                                        const Eigen::Vector3i& face,
                                        std::size_t v)
{
  const Eigen::Vector3d& v0 = bn->getPointMass(face[v])->getLocalPosition();
  const Eigen::Vector3d& v1 = bn->getPointMass(face[(v+1)%3])->getLocalPosition();
  const Eigen::Vector3d& v2 = bn->getPointMass(face[(v+2)%3])->getLocalPosition();

  const Eigen::Vector3d dv1 = v1-v0;
  const Eigen::Vector3d dv2 = v2-v0;
  const Eigen::Vector3d n = dv1.cross(dv2);

  double weight = n.norm()/(dv1.norm()*dv2.norm());
  weight = std::max( -1.0, std::min( 1.0, weight) );

  return n.normalized() * asin(weight);
}

static void computeNormals(std::vector<Eigen::Vector3d>& normals,
                           const dart::dynamics::SoftBodyNode* bn)
{
  for(std::size_t i=0; i<normals.size(); ++i)
    normals[i] = Eigen::Vector3d::Zero();

  for(std::size_t i=0; i<bn->getNumFaces(); ++i)
  {
    const Eigen::Vector3i& face = bn->getFace(i);
    for(std::size_t j=0; j<3; ++j)
      normals[face[j]] += normalFromVertex(bn, face, j);
  }

  for(std::size_t i=0; i<normals.size(); ++i)
    normals[i].normalize();
}

After seeing the snippet I thought calculating smooth normals does not look very complicated, but the angle calculation looked suspicious. So I looked up:

  • a very detailed explanation, but with pseudocode which needs to be reworked to match our API, also quite different from above (as far as i can see, it's calculating the angle in three different cases, while the above lacks surface area weighting)
  • an implementation on SO that looks simple and correct but inefficient (array of normals can be reduced to simple +=, but I'm not sure why I need to calculate three angles instead of just one)

Besides that, this requires renaming a header in MeshTools.

TODO:

  • mention the header renaming in the changelog
  • install the new header, install the old header only if building deprecated
  • don't remove duplicates in generateFlatNormals(), leave that up to the user (and mention that in docs) (or not?) ... or remove them in some better way (reusing the original index array?)
  • integrate flat / smooth normal generation into MeshTools::compile()
  • mention using MeshTools::generate*Normals() in StanfordImporter -- mosra/magnum-plugins@382a72b
  • make magnum-player generate normals if needed -- mosra/magnum-extras@78e6e7c
  • implement MeshTools::generateSmoothNormals(), taking index array and vertex position array, from which to calculate neighboring faces (could reuse / publish the algo from tipsify())
  • document that hard edges can be created simply by not sharing vertices along the edge
  • assimp has some angle limit for smoothing, implement that as well? postponed
  • think about what data to use to properly test for all cases mentioned in the blog above (in particular, generating smooth normals for a cube should result in all of them being the same, just mirrored)

@mosra mosra added this to the 2018.0b milestone Feb 19, 2018

@mosra mosra self-assigned this Feb 19, 2018

@mosra mosra added this to TODO in Math and algorithms via automation Feb 19, 2018

@mosra mosra moved this from TODO to In progress in Math and algorithms Feb 19, 2018

@mosra mosra referenced this pull request Feb 19, 2018

Merged

Dart integration v2 #29

12 of 12 tasks complete

@mosra mosra force-pushed the generate-smooth-normals branch from 8c7d7fe to 8a02af0 Feb 19, 2018

@mosra mosra modified the milestones: 2018.0b, 2018.0c Apr 29, 2018

@mosra mosra modified the milestones: 2018.0c, 2018.0d Aug 27, 2018

@mosra mosra modified the milestones: 2019.01, 2019.0b Dec 29, 2018

mosra added some commits Apr 13, 2019

MeshTools: make a version of duplicate() not dependent on std::vector.
Provide a version taking StridedArrayView instead, taking an arbitrary
view and also arbitrary index type. Also introduce duplicateInto() which
doesn't even allocate, but rather writes the output to pre-existing
location.
MeshTools: de-STL-ify generateFlatNormals().
Also move it to a new GenerateNormals.h header so we can easily add
generateSmoothNormals() to it. The old API and header is deprecated and
will be removed in the future. I can't be bothered rewriting the old
code using the new thing, so it's preserved there as a mausoleum until
it gets finally nuked from the orbit.
MeshTools: added generateSmoothNormals().
This was a *fun* algorithmic exercise. Seriously.
MeshTools: Primitives are now always needed for the tests.
The new generateSmoothNormals() needs it.

@mosra mosra force-pushed the generate-smooth-normals branch from 8a02af0 to 27f0263 May 27, 2019

mosra added some commits May 27, 2019

MeshTools: benchmark generate{Flat,Smooth}Normals().
There's some room for improvement so I want to be able to see the
difference.
MeshTools: cache angles and cross product in generateSmoothNormals().
It allocates one more array, but that speeds up the calculation to twice
as fast. Before the benchmark was around 1 ms for flat normals and 12 ms
for smooth, now it's 6 ms for smooth. There is probably more I could do
(I feel like I could save at least one more normalization), however this
is good enough for now.

@mosra mosra force-pushed the generate-smooth-normals branch from 27f0263 to 5d2cf7e May 27, 2019

@mosra mosra changed the title [WIP] MeshTools: adding generateSmoothNormals() and some more things MeshTools: adding generateSmoothNormals() and some more things May 27, 2019

@mosra

This comment has been minimized.

Copy link
Owner Author

commented May 27, 2019

This was a fun algorithmic exercise -- and I dare to say the implementation is much more efficient than what DART is attempting to do in the above snippet. Waiting on the CIs to get green, then it goes to master! :shipit:

@costashatz I don't really remember the original discussion, is there any place in DartIntegration where this could get used?

@costashatz

This comment has been minimized.

Copy link
Contributor

commented May 28, 2019

@costashatz I don't really remember the original discussion, is there any place in DartIntegration where this could get used?

Yes of course! It can be used for the Soft objects: right now, we are creating the faces twice, see here. I am putting it on my TO-DO list.. Thanks for reminding..

@mosra

This comment has been minimized.

Copy link
Owner Author

commented May 28, 2019

As you said there, that would still need to implement some facet orientation fixer, right? Or did DART get fixed in the meantime? The thing igl uses (paper) is

our method is very simple

but by simple they mean it depends on the whole of Embree 😅 ... so I guess we would need to implement something actually simple. Another option I can think of is, instead of duplicating all faces, making the Phong shader optionally double-sided, but that would be even less performant I think.

@mosra mosra merged commit 5d2cf7e into master May 28, 2019

3 of 4 checks passed

continuous-integration/travis-ci/push The Travis CI build is in progress
Details
continuous-integration/appveyor/branch AppVeyor build succeeded
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

Math and algorithms automation moved this from In progress to Done May 28, 2019

@mosra mosra deleted the generate-smooth-normals branch May 28, 2019

@codecov-io

This comment has been minimized.

Copy link

commented May 28, 2019

Codecov Report

Merging #229 into master will decrease coverage by 2.96%.
The diff coverage is 97.2%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #229      +/-   ##
==========================================
- Coverage    74.3%   71.34%   -2.97%     
==========================================
  Files         346      347       +1     
  Lines       16430    17542    +1112     
==========================================
+ Hits        12209    12515     +306     
- Misses       4221     5027     +806
Impacted Files Coverage Δ
src/Magnum/MeshTools/CompressIndices.cpp 93.75% <ø> (-2.68%) ⬇️
src/Magnum/MeshTools/Compile.h 100% <100%> (ø)
src/Magnum/MeshTools/Duplicate.h 100% <100%> (ø) ⬆️
src/Magnum/MeshTools/Compile.cpp 79.31% <94%> (+4.78%) ⬆️
src/Magnum/MeshTools/GenerateNormals.cpp 98.71% <98.71%> (ø)
src/Magnum/DebugTools/BufferData.cpp 80% <0%> (-20%) ⬇️
src/Magnum/Audio/Source.h 82.85% <0%> (-17.15%) ⬇️
src/Magnum/GL/Implementation/RendererState.h 33.33% <0%> (-16.67%) ⬇️
src/Magnum/Trade/CameraData.cpp 83.33% <0%> (-16.67%) ⬇️
src/Magnum/GL/AbstractShaderProgram.h 75% <0%> (-16.31%) ⬇️
... and 130 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 8110de3...5d2cf7e. Read the comment docs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.