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

Validation precision issues with large objects #2227

Closed
IamTheCarl opened this issue Feb 19, 2024 · 2 comments
Closed

Validation precision issues with large objects #2227

IamTheCarl opened this issue Feb 19, 2024 · 2 comments

Comments

@IamTheCarl
Copy link
Contributor

Good morning. I started implementing a custom scripting language built around Fornjot. I'll showcase it later when it's more complete.
Two of the key features of this scripting language are that it has type safety revolving around the dimensions of measurements and automatic unit conversion.

The following is an example of how the unit conversion can look. Note that the interpreter is currently configured to convert all inputs to Fornjot as millimeters (this is configurable).

new_region(struct Circle { center = [1m, 2m], radius = 3m })

This would result in Fornjot being fed something similar to this:

Region::circle(
    [1000, 2000],
    3000,
    &mut context.global_resources.fornjot_services,
)

This results in the following validation error:

`Cycle` validation error

Caused by:
	Adjacent `HalfEdge`s are not connected
- End position of first `HalfEdge`: [4000.0, 1999.9999999999993]
- Start position of second `HalfEdge`: [4000.0, 2000.0]
- Distance between vertices: 0.0000000000006821210263296962
- `HalfEdge`s: [
    HalfEdge @ 0x7f0e76dd7010 => HalfEdge {
        path: Circle(
            Circle {
                center: [
                    1000.0,
                    2000.0,
                ],
                a: [
                    3000.0,
                    0.0,
                ],
                b: [
                    0.0,
                    3000.0,
                ],
            },
        ),
        boundary: CurveBoundary {
            inner: [
                [
                    0.0,
                ],
                [
                    6.283185307179586,
                ],
            ],
        },
        curve: Curve @ 0x7f0e481a34a0 => Curve,
        start_vertex: Vertex @ 0x7f0e481a77f0 => Vertex,
    },
    HalfEdge @ 0x7f0e76dd7010 => HalfEdge {
        path: Circle(
            Circle {
                center: [
                    1000.0,
                    2000.0,
                ],
                a: [
                    3000.0,
                    0.0,
                ],
                b: [
                    0.0,
                    3000.0,
                ],
            },
        ),
        boundary: CurveBoundary {
            inner: [
                [
                    0.0,
                ],
                [
                    6.283185307179586,
                ],
            ],
        },
        curve: Curve @ 0x7f0e481a34a0 => Curve,
        start_vertex: Vertex @ 0x7f0e481a77f0 => Vertex,
    },
]

The statement 1999.9999999999993 != 2000.0 is true, so it makes sense we got this failure. I can probably just setup my scripting language to preset this as a warning rather than a hard error, especially for how unrealistic of a situation this is (imagine that big of a 3D printer). But I do think it would be bad to get engineers into the habit of ignoring warnings, so I think it's best to fix false positives.

Could we possibly set an epsilon for comparisons in validation? We can default it to 0.0 and if the engineer is aware that they are making a massive unit conversion like this, they could set the epsilon to be reasonably large.

@hannobraun
Copy link
Owner

Good morning. I started implementing a custom scripting language built around Fornjot. I'll showcase it later when it's more complete.

Sounds awesome! Looking forward to seeing it!

Could we possibly set an epsilon for comparisons in validation? We can default it to 0.0 and if the engineer is aware that they are making a massive unit conversion like this, they could set the epsilon to be reasonably large.

We can! Here:

/// Construct an instance of `Instance`, using the provided configuration
pub fn with_validation_config(config: ValidationConfig) -> Self {
let layers = Layers::with_validation_config(config);
Self { layers }
}

This configuration has existed since forever, but it has only recently been exposed through Services (see #2060, #2150). This is not in a release yet.

I'm going to close this issue, as I think this has been addressed. If you have suggestions for specific improvements (like better default values, or maybe automatic adjustment of the settings in some way), feel free to open another issue or pull request!

(I don't know exactly when the next release, which will include the new API, will be out. I hope that I can get to it very soon, but there are a few other items on my list that I have to address first.)

@IamTheCarl
Copy link
Contributor Author

Ah perfect! That would explain why I didn't find it, as I've been using the release on crates.io assuming it would be just a smidge more stable.

I'll update my scripting language to be able to configure the validation when that feature is ready.

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

No branches or pull requests

2 participants