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

Smoother augment_profile #10

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

jacekkopecky
Copy link

@jacekkopecky jacekkopecky commented Dec 25, 2018

augment_profile in skin.scad inserts new vertices in a profile so that the result has a given number of vertices. Currently, the order of insertions is not specified, and in some cases the new vertices cause distortion.

This profile (in context of extrusion.scad) demonstrates the issue: the cylinder is visibly distorted:

// profiles that have different vertex counts
skin([
	transform(translation([0,0,0]), circle($fn=70,r=1)),
	transform(translation([0,0,1]), circle($fn=75,r=1)),
	transform(translation([0,0,2]), circle($fn=100,r=1)),
]);

This image shows the distortion.
screen shot 2018-12-25 at 14 50 39

The example above is artificial, but the issue comes up when combining profiles that you naturally produce with different vertex counts.

This pull requests introduces insert_extra_vertices_1 which distributes new vertices uniformly around the length of the profile, resulting in much smaller distortions.

I'll be happy to polish the code if need be.

@jacekkopecky
Copy link
Author

Hi, do we have any maintainers here please? I know no commits happened in over 3 years, but maybe?

@OskarLinde
Copy link

Thanks for submitting. This looks like a reasonable approach. (note that both this and the original code are quite inefficient – there are better ways to do it)

Have you verified that none of the samples degrade by taking this approach?

@jacekkopecky
Copy link
Author

Hi, thanks; I'll do that but need a few more weeks before I get to it due to $dayjob.

@jacekkopecky
Copy link
Author

Hi, yes, I can now confirm that all the other examples in this repo that use skin work correctly.

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.

2 participants