-
Notifications
You must be signed in to change notification settings - Fork 120
Fix OCD text framing import/export #2419
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
base: master
Are you sure you want to change the base?
Conversation
Fix type of horizontal and vertical offsets for text symbol shadow framing to be signed. Call already existing function to setup the text symbol framing attributes for OCD export. Import color when importing a text symbol with line framing and issue warning for unsupported lines styles. Closes OpenOrienteeringGH-2418 (OCAD import and export of text framing is buggy).
Fix the convertSize() functions to round half down for negative values.
|
When adding a roundtrip test to file_format_t I noticed that a negative shadow framing would always (since Mapper does only allow to enter one digit after the dot) result in a wrong rounding, e.g. a value of -0.2 (being stored as -200) would be exported as -19 due to I assume that my commit removes the constexpr property from these functions. seem to be affected by the faulty conversion of negative values. @dg0yt: What do you propose? |
src/fileformats/ocd_file_export.cpp
Outdated
| if (size >= 0) | ||
| return qint16((size+5) / 10); | ||
| else | ||
| return qint16((size-5) / 10); |
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.
In case of constexpr doubt, this is a single expression:
| if (size >= 0) | |
| return qint16((size+5) / 10); | |
| else | |
| return qint16((size-5) / 10); | |
| return qint16(((size < 0) ? (size-5) : (size+5)) / 10); |
I wonder about the desired result for -5, -15 etc. Half down or half up? Obviously we are not converting a "size" here, but an "offset".
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 first added the convertOffset() function but then decided that this change should be applied to the convertSize() functions (even if it's rather an offset):
If the values are always positive, nothing changes. If the values are negative (besides the shadow framing offsets) then this would be fixed (an assertion would help identifying those occurences).
And obviously, 200..204 should become 20 whereas -200..-204 should become -20 instead of -19.
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.
And obviously, 200..204 should become 20 whereas -200..-204 should become -20 instead of -19.
I'm really asking about the 5. What do we want for -205 when +205 means +21?
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 would go for the standard rounding behaviour ("rounding halfway cases away from zero") as described in cppreference.com which means that -205 will become -21. Basically, it makes not really a difference if -205 becomes -21 or -20 as rounding means that always something is changed.
Regarding our specific mapper toppic: I thought it cannot even happen since the shadow framing offset integer values can only be a multiple of +-100 due to the GUI setting. But scaling the symbol can lead to any value!
Fix type of horizontal and vertical offsets for text symbol shadow framing to be signed.
Call already existing function to setup the text symbol framing attributes for OCD export.
Import color when importing a text symbol with line framing and issue warning for unsupported lines styles.
Closes GH-2418 (OCAD import and export of text framing is buggy).