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

Add brim_overlap option to allow control of brim bonding strength. Fix #4174. #4957

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ianmackinnon
Copy link

@ianmackinnon ianmackinnon commented Apr 18, 2020

This is my first pull request. Please let me know if there is anything in terms of content or protocol that I should do differently.

The value I have specified as the default (-3.13%, eg. a very slight separation, a negative overlap) maintains the current brim distance although it's not a very round number.

slic3r-branch-brim-overlap -3 13 default

I have added a “default” option in PrintConfig.cpp but the default value still shows as -3.13% in the Print Settings panel. I'm not sure what I would have to change to make this show default instead.

2020-04-18_16-05

@AppVeyorBot
Copy link

@ianmackinnon ianmackinnon changed the title Add brim_overlap option to allow control of brim bonding strength. Issue #4174. Add brim_overlap option to allow control of brim bonding strength. Fix #4174. Apr 18, 2020
@lordofhyphens
Copy link
Member

@ianmackinnon please rebase.

Additionally, once you've saved a config the defaults aren't going to show for you.

Copy link
Member

@lordofhyphens lordofhyphens left a comment

Choose a reason for hiding this comment

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

Tests need work to show that they actually have the desired effect.

auto exported {gcode.str()};
REQUIRE(exported.size() > 0);
}
}
Copy link
Member

Choose a reason for hiding this comment

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

This is a really basic test -- would really prefer testing that the spacing is what is expected.

Perhaps some parsing of the GCode to look at the brim lines?

@lordofhyphens
Copy link
Member

@ianmackinnon see the review comment, please.

@lordofhyphens
Copy link
Member

@ianmackinnon #4997 has some code around testing the brim generation itself (it also fixes a minor off-by-one error I found when writing the test).

It looks like you're pushing the entire brim out by that little amount; I think that referring to the gap in some small absolute unit like mm or maybe um would be more understandable by users than percentage, as you're basically changing the inflate/deflate scale numbers.

@VanessaE any thoughts about this?

@ianmackinnon
Copy link
Author

@lordofhyphens Thanks for the feedback and linking that issue with the relevant test. When I get a moment I'll rebase and add a proper test.

I imagine that the ability to control the gap will be important only to a small subset of users, so in my opinion the priority should be to not alter current behaviour. -3.13% is not a neat number but it is the value that matches current behaviour and functionally I think this default in the existing code works great for the vast majority of prints. I also think it makes sense for the size of the gap to scale with the print width rather than be an absolute value.

So my preference would be to stick with a percentage and keep that number as the default but if others feel strongly it should have a different unit or default value I'm happy to try it out.

@VanessaE
Copy link
Collaborator

VanessaE commented Jul 23, 2020

I favor an exact measure for this e.g. mm. µm might be okay, though I guess this would be the only option to use that.

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.

4 participants