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.

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

Persist OCD color numbers #2158

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

lpechacek
Copy link
Member

@lpechacek lpechacek commented May 23, 2023

This is a working implementation addressing issue 1245. The introduction of a stable color id uncovered some room for improvement in the native Mapper format and the new tests discovered bugs and limitations in the OCD format implementation. The first four patches introduce the stable color id required for smooth interoperability with the other software. The next four ones address defects and limitations in the OCD support routines.

There is certainly number of points for discussion in the patch set which I'm leaving for a possible review round.

@lpechacek lpechacek marked this pull request as draft September 10, 2023 17:59
@lpechacek
Copy link
Member Author

Marking the pull request as draft. There are problems in symbol import - the newly created colors can have duplicate id's which in turn breaks OCD export. I'll update the patch set soon.

@lpechacek lpechacek force-pushed the issue-1245-color-numbers branch from b04ea99 to 29a4b10 Compare April 3, 2024 12:28
@lpechacek
Copy link
Member Author

FWIW, this implementation has served me well for a few months. I'm not saying it's perfect. There are many spots in the code where improvement might be desirable. On the other hand, this exercise has uncovered a few defects. The fixes may be cherry-picked on its own. That said, I'm removing the draft status.

@lpechacek lpechacek marked this pull request as ready for review April 16, 2024 18:55
@dl3sdo
Copy link
Member

dl3sdo commented Apr 17, 2024

Libor, thank you for solving that issue.
At a first glance I noticed that you don't use a color_id value of 0. On the other hand, your checks don't exclude 0 (e.g., if (color_id >= 0 or return (id >= 0). I consider that not an issue indeed.

In addition I noticed that the Registration black color changed its id from 1 to 45 during the course of import from and export to .ocd (OCAD12), although 1 is not used in the exported map.

@lpechacek
Copy link
Member Author

At a first glance I noticed that you don't use a color_id value of 0. On the other hand, your checks don't exclude 0 (e.g., if (color_id >= 0 or return (id >= 0). I consider that not an issue indeed.

Ack. I noticed in some OCD files that zero is used as an ID. The Viewer also allows that value. But start at one is more common and it's also aligned with the Pascal view of the world. That's why zero is allowed but not preferred.

In addition I noticed that the Registration black color changed its id from 1 to 45 during the course of import from and export to .ocd (OCAD12), although 1 is not used in the exported map.

Yes, likely a bug in the code. :) I'll have a look.

The number accepted by getMapColor() and similar methods is not exactly
an index. The value is treated as the color rendering priority, it is
synced into the color Priority member and should be treated as such.
Namely, reordering the color table changes the priorities
invalidating any previous parameter values. This commit makes it obvious
what value we are using for the color lookup.
The color id value will help us reference the map color in file format
context. Currently colors are addressed by its rendering priority. The
goal is to address colors by its stable id in a future file format.

For now, we do not compare the color id in MapColor::equals() as there
is no use for it. The id is not part of the color identity, much like
its priority says nothing about the color itself.
The introduction of color id makes the tests alter example and symbol
set sources.
Use the color id to store the OCD color number. We know that the
registration black may move in the table, get a new number, or might be
dropped. The thing is that Mapper has registration black as a special
color that cannot get an OCD color number, and we merely guess whether
we need the color in the table in the export path.

Closes OpenOrienteeringGH-1245.
CMYK color component values can include fractional parts. The v8 format
expresses the component values as a number in the range 0-200 that maps
to percentages therefore component values are recorded with 0.5 %
precision. The higher format versions use strings for the color
definition, and the numbers can include one decimal place. This patch
makes Mapper export the fractional part as needed.
Although the official docs do not mention the color overprint property,
the program dialog contains this setting. A quick experiment shows that
the checkbox controls a reserved struct member. This patch implements
color overprint import and export in OCD v8 format.
Mapper is unable to detect Registration black in absence of spot
colors. This patch fixes the defect, loosens the name matching criterion
and aligns the matching criteria along the supported OCD format
versions.
Transparent (semi-opaque) colors are supported only in later format
versions. This patch makes Mapper write out a warning when the color
opacity value gets dropped.
@lpechacek lpechacek force-pushed the issue-1245-color-numbers branch from 29a4b10 to 30a443e Compare June 28, 2024 21:58
@lpechacek
Copy link
Member Author

Yes, likely a bug in the code. :) I'll have a look.

Fixed and rebased the patch set. The thing is that spot colors were assigned new id's in the OCD import path. The export path saw the low id numbers as used and searched for a new id for the registration black. Thanks again for testing the change reporting your findings!

@dg0yt dg0yt added this to the v0.9.6+1 milestone Jul 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants