-
Notifications
You must be signed in to change notification settings - Fork 105
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
OCD: Silence false warnings on shifted rows pattern export #1523
OCD: Silence false warnings on shifted rows pattern export #1523
Conversation
Mapper has a flexible area pattern definition that cannot translate to OCD in general. However, there is one case when Mapper and OCD definitions match. Detect the case and silence the export warning in this case. As a result, it is possible to load an OCD file and save it again without warnings.
@dg0yt, I'd be thankful for your review and comments. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is a good thing to silence this warning when it is not needed. However, I fear that the proposed change may give us headaches in the future.
- It turns a short and simple
case
branch into a long and less simple one, making the surrounding loop more difficult to understand. Maybe a separate function could resolve this issue. - With that complexity, it might be possible to identify all the cases which are handled incorrectly, and fall through to just skipping the fill pattern (instead of exporting it wrongly), like we already do in other cases, removing the need for this particular warning.
From a more general point of view / long-term perspective:
- With moving symbol analysis even more ahead of the actual export, it might be possible to split the original symbol for individual patterns, similar to what is done for certain line symbols or area symbol with border for OCD version 8.
- Re-importing each exported symbol and comparing it to the original might be the best approach to detect changes in export, independent of export format.
Now this is a lot of comments for a short PR. So let me express my major wishes:
- a separate test function, to keep the
for
andswitch
statements short, - leaving the warning string (source for translation) unchanged.
quint32 conditions = (i == 1); | ||
conditions |= (area_symbol->getNumFillPatterns() == 2) << 1; | ||
|
||
if (conditions == (1<<2) - 1) |
There was a problem hiding this comment.
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.
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); |
There was a problem hiding this comment.
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())); |
There was a problem hiding this comment.
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.
Shall I pick this up? It is really worth pursueing. |
Feel free to do so. The patch rework is currently low on my TODO list. Thanks! |
And thanks for the review! I consider all the above valid points and I also share the long term ideas about export code improvements. |
Mapper has a flexible area pattern definition that does not translate to OCD in general. However, the special case of "shifted rows" can be recognized, and sub symbols of row structures can be adjusted to match the OCD model.In this case, no warning needs to be issued. Closes GH-1523 (silence false warnings).
Mapper has a flexible area pattern definition that does not translate to OCD in general. However, the special case of "shifted rows" can be recognized, and sub symbols of row structures can be adjusted to match the OCD model.In this case, no warning needs to be issued. Closes GH-1523 (silence false warnings).
Mapper has a flexible area pattern definition that does not translate to OCD in general. However, the special case of "shifted rows" can be recognized, and subsymbols of row structures can be adjusted to match the OCD model. In this case, no warning needs to be issued. Closes GH-1523 (silence false warnings).
Mapper has a flexible area pattern definition that does not translate to OCD in general. However, the special case of "shifted rows" can be recognized, and subsymbols of row structures can be adjusted to match the OCD model. In this case, no warning needs to be issued. Closes GH-1523 (silence false warnings).
Mapper has a flexible area pattern definition that does not translate to OCD in general. However, the special case of "shifted rows" can be recognized, and subsymbols of row structures can be adjusted to match the OCD model. In this case, no warning needs to be issued. Closes GH-1523 (silence false warnings).
Mapper has a flexible area pattern definition that does not translate to OCD in general. However, the special case of "shifted rows" can be recognized, and subsymbols of row structures can be adjusted to match the OCD model. In this case, no warning needs to be issued. Closes GH-1523 (silence false warnings).
Mapper has a flexible area pattern definition that does not translate to OCD in general. However, the special case of "shifted rows" can be recognized, and subsymbols of row structures can be adjusted to match the OCD model. In this case, no warning needs to be issued. Closes GH-1523 (silence false warnings).
Reimplemented in the above-referenced pull request. |
Mapper has a flexible area pattern definition that does not translate to OCD in general. However, the special case of "shifted rows" can be recognized, and subsymbols of row structures can be adjusted to match the OCD model. In this case, no warning needs to be issued. Closes GH-1523 (silence false warnings). In another commit, this will be complemented by changes which improve "aligned rows" structures.
I've been irritated by the warning about shifted rows pattern upon export to OCD for some time. While Mapper has a more flexible point pattern definition mechanism than the other program, there is one case where they match. The case typically occurs when and OCD is loaded to Mapper and then exported back to OCD. The import/export routines are at the level of completeness that this workflow can complete without excessive noise.
This patch detects when Mapper can successfully export shifted rows area pattern and silences the warning in this particular case. The code change does not look particularly beautiful and I'm considering it a "v1" with the possibility of rework.