OcdFileImport: Handle objects with rotated patterns

[lpechacek]

This pull request is a softer attempt at fixing GH-1926. The logic of OCD "hatch/structure is orientated to north" feels somewhat ad-hoc to me even after another look.

This patch makes Mapper recognize two categories of the possible three options supported by .ocd format:

1. areas with a non-zero pattern angle against the north, the pattern angle changes upon map rotation
2. areas with zero pattern angle against the north, the pattern is never rotated

The third possibility (areas with non-zero pattern angle against the north, the pattern angle is not changing upon map rotation) gets translated to 1) and an import warning is emitted.

[lpechacek]

No build warnings and the patch looks good to me. We can certainly talk about whether the pattern north orientation can be called "rotation lock" and similar, but I think that the general idea about how to handle the conceptual discrepancy between the two mapping applications is clearly expressed in the patch.

The aim of this PR is to supersede #1931.

[lpechacek]

@dg0yt I'd be grateful for your review.

[dg0yt]

The PR fixes the broken wineyards patterns in our existing maps.
However, it doesn't work for using an OCD symbol set for a new map - the patterns cannot be rotated without editing the symbol first.

One old test map had a rotated undergrowth pattern. This case is covered well by the warning message when I review them carefully. But on the other hand, it allows rotation were I don't want it.

With these comments, do you think the net effect is positive for this heuristics, in contrast to the original behaviour restored by #1931?

[lpechacek]

Thanks, Kai, for the review. The delay in my response was caused by my effort to find peace with the OCAD interpretation of the symbol north orientation. ISOM says that most of the map symbols are oriented to magnetic north, therefore also oriented against the paper edges. I thought that the Mapper implementation is the only possible interpretation of the standard but it isn't...

With the new knowledge about how OCAD handles the symbol north orientation, I tend to agree that making all area patterns rotatable on import makes Mapper match the OCAD user experience. Also, a large number of non-compliant maps out there will smoothly pass the import, ensuring user satisfaction.

On the concept level, going back to the original behavior, we not only open one more way to breaching ISOM but we are re-introducing an asymmetry in the export/import path. Mapper exports non-rotatable area patterns as "north oriented" to .ocd format (see OcdFileExport::exportAreaSymbolCommon()). But the import path was dropping the north orientation flag, losing our own information that can be well represented even in the foreign file format.

For me, the decision between the two pull requests is a decision between user comfort (original behavior) and stricter ISOM adherence and consistency (status quo). I understand that with perfection we are repelling users, and with comfort we are littering the codebase. That said, I'm leaning slightly towards masking the OCAD map issues in the import path for the sake of increased Mapper popularity.

P.S. Please note that the map provided in the context of issue #1926 has all symbols marked as oriented to the north. That means that its symbol set needs fixing. I've examined a few other .ocd maps I have at hand, and the area north orientation flag is set correctly in their symbol sets and some of them even imported without triggering the new warning.

[dg0yt]

I have a tendency to go with #1931 for now, and perhaps add an explicit attribute for the OCD feature sooner or later.

Some parts of export/import will just remain lossy. But as long as you the original format, it should be possible to restore the original symbol set if needed after a round trip of editing.

[lpechacek]

Fine with me. I'll write an explanatory comment in #1677 and mark it as WONTFIX. Thanks!

[lpechacek]

Ummm, life is full of surprises. While formulating the comment for the aforementioned issue about area hatches, I noticed that Mapper (silently) removes the rotation lock on point symbols when there is a rotated object (

mapper/src/fileformats/ocd_file_import.cpp

Lines 1932 to 1939 in 20065ec

	else if (ocd_object.angle != 0)
	{
	if (!point_symbol->isSymmetrical())
	{
	point_symbol->setRotatable(true);
	p->setRotation(convertAngle(ocd_object.angle));
	}
	}

). It is the same principle as in this proposal.

This rotation lock removal behavior was first implemented in commit 2cce659 without explanation why is this needed. We know the why now, though. Where to go from here? Align the behavior, either way, across all symbol types?

[lpechacek]

lpechacek@e8a3925 is my final say on the object rotation topic. The export path needs review for the area, point, and text objects.