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

Offset for Face and Wire #145

Merged
merged 4 commits into from
Nov 25, 2023
Merged

Conversation

mkovaxx
Copy link
Collaborator

@mkovaxx mkovaxx commented Oct 18, 2023

Closes #144.

@mkovaxx mkovaxx self-assigned this Oct 18, 2023
Copy link
Owner

@bschwind bschwind left a comment

Choose a reason for hiding this comment

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

Nice! Let's do a few things to finish this up:

  • Format the code with cargo +nightly fmt
  • Fix the compilation error
  • Add a usage of this feature, either in a new example or add it to an existing example if it makes sense

@mkovaxx
Copy link
Collaborator Author

mkovaxx commented Oct 24, 2023

I need to figure out a couple of things, so this is still a draft. Stay tuned! :)

@mkovaxx mkovaxx marked this pull request as ready for review November 5, 2023 09:08
@mkovaxx mkovaxx requested a review from bschwind November 5, 2023 09:08
Copy link
Owner

@bschwind bschwind left a comment

Choose a reason for hiding this comment

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

All the binding code looks good! I know I was sitting with you when you created the example file, but I think we should add a bit more to it to demonstrate what's going on:

Here's an example of a chevron shape with acute and obtuse angles:

let solid_1 = Workplane::xy()
    .translated(DVec3::new(32.0 * i as f64, 0.0, 0.0))
    .sketch()
    .move_to(0.0, 10.0)
    .line_to(5.0, 5.0)
    .line_to(5.0, -5.0)
    .line_to(0.0, 0.0)
    .line_to(-5.0, -5.0)
    .line_to(-5.0, 5.0)
    .close()
    .offset(1.0, join_type) // offset a wire
    .to_face()
    .extrude(DVec3::new(0.0, 0.0, 8.0))
    .into_shape();

I think if we add that shape along with positive and negative offsets, it will more readily demonstrate what the feature does.

While testing this I also had crashes with positive offsets for the Tangent join type and I'm not sure why.

It's nice to demonstrate that you can offset a face or a wire, but ultimately they result in the same exact shape so the visuals don't really show you what's happening so much.

@mkovaxx
Copy link
Collaborator Author

mkovaxx commented Nov 12, 2023

Uh-oh I'm getting the segfaults too...

@mkovaxx mkovaxx marked this pull request as draft November 12, 2023 05:16
@bschwind
Copy link
Owner

bschwind commented Nov 12, 2023

@mkovaxx that particular join type just might not be supported yet, I'd have to look at the code to be sure.

Or rather, not all cases of using it are supported.

@mkovaxx
Copy link
Collaborator Author

mkovaxx commented Nov 25, 2023

@bschwind I propose that we put JoinType::Tangent out of scope for now, and try to add it later.
Issue to track that work: #153

@bschwind
Copy link
Owner

I propose that we put JoinType::Tangent out of scope for now, and try to add it later.

Makes sense! Shall we mark this ready for review?

@mkovaxx mkovaxx marked this pull request as ready for review November 25, 2023 08:12
Copy link
Owner

@bschwind bschwind left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@bschwind bschwind merged commit 4ea7cdf into bschwind:main Nov 25, 2023
4 checks passed
@mkovaxx mkovaxx deleted the mate_offset_face_wire branch June 9, 2024 04:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Offset for Face and Wire
2 participants