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.

Sign up for GitHub

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

Jump to bottom

Fixed drawing of O in aldehydes. #5515

Merged
merged 6 commits into from
Sep 12, 2022
Merged

Fixed drawing of O in aldehydes. #5515

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
b3c0ecf
Fixed drawing of O in aldehydes.
Aug 24, 2022
cc76dab
Added DrawColour::operator!=.
Aug 29, 2022
4b2f261
Remove unused variable.
Sep 7, 2022
9000358
Merge master.
Sep 7, 2022
3207553
Export doLinesIntersect for test.
Sep 7, 2022
5068a38
Merge branch 'master' into Github5511
greglandrum Sep 11, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 19 additions & 7 deletions Code/GraphMol/MolDraw2D/DrawMol.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1659,11 +1659,20 @@ void DrawMol::makeDoubleBondLines(
// in, for example, an aldehyde, such as in catch_tests.cpp's
// testGithub_5269_2.svg, might be asymmetrically shorter, so we don't
// want to change colour at halfway
if (bond->getEndAtom()->getDegree() == 1 && !(cols.first == cols.second) &&
fabs((l1s - l1f).lengthSq() - (l2s - l2f).lengthSq()) > 0.01) {
double midlen = (l1s - l1f).length() / 2.0;
Point2D lineDir = l2s.directionVector(l2f);
Point2D notMid = l2s + lineDir * midlen;
auto l1 = (l1s - l1f).lengthSq();
auto l2 = (l2s - l2f).lengthSq();
if ((bond->getBeginAtom()->getDegree() == 1 ||
bond->getEndAtom()->getDegree() == 1) &&
cols.first != cols.second && fabs(l1 - l2) > 0.01) {
double midlen = sqrt(l1) / 2.0;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
double midlen = sqrt(l1) / 2.0;
double midlen = sqrt(l1) * 0.5;

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I always let the compiler sort out this sort of micro-optimisation. Personally I think the intention is more immediately obvious with / 2.0 than * 0.5 but that might just be me. I can change it if it's house style.

Copy link
Contributor

Choose a reason for hiding this comment

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

More than a micro-optimization my suggestion was intended for consistency with the * 0.5 that is used a few lines below (line 2799), but again, not a must.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ah, sorry, I missed that. Consistency within a function is very desirable. I will alter it. If nothing else, that will force another run of the CI checks which will hopefully pass this time.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

At a quick scan there is pretty much a 50:50 mixture of / 2.0 and * 0.5 through this file. I think I will leave this particular instance, but aim for more consistency in future.

Point2D notMid;
if (bond->getBeginAtom()->getDegree() == 1) {
Point2D lineDir = l2s.directionVector(l2f);
notMid = l2s + lineDir * midlen;
} else {
Point2D lineDir = l2f.directionVector(l2s);
notMid = l2f + lineDir * midlen;
}
newBondLine(l2s, notMid, cols.first, cols.first, at1Idx, at2Idx, bondIdx,
noDash);
newBondLine(notMid, l2f, cols.second, cols.second, at1Idx, at2Idx,
Expand Down Expand Up @@ -2840,6 +2849,11 @@ void DrawMol::doubleBondTerminal(Atom *at1, Atom *at2, double offset,
l2s = at1_cds + perp * offset;
l2f = doubleBondEnd(at1->getIdx(), at2->getIdx(), thirdAtom->getIdx(),
offset, true);
// if at1 has a label, need to move it so it's centred in between the
// two lines (Github 5511).
if (atomLabels_[at1->getIdx()]) {
atomLabels_[at1->getIdx()]->cds_ += perp * offset * 0.5;
}
}
if (swapped) {
std::swap(l1s, l1f);
Expand Down Expand Up @@ -2896,8 +2910,6 @@ void DrawMol::findOtherBondVecs(const Atom *atom, const Atom *otherAtom,
}
for (unsigned int i = 1; i < atom->getDegree(); ++i) {
auto thirdAtom = otherNeighbor(atom, otherAtom, i - 1, *drawMol_);
auto bond =
drawMol_->getBondBetweenAtoms(atom->getIdx(), thirdAtom->getIdx());
Point2D const &at1_cds = atCds_[atom->getIdx()];
Point2D const &at2_cds = atCds_[thirdAtom->getIdx()];
otherBondVecs.push_back(at1_cds.directionVector(at2_cds));
Expand Down
4 changes: 2 additions & 2 deletions Code/GraphMol/MolDraw2D/DrawMol.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -319,8 +319,8 @@ double getHighlightBondWidth(
Point2D calcPerpendicular(const Point2D &cds1, const Point2D &cds2);
Point2D calcInnerPerpendicular(const Point2D &cds1, const Point2D &cds2,
const Point2D &cds3);
// return a point that is end1 moved so as not to clash with any of the
// rects of a label. end1 to end2 and the coords of 2 ends of a bond.
// return a point that is moveEnd moved so as not to clash with any of the
// rects of a label. moveEnd to end2 are the coords of 2 ends of a bond.
void adjustBondEndForString(
const Point2D &end2, double padding,
const std::vector<std::shared_ptr<StringRect>> &rects, Point2D &moveEnd);
Expand Down
1 change: 1 addition & 0 deletions Code/GraphMol/MolDraw2D/MolDraw2DHelpers.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ struct DrawColour {
bool operator==(const DrawColour &other) const {
return r == other.r && g == other.g && b == other.b && a == other.a;
}
bool operator!=(const DrawColour &other) const { return !(*this == other); }
bool feq(const DrawColour &other, double tol = 0.001,
bool ignoreAlpha = true) const {
return fabs(r - other.r) <= tol && fabs(g - other.g) <= tol &&
Expand Down
80 changes: 71 additions & 9 deletions Code/GraphMol/MolDraw2D/catch_tests.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,8 +86,8 @@ static const std::map<std::string, std::hash_result_t> SVG_HASHES = {
{"testHandDrawn-2.svg", 2605087576U},
{"testHandDrawn-3.svg", 1015633173U},
{"testHandDrawn-4.svg", 830784921U},
{"testHandDrawn-5a.svg", 3582113444U},
{"testHandDrawn-5b.svg", 2974975931U},
{"testHandDrawn-5a.svg", 2845825621U},
{"testHandDrawn-5b.svg", 476521352U},
{"testBrackets-1a.svg", 3257646535U},
{"testBrackets-1b.svg", 776088825U},
{"testBrackets-1c.svg", 3257646535U},
Expand All @@ -97,10 +97,10 @@ static const std::map<std::string, std::hash_result_t> SVG_HASHES = {
{"testBrackets-2b.svg", 1408188695U},
{"testBrackets-2c.svg", 728321376U},
{"testBrackets-2d.svg", 1408188695U},
{"testBrackets-3a.svg", 630742918U},
{"testBrackets-3a.svg", 791450653U},
{"testBrackets-4a.svg", 769125635U},
{"testBrackets-4b.svg", 4066682338U},
{"testBrackets-5a.svg", 2845825621U},
{"testBrackets-5a.svg", 1388227932U},
{"testSGroupData-1a.svg", 1463366807U},
{"testSGroupData-1b.svg", 223883202U},
{"testSGroupData-2a.svg", 3547547260U},
Expand Down Expand Up @@ -174,8 +174,8 @@ static const std::map<std::string, std::hash_result_t> SVG_HASHES = {
{"testGithub4519_2.svg", 2515716875U},
{"testGithub4519_3.svg", 1017109741U},
{"testGithub4519_4.svg", 645908829U},
{"testBaseFontSize.1a.svg", 3213010239U},
{"testBaseFontSize.1b.svg", 1147057058U},
{"testBaseFontSize.1a.svg", 3939288880U},
{"testBaseFontSize.1b.svg", 2617787443U},
{"testBaseFontSize.2a.svg", 1031690455U},
{"testBaseFontSize.2b.svg", 3440038194U},
{"testFlexiCanvas.1a.svg", 3145560884U},
Expand Down Expand Up @@ -214,7 +214,7 @@ static const std::map<std::string, std::hash_result_t> SVG_HASHES = {
{"testGithub_5061.svg", 1947248304U},
{"testGithub_5185.svg", 2944445711U},
{"testGithub_5269_1.svg", 3660737449U},
{"testGithub_5269_2.svg", 2580783009U},
{"testGithub_5269_2.svg", 3987247770U},
{"test_classes_wavy_bonds.svg", 1271445012U},
{"testGithub_5383_1.svg", 1391972140U},
{"github5156_1.svg", 695855770U},
Expand All @@ -228,7 +228,7 @@ static const std::map<std::string, std::hash_result_t> SVG_HASHES = {
{"acs1996_4.svg", 55815911U},
{"acs1996_5.svg", 362495488U},
{"acs1996_6.svg", 4274355858U},
{"acs1996_7.svg", 729001900U},
{"acs1996_7.svg", 3993278473U},
{"acs1996_8.svg", 2032371436U},
{"acs1996_9.svg", 2589221154U},
{"acs1996_10.svg", 4037187899U},
Expand All @@ -244,6 +244,8 @@ static const std::map<std::string, std::hash_result_t> SVG_HASHES = {
{"bond_highlights_7.svg", 2936856212U},
{"bond_highlights_8.svg", 64473502U},
{"testGithub5486_1.svg", 1149144091U},
{"testGithub5511_1.svg", 940106456U},
{"testGithub5511_2.svg", 1448975272U}
};

// These PNG hashes aren't completely reliable due to floating point cruft,
Expand All @@ -263,7 +265,7 @@ static const std::map<std::string, std::hash_result_t> PNG_HASHES = {
{"testHandDrawn-2.png", 2979412913U},
{"testHandDrawn-3.png", 1765396301U},
{"testHandDrawn-4.png", 2989933219U},
{"testHandDrawn-5.png", 3794840403U},
{"testHandDrawn-5.png", 1526220279U},
{"testGithub4323_1.png", 3711520691U},
{"testGithub4323_3.png", 2300228708U},
{"testFlexiCanvas.2a.png", 3618977786U},
Expand Down Expand Up @@ -5713,3 +5715,63 @@ M END)CTAB"_ctab;
}
}
}

TEST_CASE("Bad O position in aldehydes", "") {
std::string nameBase("testGithub5511_");
{
auto m1 = "O=Cc1cccc(C=O)c1"_smiles;
REQUIRE(m1);
MolDraw2DUtils::prepareMolForDrawing(*m1);
MolDraw2DSVG drawer(250, 250, -1, -1, NO_FREETYPE);
drawer.drawOptions().addAtomIndices = true;
drawer.drawMolecule(*m1);
drawer.finishDrawing();
auto text = drawer.getDrawingText();
std::string filename = nameBase + "1.svg";
std::ofstream outs(filename);
outs << text;
outs.flush();
check_file_hash(filename);
}
{
auto m = R"CTAB(
RDKit 2D

11 11 0 0 0 0 0 0 0 0999 V2000
-4.2885 0.5445 0.0000 C 0 0 0 0 0 0 0 0 0 0 0 0
-2.9895 1.2945 0.0000 O 0 0 0 0 0 0 0 0 0 0 0 0
-1.6818 0.5394 0.0000 C 0 0 0 0 0 0 0 0 0 0 0 0
-0.3738 1.2945 0.0000 C 0 0 0 0 0 0 0 0 0 0 0 0
0.9342 0.5394 0.0000 C 0 0 0 0 0 0 0 0 0 0 0 0
2.2422 1.2945 0.0000 C 0 0 0 0 0 0 0 0 0 0 0 0
2.2422 2.7945 0.0000 O 0 0 0 0 0 0 0 0 0 0 0 0
0.9342 -0.9708 0.0000 C 0 0 0 0 0 0 0 0 0 0 0 0
-0.3738 -1.7262 0.0000 C 0 0 0 0 0 0 0 0 0 0 0 0
-1.6818 -0.9708 0.0000 C 0 0 0 0 0 0 0 0 0 0 0 0
-2.9895 -1.7262 0.0000 O 0 0 0 0 0 0 0 0 0 0 0 0
1 2 1 0
2 3 1 0
3 4 2 0
4 5 1 0
5 6 1 0
6 7 2 0
5 8 2 0
8 9 1 0
9 10 2 0
10 11 1 0
10 3 1 0
M END
)CTAB"_ctab;
REQUIRE(m);
MolDraw2DSVG drawer(250, 250, -1, -1);
drawer.drawOptions().addAtomIndices = true;
drawer.drawMolecule(*m);
drawer.finishDrawing();
std::string text = drawer.getDrawingText();
std::string filename = nameBase + "2.svg";
std::ofstream outs(filename);
outs << text;
outs.flush();
check_file_hash(filename);
}
}
30 changes: 15 additions & 15 deletions Code/GraphMol/MolDraw2D/rxn_test1.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,25 +48,25 @@ namespace {
static const bool DELETE_WITH_GOOD_HASH = true;
#ifdef RDK_BUILD_FREETYPE_SUPPORT
static const std::map<std::string, std::hash_result_t> SVG_HASHES = {
{"rxn_test1_1.svg", 46725873U}, {"rxn_test1_2.svg", 2822273042U},
{"rxn_test1_1.svg", 46725873U}, {"rxn_test1_2.svg", 2822273042U},
{"rxn_test1_3.svg", 3749362740U}, {"rxn_test1_4.svg", 2720989271U},
{"rxn_test1_5.svg", 3245376196U}, {"rxn_test1_6.svg", 1371904127U},
{"rxn_test1_7.svg", 3410910592U}, {"rxn_test2_1.svg", 409647324U},
{"rxn_test2_2_1.svg", 1275078201U}, {"rxn_test2_2_2.svg", 2689198952U},
{"rxn_test1_5.svg", 3245376196U}, {"rxn_test1_6.svg", 1201850884U},
{"rxn_test1_7.svg", 3483485271U}, {"rxn_test2_1.svg", 409647324U},
{"rxn_test2_2_1.svg", 1275078201U}, {"rxn_test2_2_2.svg", 2689198952U},
{"rxn_test2_2_3.svg", 3750657998U}, {"rxn_test2_2_4.svg", 844519751U},
{"rxn_test3_1.svg", 3370327616U}, {"rxn_test4_1.svg", 1951283390U},
{"rxn_test4_2.svg", 1355336361U},
{"rxn_test3_1.svg", 2642962178U}, {"rxn_test4_1.svg", 2201210664U},
{"rxn_test4_2.svg", 3875105540U},
};
#else
static const std::map<std::string, std::hash_result_t> SVG_HASHES = {
{"rxn_test1_1.svg", 3077363070U}, {"rxn_test1_2.svg", 343913088U},
{"rxn_test1_3.svg", 1743857837U}, {"rxn_test1_4.svg", 421748462U},
{"rxn_test1_5.svg", 2287478842U}, {"rxn_test1_6.svg", 1059343942U},
{"rxn_test1_7.svg", 301064722U}, {"rxn_test2_1.svg", 2821179072U},
{"rxn_test2_2_1.svg", 1304295583U}, {"rxn_test2_2_2.svg", 1361778996U},
{"rxn_test1_5.svg", 2287478842U}, {"rxn_test1_6.svg", 1657115641U},
{"rxn_test1_7.svg", 2526900298U}, {"rxn_test2_1.svg", 2821179072U},
{"rxn_test2_2_1.svg", 1304295583U}, {"rxn_test2_2_2.svg", 1361778996U},
{"rxn_test2_2_3.svg", 2608405344U}, {"rxn_test2_2_4.svg", 574045696U},
{"rxn_test3_1.svg", 265057038U}, {"rxn_test4_1.svg", 684852697U},
{"rxn_test4_2.svg", 2675536892U},
{"rxn_test3_1.svg", 2373233565U}, {"rxn_test4_1.svg", 1661501846U},
{"rxn_test4_2.svg", 638174091U},
};
#endif

Expand All @@ -83,12 +83,12 @@ static const std::map<std::string, std::hash_result_t> SVG_HASHES = {
static const std::map<std::string, std::hash_result_t> PNG_HASHES = {
{"rxn_test1_1.png", 3579100589U}, {"rxn_test1_2.png", 3996724834U},
{"rxn_test1_3.png", 4153817948U}, {"rxn_test1_4.png", 4175225545U},
{"rxn_test1_5.png", 3400977230U}, {"rxn_test1_6.png", 2636974466U},
{"rxn_test1_7.png", 4164917700U}, {"rxn_test2_1.png", 2654417911U},
{"rxn_test1_5.png", 3400977230U}, {"rxn_test1_6.png", 1819663219U},
{"rxn_test1_7.png", 3834655428U}, {"rxn_test2_1.png", 2654417911U},
{"rxn_test2_2_1.png", 997060634U}, {"rxn_test2_2_2.png", 2090979640U},
{"rxn_test2_2_3.png", 857100114U}, {"rxn_test2_2_4.png", 610638635U},
{"rxn_test3_1.png", 501226401U}, {"rxn_test4_1.png", 4259011005U},
{"rxn_test4_2.png", 2214400478U},
{"rxn_test3_1.png", 2130633490U}, {"rxn_test4_1.png", 2917597113U},
{"rxn_test4_2.png", 3031850382U},
};

std::hash_result_t hash_file(const std::string &filename) {
Expand Down
4 changes: 2 additions & 2 deletions Code/GraphMol/MolDraw2D/test1.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -158,7 +158,7 @@ static const std::map<std::string, std::hash_result_t> SVG_HASHES = {
{"test21_1.svg", 3363530709U},
{"test21_2.svg", 3470002858U},
{"test22_1.svg", 3716192373U},
{"test22_2.svg", 1926484323U},
{"test22_2.svg", 3812042529U},
{"testGithub3112_1.svg", 3236038294U},
{"testGithub3112_2.svg", 1810059147U},
{"testGithub3112_3.svg", 135218742U},
Expand Down Expand Up @@ -287,7 +287,7 @@ static const std::map<std::string, std::hash_result_t> SVG_HASHES = {
{"test20_3.svg", 968052569U},
{"test20_4.svg", 2298201486U},
{"test22_1.svg", 3716192373U},
{"test22_2.svg", 2843125073U},
{"test22_2.svg", 3258508270U},
{"testGithub3112_1.svg", 2613843920U},
{"testGithub3112_2.svg", 3639942551U},
{"testGithub3112_3.svg", 1107662781U},
Expand Down