Optimize barycentric coordinate computation #164
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
* Makes geodesic path tracing ~100x faster
nmwsharp
reviewed
Nov 19, 2023
| @@ -23,7 +23,6 @@ double displacementLength2(Vector3 displacement, Vector3 triangleLengths); | |||
| double displacementLength(Vector3 displacement, Vector3 triangleLengths); | |||
|
|
|||
| // Convert between cartesian and barycentric coordinates | |||
| Vector3 cartesianVectorToBarycentric(const std::array<Vector2, 3>& vertCoords, Vector2 faceVec); | |||
|
Hi! Thanks for opening this. I'm interested in merging this, the fast method seems quite nice. I am a liiiiitle wary because the existing colPivQR approach was very extensively numerically tested as part of our intrinsic triangulations work. The explicit version here could be more stable or less stable, but either way I am a bit reluctant to merge it in without repeating those tests (and I don't have time to repeat those tests). What if instead we could just merge it as an alternative option, for now? As a new |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
In my application which traces thousands of geodesic paths, I found that the main bottleneck was solving the system of equations to convert cartesian -> barycentric coordinates.
I found a specialized solution for performing the conversion, and swapping it in produces the same results as the Eigen solver while doing so very much faster.
I totally understand if you're not interested in taking this change since it pulls code from a third party source and might have different behavior in case of weird things like degenerate/invalid triangles, NaNs, etc. but I thought I'd put it up to see if you're interested.