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

Only limit the Bezier curve closest point at the end of the whole curve #523

Conversation

MFraters
Copy link
Member

@MFraters MFraters commented Dec 5, 2023

Looking at issue #522, I realized that the end points of slabs and fault (i.e. bezier curves) are not handled properly. The issues stems form wanting to find the closest point to a line consisting of segments. It can happen that the closest point found is just off the segment. In the interior this may be fine (especially if the derivatives are non-continuous, although I should revisit this in the future again because with Bezier Curves they now should be), but it should not happen at the edges of the full line itself.

This part at the end of line lines can be easily tested for, which is what this pull request does. I tested it with the provided example and this fixes the issue in #522.

This may break tests and I need to do some more testing to look if it breaks anything else it should not break.

Copy link

github-actions bot commented Dec 5, 2023

Benchmark Main Feature Difference (99.9% CI)
Slab interpolation simple none 1.021 ± 0.008 (s=441) 1.020 ± 0.004 (s=444) -0.2% .. +0.1%
Slab interpolation curved simple none 1.026 ± 0.003 (s=459) 1.028 ± 0.004 (s=420) +0.2% .. +0.3%
Spherical slab interpolation simple none 1.261 ± 0.004 (s=340) 1.262 ± 0.011 (s=376) -0.1% .. +0.3%
Slab interpolation simple curved CMS 1.099 ± 0.005 (s=433) 1.099 ± 0.006 (s=389) -0.1% .. +0.1%
Spherical slab interpolation simple CMS 1.859 ± 0.015 (s=235) 1.864 ± 0.005 (s=251) +0.1% .. +0.4%
Spherical fault interpolation simple none 1.270 ± 0.004 (s=350) 1.273 ± 0.005 (s=360) +0.1% .. +0.3%
Cartesian min max surface 2.382 ± 0.021 (s=186) 2.379 ± 0.021 (s=195) -0.4% .. +0.2%
Spherical min max surface 7.353 ± 0.026 (s=60) 7.369 ± 0.027 (s=65) -0.0% .. +0.4%

MFraters added a commit to MFraters/WorldBuilder-1 that referenced this pull request Dec 7, 2023
@MFraters MFraters force-pushed the fix_bezier_curve_endpoint_overreach branch from 58535a8 to 4ba3ad3 Compare December 7, 2023 20:25
@MFraters MFraters added the bug Something isn't working label Dec 7, 2023
MFraters added a commit to MFraters/WorldBuilder-1 that referenced this pull request Dec 7, 2023
@MFraters MFraters force-pushed the fix_bezier_curve_endpoint_overreach branch from 4ba3ad3 to f5e02d1 Compare December 7, 2023 21:25
MFraters added a commit to MFraters/WorldBuilder-1 that referenced this pull request Dec 7, 2023
MFraters added a commit to MFraters/WorldBuilder-1 that referenced this pull request Dec 7, 2023
MFraters added a commit to MFraters/WorldBuilder-1 that referenced this pull request Dec 7, 2023
@MFraters MFraters force-pushed the fix_bezier_curve_endpoint_overreach branch from f5e02d1 to b48c0eb Compare December 7, 2023 21:43
MFraters added a commit to MFraters/WorldBuilder-1 that referenced this pull request Dec 8, 2023
MFraters added a commit to MFraters/WorldBuilder-1 that referenced this pull request Dec 8, 2023
MFraters added a commit to MFraters/WorldBuilder-1 that referenced this pull request Dec 8, 2023
@MFraters MFraters force-pushed the fix_bezier_curve_endpoint_overreach branch from b48c0eb to aabc002 Compare December 8, 2023 06:04
Copy link

codecov bot commented Dec 8, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (c3188ad) 93.46% compared to head (0580460) 93.46%.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##             main     #523   +/-   ##
=======================================
  Coverage   93.46%   93.46%           
=======================================
  Files          92       92           
  Lines        6301     6301           
=======================================
  Hits         5889     5889           
  Misses        412      412           
Files Coverage Δ
source/world_builder/objects/bezier_curve.cc 100.00% <100.00%> (ø)

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c3188ad...0580460. Read the comment docs.

@MFraters MFraters force-pushed the fix_bezier_curve_endpoint_overreach branch from aabc002 to 70cf179 Compare December 8, 2023 06:14
MFraters added a commit to MFraters/WorldBuilder-1 that referenced this pull request Dec 8, 2023
@MFraters
Copy link
Member Author

MFraters commented Dec 8, 2023

After looking into a few changed test results, the provided test case and some of my more complex models, I think this pull request is ready to merge.

@danieldouglas92, could you have a look at this pull request to see if 1. it resolves the issue in #522, and 2. your models still work.

@danieldouglas92
Copy link
Contributor

danieldouglas92 commented Jan 22, 2024

I just tested and it does fix the issue! Below are two screenshots with the same .wb and .grd file with and without the changes in this PR showcasing how the fault behaves linearly at depth and not circular

image

image

@MFraters MFraters force-pushed the fix_bezier_curve_endpoint_overreach branch from 70cf179 to 0580460 Compare January 24, 2024 20:41
@MFraters MFraters merged commit 40655ff into GeodynamicWorldBuilder:main Jan 25, 2024
34 checks passed
danieldouglas92 pushed a commit to danieldouglas92/WorldBuilder that referenced this pull request Feb 15, 2024
danieldouglas92 pushed a commit to danieldouglas92/WorldBuilder that referenced this pull request Feb 15, 2024
danieldouglas92 pushed a commit to danieldouglas92/WorldBuilder that referenced this pull request Feb 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants