-
Notifications
You must be signed in to change notification settings - Fork 106
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
Import of graphic, layout and image objects #2124
base: master
Are you sure you want to change the base?
Conversation
OCD format recognizes symbol-less map objects that, however, render in map colors. They are called "graphic objects". We create synthetic symbols for graphic objects on import. All the Mapper symbols have the same name and number reflecting the anonymous nature of the source graphic objects. Closes OpenOrienteeringGH-959.
There is a flag in the .ocd file to hide all graphics objects which was not yet evaluated. The status of the flag is now read and applied to all imported graphic objects.
Layout objects (line, area, text) can now be imported. Significant code parts were taken over from Libor's commits. Layout objects' symbols are hidden depending on the 'l' flag in si_ViewPar. The 'l' flag in si_DisplayPar is apparently not used for hiding layouts objects, but both flags are evaluated in parallel. Todo: for text objects the import of the format (font, size, alignment, bold, italic) needs to be added.
Refactor importDisplayPar() by using OcdParameterStreamReader.
There is no need to cache the dynamically created graphic and layout symbols. Instead create a temporary symbol and then scan the list of symbols whether an equal symbol is already present and thus can be used instead.
Text layout objects contain a limited set of formatting options, e.g., font type, font size, horizontal alignment, italic and bold type. These options are stored after the text string in the same format as for Parameter Strings.
In addition to 'addWarning()' 'addWarningOnce()' only adds a warning to the list of warnings if the warning is not yet in the list.
Image objects (as well as layout objects) define their color directly in the object definition. With this commit the import of layout, image and graphic objects is now done within a single function. A single warning is issued that these kind of objects are imported by means of auxiliary symbols and can't be exported in their original format. Closes OpenOrienteering#1231
Layout objects can be hidden individually. This property is set within a parameter string (type 27). If the parameter string defines an image layout object, a warning is issue that this object type cannot be imported.
Setting of cap and join styles was done in the importLineSymbolBase() function and was replicated by this PR for importing special objects. In addition setupLineSymbolFraming() was setting the cap and join styles for framing lines (although only checking for a subset of cap and join styles). This commit moves the setting to a separate function.
Test data: Libor: thank you for reviewing |
Image layout objects are stored in parameter strings very similar to background images (i.e., templates), whereas the image file itself is stored outside the .ocd file. This commit imports image layout objects as templates (and informs the user about it) after the original templates have been imported and thus adds them to the end of the list of templates.
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.
Thanks, Matthias for the thorough implementation! I like the functionality a lot.
I have a few questions about the code though and some request about the patch set organization. I know how painful patch set rewrites tend to be but in my opinion some of the later patches ("OcdFileImport: Apply OcdParameterStreamReader", "OcdFileImport: Import of image objects", "OcdFileImport: Set cap and join styles in separate function") should be integrated into the earlier ones. I have indicated that in my comments as well.
The patch "ImportExport: Avoid duplicate warnings" should be renamed to make it clear that it introduces a new function rather than changing existing code. I would also suggest merging the change with the patch that uses the function for the first time.
The patch set adjustment should lead to a lower number of patches and a better readable change log. I'm looking forward to the next patch set version!
@@ -107,6 +107,7 @@ OcdFileImport::OcdImportedPathObject::~OcdImportedPathObject() = default; | |||
OcdFileImport::OcdFileImport(const QString& path, Map* map, MapView* view) | |||
: Importer { path, map, view } | |||
, custom_8bit_encoding { codecFromSettings() } | |||
, graphic_objects_hidden(false) |
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.
Please use uniform initialization (graphic_objects_hidden { false }
) for the member. Not that my code would be perfect in this aspect but the uniform initialization better fits the existing code here in ocd_file_import.cpp
and also ocd_file_export.cpp
.
My comment also applies to the other class members added further down the patch set.
src/fileformats/ocd_file_import.cpp
Outdated
int i = param_string.indexOf(QLatin1Char('\t'), 0); | ||
; // skip first word for this entry type |
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.
Just a nit ... or in other words, why repeat old sins. Use auto
for the variable. Fix grammar in the comment ("skip the first ...").
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.
Well, and after reading the rest of the patch set, I can see that this C&P goes away anyway. Please use the OcdParameterStreamReader
right from the beginning. I.e. squash "OcdFileImport: Apply OcdParameterStreamReader " into this patch.
src/fileformats/ocd_file_import.cpp
Outdated
for (int i = 0; i < num_symbols; ++i) | ||
{ | ||
if (symbol->equals(map->getSymbol(i))) | ||
{ | ||
delete symbol; | ||
return map->getSymbol(i); | ||
} | ||
} |
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.
Creating a symbol object and for every imported graphic object and then scanning the symbol list and potentially deleting the symbol object feels wasteful to me. Maybe I'm old school but maybe not that much when my views are reflected also in C++ Guidelines.
What is the reason for dropping the symbol hash structure, please?
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.
The hash structure was not applicable (easily) for text symbols: the hash value was previously created for line and area symbols from the type (line/area), color and width and thus fitted good into a 64 bit type. However, for text symbols there are additional properties: font size, bold/italic, font family. And for line symbols the cap and join styles had not been considered before.
There is a small runtime optimization possible that I did not apply. Instead of searching through the whole list of symbols it's sufficient to remember the index of the first auxiliary symbol and then start searching from there on.
src/fileformats/ocd_file_import.cpp
Outdated
{ | ||
symbol = symbol_index[ocd_object.symbol]; | ||
case -2: // graphic object |
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.
Please inroduce the enums for object types (now in patch "OcdFileImport: Import of image objects") in this patch and use the enum right from the beginning.
Don't worry about rewriting my patch. Take the change ownership and name me as a co-author.
else if (ocd_object.symbol == Ocd::ImageObject) | ||
addWarningOnce(tr("Importing image objects using auxiliary symbols. Export as image objects is not possible.")); | ||
else | ||
addWarningOnce(tr("Importing graphic objects using auxiliary symbols. Export as graphic objects is not possible.")); |
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.
I very much like these warnings and thought about them some time ago. Can you please look into integrating the warning messages into the earlier patches that implement the respective functionality?
/// The protection and visibility state of image objects (bitfield: 1 = protected, 2 = hidden) | ||
int image_objects_displaymode; | ||
|
||
/// counting objects during import | ||
int object_index; |
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.
I'm not completely sure about the purpose and naming of this member. Can you make its purpose more obvious, please?
@@ -1302,22 +1302,13 @@ Symbol* OcdFileImport::importLineSymbol(const S& ocd_symbol) | |||
return combined_line; | |||
} | |||
|
|||
void OcdFileImport::importLineSymbolBase(OcdImportedLineSymbol* symbol, const Ocd::LineSymbolCommonV8& attributes) | |||
bool OcdFileImport::setupLineStyle(OcdImportedLineSymbol* symbol, const quint16 line_style) |
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.
Another welcome deduplication! Could it go earlier in the patch set?
@@ -1131,6 +1140,39 @@ void OcdFileImport::importLayoutObjects(const QString& param_string) | |||
if (ok) | |||
object_number = i_value; | |||
break; | |||
case 'x': |
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.
I know that these parameters are shared with template image import. That said, is there a chance of introducing a shared function for handling the x, y, a, b, u and v parameters?
y_value = f_value; | ||
break; | ||
case 'a': | ||
case 'b': |
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.
As a side note. As to my understanding, a is in principle the angle of image x axis and b is the angle of the y axis in the map grid. The TODO should not be addressed in this patch set but if someone wants to look into it, here's the clue.
Of course, it will be easier for the future coder to adjust just one place in the code rather than fixing templates and image objects separately.
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.
I started having a look on that topic and already have a test file. Like you I decided that this fix is subject of another PR and thus deferred it.
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.
I also spent some time with the topic. I can load and save template transformation in .ocd files (see the attached archive) but the change breaks all the other template handling in Mapper.
template-ab-params.zip
The thing is that TemplateTransform::template_shear
is currently an opaque number that somehow embodies the actual template shear, along with other values like scaling and rotation. That said, the patch that introduces this class member needs a thorough review.
@dl3sdo, just to let you know, I've just spotted and .ocd with embedded layout image. It uses an undocumented code 'F'. Abbreviated string dump from Mapper follows. The .ocd can be downloaded from https://drive.google.com/drive/folders/1QR2jLfoq8pEss5z-eLqIaFI0u3d6uf1N?usp=sharing . The archive contains 'ocad12/Grafika výuková mapa_A4výška_ocad12.ocd' that has the layout thingie. The file is Base64 encoded. You may want to test your layout object code with this file to see how it behaves.
The .ocd v2018 variant also contains the embedded image. |
Layout raster image objects can also be stored as embedded images directly in the .ocd file. This commit allows to save the embedded image locally in order to be added as a template to the map.
@lpechacek: Before addressing your proposed changes I wanted to add the import of embedded images. |
Graphic, layout and image objects are stored in .ocd without dedicated symbols.
While graphic objects re-use existing symbol colors, image and layout objects store the object color inside the object.
Mapper imports these special objects by defining auxiliary symbols and colors.
Layout image objects, i.e.,objects that reference externally stored images cannot yet be imported, however, a warning is now issued instead of silently discarding them.
Closes #959
Closes #1231