Skip to content

Update fillet+chamfer with new API options #6745

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

Closed
wants to merge 3 commits into from

Conversation

adamchalmers
Copy link
Contributor

@adamchalmers adamchalmers commented May 7, 2025

Engine recently improved edge cuts (i.e. fillets/chamfers):

  • You can now do multiple cuts in a single API call, which should improve latency
  • Allows users to opt out of the "automatic" fillet strategy if they run into CSG bugs. They can set fillet(strategy = cutStrategy::basic) instead.
  • Fixes to artifact graph

Copy link

qa-wolf bot commented May 7, 2025

QA Wolf here! As you write new code it's important that your test coverage is keeping up.
Click here to request test coverage for this PR!

Copy link

vercel bot commented May 7, 2025

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
modeling-app ✅ Ready (Inspect) Visit Preview 💬 Add feedback May 7, 2025 7:04pm

@adamchalmers adamchalmers requested review from jtran and nrc May 7, 2025 18:00
Copy link

codspeed-hq bot commented May 7, 2025

CodSpeed Instrumentation Performance Report

Merging #6745 will not alter performance

Comparing achalmers/new-fillet-options (8f7ef49) with main (f938364)

Summary

✅ 54 untouched benchmarks

@adamchalmers adamchalmers force-pushed the achalmers/new-fillet-options branch 3 times, most recently from 75072e0 to ca93fb7 Compare May 7, 2025 18:27
Copy link
Contributor

@nrc nrc left a comment

Choose a reason for hiding this comment

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

My preference would be to not add strategy at all until after 1.0 and do it properly with an enum rather than add more string literal hacks. But if we think it is an essential feature then it's fine.

@settings(defaultLengthUnit = mm, kclVersion = 1.0)

/// Try the fast but simple Basic strategy, and if that fails, try the slow and complex CSG strategy.
export automatic = "automatic"
Copy link
Contributor

Choose a reason for hiding this comment

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

Could these be all caps like the turns and maths constants?

@@ -71,6 +71,8 @@ export fn fillet(
tolerance?: number(Length),
/// Create a new tag which refers to this fillet
tag?: tag,
/// Which strategy should be used to perform this chamfer?
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
/// Which strategy should be used to perform this chamfer?
/// Which strategy should be used to perform this fillet?

for _ in 0..num_extra_ids {
extra_face_ids.push(exec_state.next_uuid());
}
let strategy = strategy.unwrap_or_default();
Copy link
Contributor

Choose a reason for hiding this comment

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

We should warn or error on a bad strategy, at the moment a typo in a string literal will silently use the default

@adamchalmers
Copy link
Contributor Author

closing in favor of #6750

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.

3 participants