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

fix: don't warn when rounding by less than 1 micro-inch #597

Merged
merged 5 commits into from
Nov 20, 2021

Conversation

schumar
Copy link
Contributor

@schumar schumar commented Oct 30, 2021

When working in metric units, pcb2gcode is giving warnings like

Exporting drill... Info: bit 1 (0.8mm) is rounded to 0.8mm
Info: bit 2 (0.9mm) is rounded to 0.9mm
Info: bit 3 (1mm) is rounded to 1mm
Info: bit 4 (3.2mm) is rounded to 3.2mm

With this patch, warnings are only printed when the amount of rounding was more than 1 micro-inch.

drill.cpp Outdated
@@ -847,12 +847,16 @@ map<int, drillbit> ExcellonProcessor::optimize_bits() {
return a.difference(wanted_length, inputFactor).value_or(std::numeric_limits<double>::infinity()) <
b.difference(wanted_length, inputFactor).value_or(std::numeric_limits<double>::infinity());
});
if (best_available_drill->difference(wanted_length, inputFactor)) {

auto difference = best_available_drill->difference(wanted_length, inputFactor);
Copy link
Contributor

Choose a reason for hiding this comment

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

I prefer const auto if you don't mind. That way I don't have to later wonder about whether or not it might change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense, thanks.

drill.cpp Outdated
cerr << "Info: bit " << wanted_drill.first << " ("
<< old_string << ") is rounded to "
<< drill_to_string(wanted_drill_bit) << std::endl;
if (difference > 1e-6 || difference < -1e-6) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Or abs(difference) > 1e-6. That way you don't have to repeat the constant.

Another way that we could do this PR is to string compare old_string with drill_to_string(wanted_drill_bit). But your way seems better, yes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Using abs() was my initial approach, but then the compiler started shouting at me:

drill.cpp:855:35: error: no matching function for call to ‘abs(const boost::optional<double>&)’
                 if (abs(difference) > 1e-6) {
                                   ^
[long list of "no known conversion for argument 1 from ‘const boost::optional<double>’ to '[other format]']

Copy link
Contributor

Choose a reason for hiding this comment

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

I didn't even realize that it's an optional! Huh.

So you'll need if (difference && abs(*difference) > 1e-6)

Sorry about that! I must've had a good reason why it's optional but I forgot. Maybe because you might have units that can't be subtracted together or something.

drill.cpp Outdated
@@ -852,7 +852,7 @@ map<int, drillbit> ExcellonProcessor::optimize_bits() {
if (difference) {
wanted_drill_bit.diameter = best_available_drill->diameter().asInch(inputFactor);
wanted_drill_bit.unit = "inch";
if (difference > 1e-6 || difference < -1e-6) {
if (difference && abs(*difference) > 1e-6) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Oh man I'm so sorry to do this to you, I reviewed it on my phone and I didn't see that there is already if(difference) above! I'm a dummy.

So no need to test for difference. Doh!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was wondering about that, but then I don't know a lot about C++/boost :)
fixed in e8d68de

as it's checked a few lines above anyway
drill.cpp Outdated Show resolved Hide resolved
accidentally commited an experimental change
@eyal0 eyal0 merged commit d41ddf4 into pcb2gcode:master Nov 20, 2021
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.

None yet

2 participants