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

[DO NOT MERGE] Calculate bezier curve length #799

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

danieldouglas92
Copy link
Contributor

Add the functionality within bezier_curve.cc to determine the total arc-length of the bezier curve across all user-specified coordinates, and the length of the bezier curve between each incremental pair of user-specified coordinates.

@danieldouglas92 danieldouglas92 force-pushed the calculate_bezier_length branch 2 times, most recently from b4d044c to dcd65dc Compare January 21, 2025 08:19
Copy link

github-actions bot commented Jan 21, 2025

Benchmark Main Feature Difference (99.9% CI)
Slab interpolation simple none 1.114 ± 0.009 (s=386) 1.117 ± 0.009 (s=424) +0.1% .. +0.4%
Slab interpolation curved simple none 1.116 ± 0.011 (s=393) 1.116 ± 0.008 (s=416) -0.1% .. +0.3%
Spherical slab interpolation simple none 1.094 ± 0.008 (s=407) 1.095 ± 0.010 (s=418) -0.1% .. +0.3%
Slab interpolation simple curved CMS 1.165 ± 0.013 (s=404) 1.168 ± 0.015 (s=370) -0.0% .. +0.5%
Spherical slab interpolation simple CMS 1.442 ± 0.012 (s=331) 1.421 ± 0.013 (s=300) -1.6% .. -1.2%
Spherical fault interpolation simple none 1.100 ± 0.009 (s=385) 1.103 ± 0.009 (s=435) +0.1% .. +0.4%
Cartesian min max surface 2.535 ± 0.017 (s=181) 2.545 ± 0.020 (s=176) +0.1% .. +0.6%
Spherical min max surface 7.155 ± 0.066 (s=69) 7.179 ± 0.080 (s=59) -0.3% .. +1.0%

@danieldouglas92 danieldouglas92 force-pushed the calculate_bezier_length branch 7 times, most recently from a9ea923 to b8cec55 Compare January 22, 2025 06:23
@danieldouglas92 danieldouglas92 changed the title [WIP] Calculate bezier curve length Calculate bezier curve length Jan 22, 2025
@danieldouglas92
Copy link
Contributor Author

danieldouglas92 commented Jan 22, 2025

At this point I think the code that I have is correct, there was an issue with the existing implementation of the Bezier curve when dealing with points that were co-linear.

If you have points P1 and P2, to compute the Bezier curve two additional control points CP1 and CP2 must be determined which control the shape of the curve. Sometimes, the control points would be outside of the bounds of Points P1 and P2, as illustrated below:

x----o--------x----o
P1 CP1 P2 CP2

However, the control points should lie within on the vector P1P2, not beyond it, like this:
x----o--------o----x
P1 CP1 CP2 P2

This is important because when the control points lie beyond the vector P1P2, the arclength of the Bezier curve is artificially made longer.

Currently, some of the tests are still failing but they are mostly related to tests which utilize coordinates that are colinear (with the exception of the test slab_interpolation_simple_curved_CMS.dat
@MFraters this is ready for a look when you have the time!

Copy link
Member

@MFraters MFraters left a comment

Choose a reason for hiding this comment

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

This looks good to me. thanks for finding and fixing the bug! I have a few small comments, but otherwise I think it is good to go.

@@ -236,4 +236,4 @@ namespace WorldBuilder
} // namespace Features
} // namespace WorldBuilder

#endif
Copy link
Member

Choose a reason for hiding this comment

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

What are you doing here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure what is happening here? This is the same as the current file and I made no changes to it.

include/world_builder/objects/bezier_curve.h Outdated Show resolved Hide resolved
include/world_builder/objects/bezier_curve.h Outdated Show resolved Hide resolved
include/world_builder/objects/bezier_curve.h Outdated Show resolved Hide resolved
source/world_builder/features/interface.cc Show resolved Hide resolved
source/world_builder/objects/bezier_curve.cc Outdated Show resolved Hide resolved
source/world_builder/objects/bezier_curve.cc Outdated Show resolved Hide resolved
source/world_builder/objects/bezier_curve.cc Show resolved Hide resolved
source/world_builder/objects/bezier_curve.cc Show resolved Hide resolved
source/world_builder/objects/bezier_curve.cc Show resolved Hide resolved
@danieldouglas92 danieldouglas92 force-pushed the calculate_bezier_length branch 3 times, most recently from 40b1200 to 8bc48f0 Compare January 23, 2025 09:07
@danieldouglas92 danieldouglas92 force-pushed the calculate_bezier_length branch from 8bc48f0 to f99409b Compare January 23, 2025 09:08
@danieldouglas92
Copy link
Contributor Author

Thanks for the review! I've addressed your comments, it turns out it wasn't slab_interpolation_simple_curved_CMS that was failing, but actually slab_interpolation_simple_CMS that was failing. This means that all the tests that are actually failing currently involve coordinates that are colinear. If you agree that it is ok to update the test results then I will do that in a follow up commit and hopefully that will wrap this up.

@danieldouglas92 danieldouglas92 changed the title Calculate bezier curve length [DO NOT MERGE] Calculate bezier curve length Jan 23, 2025
@danieldouglas92
Copy link
Contributor Author

After talking to @MFraters I've tentatively updated the test results. The test results should be more correct now, because the tests that were failing involved colinear points. However, we want to do a little more checking to make sure that the new code still produces correct results, so do not merge this quite yet.

@danieldouglas92 danieldouglas92 force-pushed the calculate_bezier_length branch from 95536e3 to b178283 Compare January 23, 2025 10:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants