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.

Sign up for GitHub

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

Jump to bottom

OCD: Silence false warnings on shifted rows pattern export #1523

Closed
wants to merge 1 commit into from
Closed

OCD: Silence false warnings on shifted rows pattern export #1523

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
43 changes: 37 additions & 6 deletions src/fileformats/ocd_file_export.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1368,12 +1368,43 @@ quint8 OcdFileExport::exportAreaSymbolCommon(const AreaSymbol* area_symbol, OcdA
// NOTE: This is only a heuristic which works for the
// orienteering symbol sets, not a general conversion.
// (Conversion is not generally possible.)
// No further checks are done to find out if the conversion
// is applicable because with these checks. Already a tiny
// (not noticeable) error in the symbol definition would make
// it take the wrong choice.
addWarning(tr("In area symbol \"%1\", assuming a \"shifted rows\" point pattern. This might be correct as well as incorrect.")
.arg(area_symbol->getPlainTextName()));
// The below check silences the warning in cases an OCD file
// is being saved again. In other cases, tiny (not noticeable)
// differences in the symbol definition make the warning trigger
// and inform the user that the exported file will necessarily
// look slightly different than the Mapper original.
{
bool all_fine = false;
// storing the individual test bits in an integer
// makes diagnostics easier
quint32 conditions = (i == 1);
conditions |= (area_symbol->getNumFillPatterns() == 2) << 1;

if (conditions == (1<<2) - 1)
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 hard to read unless the reader is used to it, isn't it? And with the lowest two bits tested here, it is redundant to test them again later.

{
const auto& first_pattern = area_symbol->getFillPattern(0);
auto tolerant_eq = [](const auto a, const auto b) -> bool {
// "less than 1% difference"
return double(qAbs(b - a)) / a < .01;
};

conditions |= (first_pattern.flags == pattern.flags) << 2;
conditions |= (first_pattern.angle == pattern.angle) << 3;
conditions |= (first_pattern.line_spacing == pattern.line_spacing) << 4;
conditions |= (first_pattern.line_offset == 0) << 5;
conditions |= (pattern.line_offset == pattern.line_spacing/2) << 6;
conditions |= (first_pattern.line_width == pattern.line_width) << 7;
conditions |= (first_pattern.offset_along_line == 0) << 8;
conditions |= tolerant_eq(pattern.offset_along_line, pattern.point_distance/2) << 9;
conditions |= pattern.point->equals(pattern_symbol) << 10;

all_fine = (conditions == (1<<11) - 1);
Copy link
Member

Choose a reason for hiding this comment

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

Do you plan to extend the error handling in the future? I see the consistent pattern in the preceding lines, but I feel like this is over-engineered at the moment:

  • All individual sub-feature equalities are recorded in separate anonymous bits, but no use is made of the individual bits.
  • In the case of a future modification (moving, removing or adding a condition), one needs to carefully adjust multiple lines.

If all we want to know is if there is a single sub-feature which is not equal, just looking for the first offender would make code and future modifications simpler.

}

if (!all_fine)
addWarning(tr("In area symbol \"%1\", assuming a \"shifted rows\" point pattern. This is likely incorrect.")
.arg(area_symbol->getPlainTextName()));
Copy link
Member

Choose a reason for hiding this comment

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

Given that the warning shouldn't appear at all for orienteering symbol sets, after this change, I would hesitate to change the source string.

}

if (pattern.line_offset != 0)
ocd_area_common.structure_height /= 2;
Expand Down