Get MolDraw2DQt working again #3592

greglandrum · 2020-11-27T05:03:07Z

The original plan was to deprecate it, but it could be useful to have a non-cairo way to create high-quality PNGs. Qt lets us do that. So this updates the class to get it working with the new font handling code.

Some caveats:

it's not exposed to the Python wrappers yet... still need to figure out how to do that
if you want to use it without FreeType you need to have a QGuiApplication running. Otherwise you can have a QCoreApplication.

needs polish and testing non-freetype drawing doesn't work still

With the giant caveat that you need a QGuiApplication to use fonts. We try to detect that

greglandrum · 2020-11-27T05:03:26Z

@d-b-w : I'd love your feedback/thoughts on this

d-b-w · 2020-12-01T00:18:48Z

@ZontaNicola -
Would you mind taking a look at this?

d-b-w

Thanks for this, @greglandrum - It looks pretty wild to try to figure out the right conversions for some of this.

My only real concern is about the non-ascii data.

d-b-w · 2020-12-01T00:31:19Z

Code/GraphMol/MolDraw2D/DrawTextQt.h

+  void getStringSize(const std::string &label, double &label_width,
+                     double &label_height) const override;
+#endif
+  void drawChar(char c, const Point2D &cds) override;


What happens to non-ascii characters? Would they be rendered as individual utf-8 bytes?

I ask bc I know I've seen people talk in the RDK mailing list about, e.g. "±" in images.

I wonder if it would make sense to have this interface use QChar/QString.

The API for this is set at the MolDraw2D level and it is really only setup for text made up of single-byte characters.
We could think about moving to supporting wide chars, particularly now that the freetype support is there, but that's a bigger story

d-b-w · 2020-12-01T00:33:47Z

Code/GraphMol/MolDraw2D/DrawTextQt.cpp

+    : DrawText(max_fnt_sz, min_fnt_sz), d_qp(qp) {
+  PRECONDITION(
+      QCoreApplication::instance(),
+      "need a global QGuiApplication instance to use the Qt font system");


I think I've usually seen "QApplication" in this error message :)

sorry, I don't follow you here.

d-b-w · 2020-12-01T00:41:56Z

Code/GraphMol/MolDraw2D/catch_main.cpp

+//  of the RDKit source tree.
+//
+
+#define CATCH_CONFIG_MAIN  // This tells Catch to provide a main() - only do


I thought that the main was in catch_qt.cpp?

catch_main.cpp is currently used for moldraw2DTestCatch

d-b-w · 2020-12-01T00:43:27Z

Code/GraphMol/MolDraw2D/catch_qt.cpp

+    bool no_freetype = true;
+    MolDraw2DQt drawer(qimg.width(), qimg.height(), qpt, -1, -1, no_freetype);
+    MolDraw2DUtils::prepareAndDrawMolecule(drawer, *m1);
+    qimg.save("qttest-1b.png");


We've used http://pdiff.sourceforge.net/ to check things like this, but it's super sensitive to, e.g., platform differences or library updates. May not be useful for the RDKit

yeah, I think comparing PNGs to each other byte-for-byte isn't useful

d-b-w · 2020-12-01T00:44:19Z

Code/GraphMol/MolDraw2D/MolDraw2DQt.h

  void setColour(const DrawColour &col);

  void drawLine(const Point2D &cds1, const Point2D &cds2);
  void drawChar(char c, const Point2D &cds);
  void drawPolygon(const std::vector<Point2D> &cds);
  void clearDrawing();
-
+#if 0


bc these aren't implemented yet?

should have removed those, they were vestigial

ZontaNicola · 2020-12-01T08:39:59Z

Code/GraphMol/MolDraw2D/DrawTextFTQt.cpp

+
+  QPen pen(this_col);
+  pen.setJoinStyle(Qt::RoundJoin);
+  pen.setColor(this_col);


does this do something different from the QPen constructor two lines up?

nope. That looks like a mistake

ZontaNicola · 2020-12-01T08:49:55Z

Code/GraphMol/MolDraw2D/DrawTextQt.cpp

+  font.setPixelSize(fontSize());
+  d_qp.setFont(font);
+  QPointF loc(cds.x, cds.y);
+  QString text;


what about:
QString text (QChar (c))

ZontaNicola · 2020-12-01T09:01:01Z

Code/GraphMol/MolDraw2D/DrawTextFTQt.cpp

+  QPen pen(this_col);
+  pen.setJoinStyle(Qt::RoundJoin);
+  pen.setColor(this_col);
+  d_qp.setPen(pen);


is the painter used by other functions?
I like putting changes to painters between QPainter::save() and QPainter::restore() to make sure they don't bleed through, just a suggestion

It is used elsewhere, but the rest of the code should already be handling that particular problem since the drawing code runs across multiple rendering platforms, not all of which support equivalents to QPainter's save() and restore() methods

ZontaNicola · 2020-12-01T11:59:30Z

Code/GraphMol/MolDraw2D/DrawTextQt.cpp

+  }
+
+  for (size_t i = 0; i < draw_chars.size(); ++i) {
+    double char_width =


I was wondering if you could use QFontMetrics::boundingRect(QChar) here

yeah, this is the lowest-common denominator implementation that I added to just get things working
I will go back and look at doing it more correctly

ZontaNicola · 2020-12-01T14:15:24Z

Code/GraphMol/MolDraw2D/MolDraw2DQt.cpp

@@ -33,11 +67,11 @@ void MolDraw2DQt::setColour(const DrawColour &col) {
  QPen pen(this_col);
  pen.setJoinStyle(Qt::RoundJoin);
  pen.setColor(this_col);


you can also get rid of this line

greglandrum · 2020-12-01T15:16:53Z

@ptosco @ricrogz @bp-kelley : I'm leaning towards reclassifying this as a bug fix. It's a bit more substantial than the usual bug fix, but it takes code which doesn't work at all any more and gets it working again. It can't break existing code that uses 2020.09 since MolDraw2DQt doesn't work at all for that release.
What do you think?

ptosco · 2020-12-01T15:35:16Z

@greglandrum I agree with the reclassification. Thanks for your work on this - I think this is going to be useful to many. I guess to expose it usefully to Python it needs to be interfaced with either PyQt5 or PySide2, right?

ricrogz · 2020-12-01T17:34:05Z

I agree, reclassifying as a bug fix sounds the right thing to do.

greglandrum · 2020-12-02T03:43:06Z

@ZontaNicola Thanks for the review. I made your suggested changes.

greglandrum · 2020-12-03T16:27:56Z

Ok, merging this as is now that I have the CI builds working on at least linux and the Mac.
We should talk about how to handle the Python wrappers for this...

* Qt drawing works with freetype. needs polish and testing non-freetype drawing doesn't work still * make the Qt rendering anti-aliased * non-freetype fonts work in Qt With the giant caveat that you need a QGuiApplication to use fonts. We try to detect that * cleanup * changes in response to review * add qt support to CI builds * fix VS build problem * try resolving a linux link error * Update linux_build.yml * Update linux_build.yml * another attempt to get the linux CI builds working * getting closer? * Update linux_build.yml * disable the qt CI builds on windows

greglandrum added 4 commits November 26, 2020 10:56

Qt drawing works with freetype.

2b2c5cf

needs polish and testing non-freetype drawing doesn't work still

make the Qt rendering anti-aliased

f23f3a0

non-freetype fonts work in Qt

b645e2a

With the giant caveat that you need a QGuiApplication to use fonts. We try to detect that

cleanup

3be8511

greglandrum added the enhancement label Nov 27, 2020

greglandrum added this to the 2021_03_1 milestone Nov 27, 2020

d-b-w approved these changes Dec 1, 2020

View reviewed changes

ZontaNicola reviewed Dec 1, 2020

View reviewed changes

changes in response to review

4cdefd8

greglandrum added 3 commits December 2, 2020 05:18

add qt support to CI builds

6517112

fix VS build problem

708305c

try resolving a linux link error

e8dbfaf

greglandrum added bug and removed enhancement labels Dec 2, 2020

greglandrum modified the milestones: 2021_03_1, 2020_09_3 Dec 2, 2020

Update linux_build.yml

8c6b0aa

ZontaNicola approved these changes Dec 2, 2020

View reviewed changes

greglandrum added 5 commits December 2, 2020 09:08

Update linux_build.yml

00a15bb

another attempt to get the linux CI builds working

2b4fa9f

getting closer?

286379c

Update linux_build.yml

437405d

disable the qt CI builds on windows

0b4c075

greglandrum merged commit 938563c into rdkit:master Dec 3, 2020

greglandrum deleted the fix/get_qt_working_again branch December 3, 2020 16:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Get MolDraw2DQt working again #3592

Get MolDraw2DQt working again #3592

greglandrum commented Nov 27, 2020

greglandrum commented Nov 27, 2020

d-b-w commented Dec 1, 2020

d-b-w left a comment

d-b-w Dec 1, 2020

greglandrum Dec 1, 2020

d-b-w Dec 1, 2020

greglandrum Dec 1, 2020

d-b-w Dec 1, 2020

greglandrum Dec 1, 2020

d-b-w Dec 1, 2020

greglandrum Dec 1, 2020

d-b-w Dec 1, 2020

greglandrum Dec 1, 2020

ZontaNicola Dec 1, 2020

greglandrum Dec 1, 2020

ZontaNicola Dec 1, 2020

ZontaNicola Dec 1, 2020

greglandrum Dec 1, 2020

ZontaNicola Dec 1, 2020

greglandrum Dec 1, 2020

ZontaNicola Dec 1, 2020

greglandrum commented Dec 1, 2020 •

edited

ptosco commented Dec 1, 2020

ricrogz commented Dec 1, 2020

greglandrum commented Dec 2, 2020

greglandrum commented Dec 3, 2020

Get MolDraw2DQt working again #3592

Get MolDraw2DQt working again #3592

Conversation

greglandrum commented Nov 27, 2020

greglandrum commented Nov 27, 2020

d-b-w commented Dec 1, 2020

d-b-w left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

greglandrum commented Dec 1, 2020 • edited

ptosco commented Dec 1, 2020

ricrogz commented Dec 1, 2020

greglandrum commented Dec 2, 2020

greglandrum commented Dec 3, 2020

greglandrum commented Dec 1, 2020 •

edited