Skip to content

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

Merged
eyal0 merged 5 commits intopcb2gcode:masterfrom
schumar:master
Nov 20, 2021
Merged

fix: don't warn when rounding by less than 1 micro-inch#597
eyal0 merged 5 commits intopcb2gcode:masterfrom
schumar:master

Conversation

@schumar
Copy link
Copy Markdown
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.

Comment thread drill.cpp Outdated
});
if (best_available_drill->difference(wanted_length, inputFactor)) {

auto difference = best_available_drill->difference(wanted_length, inputFactor);
Copy link
Copy Markdown
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
Copy Markdown
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.

Comment thread 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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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.

Comment thread drill.cpp Outdated
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
Copy Markdown
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
Copy Markdown
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
Comment thread drill.cpp Outdated
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.

2 participants