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

Conversion to surface coordinates is broken #567

Closed
hannobraun opened this issue May 11, 2022 · 3 comments · Fixed by #762
Closed

Conversion to surface coordinates is broken #567

hannobraun opened this issue May 11, 2022 · 3 comments · Fixed by #762
Assignees
Labels
topic: core Issues relating to core geometry, operations, algorithms type: bug Something isn't working

Comments

@hannobraun
Copy link
Owner

Currently, there just one type of surface exists: A sweep of a curve (which can be a line or a circle) along a straight path. Conversion of 3-dimensional model coordinates into surface coordinates of these swept surface is broken.

Here's what the code looks like currently:
https://github.com/hannobraun/Fornjot/blob/3d5500e95621ebe5c786a886a11eaaaab04bd618/crates/fj-kernel/src/geometry/surfaces/swept.rs#L40-L51

I think it happens to work, if the curve and the path happen to be orthogonal to each other, but it breaks down when that isn't the case. Here's a unit test that triggers the bug:

#[test]
fn point_to_surface_coords() {
    let cylinder = SweptCurve {
        curve: Curve::Circle(Circle {
            center: Point::from([1., 2., 3.]),
            a: Vector::from([1., 0., 0.]),
            b: Vector::from([0., 1., 0.]),
        }),
        path: Vector::from([2., 3., 5.]),
    };
    let plane = SweptCurve {
        curve: Curve::Line(Line {
            origin: Point::from([1., 2., 3.]),
            direction: Vector::from([2., 3., 5.]),
        }),
        path: Vector::from([3., 5., 8.]),
    };

    for swept in [cylinder, plane] {
        dbg!(&swept);

        verify(&swept, Point::from([-1., -1.]));
        verify(&swept, Point::from([0., 0.]));
        verify(&swept, Point::from([1., 1.]));
        verify(&swept, Point::from([2., 3.]));
    }

    fn verify(swept: &SweptCurve, surface_point: Point<2>) {
        let point = swept.point_from_surface_coords(surface_point);
        let result = swept.point_to_surface_coords(point);

        assert_eq!(result, surface_point);
    }
}

I'm currently working on this, as this is blocking my ongoing work on #42.

@hannobraun hannobraun added type: bug Something isn't working topic: core Issues relating to core geometry, operations, algorithms labels May 11, 2022
@hannobraun hannobraun self-assigned this May 11, 2022
@hannobraun
Copy link
Owner Author

Ah, what I forgot to mention: While it's pretty easy to show this bug (see the unit test above), I can't see any problem in models that result from this.

While I don't fully understand this, here's my best guess: This code is used in the triangulation algorithm, to convert the 3D coordinates from the approximation into 2D coordinates that the triangulation can work with. Points not being where the triangulation algorithm thinks they are will not necessarily result in an incorrect triangulation, as long as these differently positioned points still result in triangles that have the same points.

This also won't cause a problem when converting the triangulation back to 3D, because that conversion is done using the original value.

@geniusisme
Copy link

I'm not an expert, but as as interested bystander, have to say this.

let u = self.curve.point_to_curve_coords(point).t; 
let v = self.path_to_line().point_to_line_coords(point).t; 

Those things cannot be computed independently in general case. They both depend on the path and the curve.

Solution can look like this (pseudocode):

let line_through_point = Line(point, self.path_direction);
// project point on original curve position.
let point_at_curve = intersect(self.curve, line_through_point);
let v = (point - point_at_curve).length();
// important to get coordinate at original position, otherwise we cannot know how curve is positioned relative to the point
let u = self.curve.point_to_curve_coords(point_at_curve).t;

@hannobraun
Copy link
Owner Author

Thank you for your comment, @geniusisme! I think you are correct.

I'm currently working on a cleanup that removes the need for having that functionality around in the first place. If that doesn't work out, I'll try implementing your solution.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic: core Issues relating to core geometry, operations, algorithms type: bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants