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

Implement dynamic text overlay for resource generation #670

Merged
merged 23 commits into from Nov 8, 2017

Conversation

jordangriffiths01
Copy link
Contributor

@jordangriffiths01 jordangriffiths01 commented Nov 6, 2017

This PR:

  • Adds the class TextBoxDrawer to extract text field information from an AI-exported SVG file, and dynamically render text within those fields on a PNG image which has been exported without the text field layer.
  • Utilises this functionality in the barcode checksum poster resource.

Tests and documentation will be added before merging, but I'd like to get your thoughts after an initial pass before that if possible :)

@jordangriffiths01 jordangriffiths01 added backend frontend internationalization labels Nov 6, 2017
@jordangriffiths01 jordangriffiths01 self-assigned this Nov 6, 2017
@codecov
Copy link

@codecov codecov bot commented Nov 6, 2017

Codecov Report

Merging #670 into develop will increase coverage by 0.29%.
The diff coverage is 91.45%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop     #670      +/-   ##
===========================================
+ Coverage    87.55%   87.84%   +0.29%     
===========================================
  Files           88       90       +2     
  Lines         2306     2502     +196     
  Branches       283      311      +28     
===========================================
+ Hits          2019     2198     +179     
- Misses         231      238       +7     
- Partials        56       66      +10
Impacted Files Coverage Δ
...es/views/BarcodeChecksumPosterResourceGenerator.py 100% <100%> (ø) ⬆️
csunplugged/utils/errors/TextBoxDrawerErrors.py 100% <100%> (ø)
csunplugged/utils/TextBoxDrawer.py 90.44% <90.44%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f1c3e48...286f63f. Read the comment docs.

@jordangriffiths01
Copy link
Contributor Author

@jordangriffiths01 jordangriffiths01 commented Nov 7, 2017

Unit testing and documentation has been added

Copy link
Member

@JackMorganNZ JackMorganNZ left a comment

Amazing work @jordangriffiths01, very impressive!

Two other items generally to change:

  • PR contains about 50/50 split on single and double quotes, could we change them all to double for consistency?
  • Documentation seems to be missing loads of full stops (didn't mark them all as it would a lot of comments).

DEFAULT_FONT_SIZE = 20
DEFAULT_COLOR = (0, 0, 0)

FONT_MAX_ORD = {
Copy link
Member

@JackMorganNZ JackMorganNZ Nov 7, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is this? Don't understand what ord is.

Copy link
Contributor Author

@jordangriffiths01 jordangriffiths01 Nov 7, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ord = Ordinal of the character, or its unicode code point. In python, the function ord is used - i.e.

>>> ord("a")
97
>>> ord("は")
12399

Many fonts only contain glyphs for a small subset of unicode - in this case, I believed PatrickHand-Regular only contains glyphs for characters up to 0xFF (255). This dictionary is used to map fonts to the highest ordinal for which they contain a glyph. For strings containing chars with a higher ordinal than this, a fallback font will have to be used.

On further research, it seems that PatrickHand-Regular actually supports more codepoints than I first thought. Ideally, we would extract this information directly from the font file rather than hardcoding it in, although a bit of quick research made this seem non-trivial. It may be possible using the fontTools library, I will investigate tomorrow.

Copy link
Contributor Author

@jordangriffiths01 jordangriffiths01 Nov 7, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like an approach similar to https://stackoverflow.com/questions/43834362 may work.

Copy link
Member

@JackMorganNZ JackMorganNZ Nov 7, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That solution looks smarter, but also Patrick Hand will be gutted from the system soon and replaced by Noto.

Copy link
Contributor Author

@jordangriffiths01 jordangriffiths01 Nov 7, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As discussed offline, this will be left as is for now. It will be improved in a later PR (tracked in issue #671)

self.width_ratio, self.height_ratio = self.get_dimension_ratios()

@staticmethod
def load_svg(svg_path):
Copy link
Member

@JackMorganNZ JackMorganNZ Nov 7, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would this be better as a top level function or even in utils?

Copy link
Contributor Author

@jordangriffiths01 jordangriffiths01 Nov 7, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Personally I don't think so - I find it unlikely we will be parsing SVG files in situations independent of this class, and having it as a static method in the class namespace is cleaner IMO.

Copy link
Member

@JackMorganNZ JackMorganNZ Nov 7, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just asking as when I searched what a @staticmethod was, a lot of the documentation stated how little it should be used. But happy to leave as is.

"""Write text onto image in the space defined by the given textbox.
Args:
box: (str or TextBox) either the id of a text element in the SVG,
Copy link
Member

@JackMorganNZ JackMorganNZ Nov 7, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you state why a TextBox would be directly given? I have some guesses but want to hear your thoughts.

Copy link
Contributor Author

@jordangriffiths01 jordangriffiths01 Nov 7, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is covered in the documentation, under the heading Rendering text without defined text fields (i.e. without an SVG file)

The TL;DR is that it allows for abstract use of the dynamic text box rendering functionality onto any arbitrary PNG, without relying on an AI-exported SVG file to define the available text fields. This would be necessary for a PNG that came from a source other than Illustrator (or without an exported SVG), and/or allows for additional text to be added onto an image as required, independently of the text fields defined in the SVG.

I don't envisage this being used often, as it breaks with the idea that the text field layout should be part of the resource design process.

Copy link
Member

@JackMorganNZ JackMorganNZ Nov 7, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, it did make sense in the documentation later. Thankfully most of the dynamically drawn text in our resources so far is numbers or symbols (letters) which don't need translations so can be drawn directly with Pillow. But great to know if we need it down the line, it's there.


For Designers
------------------------------------------------------------------------------
The following workflow has been designed for Adobe Illustrator. Currently, other graphics software is not supported.
Copy link
Member

@JackMorganNZ JackMorganNZ Nov 7, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What other software has been tested? The only other main one I can think of would be Inkscape.

Copy link
Contributor Author

@jordangriffiths01 jordangriffiths01 Nov 7, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I haven't tried it on any other software - it's compatibility would depend entirely on the way an SVG is exported (if even possible), and how similar its structure is to the one exported from AI. This is something that could be investigated later, if the need arose.

Copy link
Contributor Author

@jordangriffiths01 jordangriffiths01 left a comment

Ugh, my constant single/double quotes battle :P I'm so used to single quotes that I often slip back into that habit. I'll fix this up, and also the full stops in the documentation, thanks!

"""Write text onto image in the space defined by the given textbox.
Args:
box: (str or TextBox) either the id of a text element in the SVG,
Copy link
Contributor Author

@jordangriffiths01 jordangriffiths01 Nov 7, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is covered in the documentation, under the heading Rendering text without defined text fields (i.e. without an SVG file)

The TL;DR is that it allows for abstract use of the dynamic text box rendering functionality onto any arbitrary PNG, without relying on an AI-exported SVG file to define the available text fields. This would be necessary for a PNG that came from a source other than Illustrator (or without an exported SVG), and/or allows for additional text to be added onto an image as required, independently of the text fields defined in the SVG.

I don't envisage this being used often, as it breaks with the idea that the text field layout should be part of the resource design process.

DEFAULT_FONT_SIZE = 20
DEFAULT_COLOR = (0, 0, 0)

FONT_MAX_ORD = {
Copy link
Contributor Author

@jordangriffiths01 jordangriffiths01 Nov 7, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ord = Ordinal of the character, or its unicode code point. In python, the function ord is used - i.e.

>>> ord("a")
97
>>> ord("は")
12399

Many fonts only contain glyphs for a small subset of unicode - in this case, I believed PatrickHand-Regular only contains glyphs for characters up to 0xFF (255). This dictionary is used to map fonts to the highest ordinal for which they contain a glyph. For strings containing chars with a higher ordinal than this, a fallback font will have to be used.

On further research, it seems that PatrickHand-Regular actually supports more codepoints than I first thought. Ideally, we would extract this information directly from the font file rather than hardcoding it in, although a bit of quick research made this seem non-trivial. It may be possible using the fontTools library, I will investigate tomorrow.

self.width_ratio, self.height_ratio = self.get_dimension_ratios()

@staticmethod
def load_svg(svg_path):
Copy link
Contributor Author

@jordangriffiths01 jordangriffiths01 Nov 7, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Personally I don't think so - I find it unlikely we will be parsing SVG files in situations independent of this class, and having it as a static method in the class namespace is cleaner IMO.


For Designers
------------------------------------------------------------------------------
The following workflow has been designed for Adobe Illustrator. Currently, other graphics software is not supported.
Copy link
Contributor Author

@jordangriffiths01 jordangriffiths01 Nov 7, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I haven't tried it on any other software - it's compatibility would depend entirely on the way an SVG is exported (if even possible), and how similar its structure is to the one exported from AI. This is something that could be investigated later, if the need arose.

@jordangriffiths01
Copy link
Contributor Author

@jordangriffiths01 jordangriffiths01 commented Nov 7, 2017

Adding @saranglove as a reviewer to review documentation of the process on the Illustrator end.

saranglove
saranglove previously approved these changes Nov 7, 2017
Copy link
Contributor

@saranglove saranglove left a comment

I've only read over the For Designers part of the docs and they're fine.

@jordangriffiths01 jordangriffiths01 merged commit dd86490 into develop Nov 8, 2017
4 checks passed
@jordangriffiths01 jordangriffiths01 deleted the resource-text-overlay branch Nov 8, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backend frontend internationalization
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants