From e4f4644a89d6446ddebda0bf396fa4335324c41c Mon Sep 17 00:00:00 2001 From: Paolo Tosco Date: Tue, 3 Sep 2024 21:03:40 +0200 Subject: [PATCH] - Expose multicolor highlights to MinimalLib (#7787) --- .../MolDraw2D/DrawMolMCHCircleAndLine.cpp | 1 + Code/GraphMol/MolDraw2D/catch_tests.cpp | 47 +++++++++++ Code/MinimalLib/cffi_test.c | 47 +++++++++-- Code/MinimalLib/common.h | 78 +++++++++++++++++-- Code/MinimalLib/common_defs.h | 56 ++++++++----- Code/MinimalLib/tests/tests.js | 14 ++++ 6 files changed, 210 insertions(+), 33 deletions(-) diff --git a/Code/GraphMol/MolDraw2D/DrawMolMCHCircleAndLine.cpp b/Code/GraphMol/MolDraw2D/DrawMolMCHCircleAndLine.cpp index 4f4daa940..1dfb645e1 100644 --- a/Code/GraphMol/MolDraw2D/DrawMolMCHCircleAndLine.cpp +++ b/Code/GraphMol/MolDraw2D/DrawMolMCHCircleAndLine.cpp @@ -203,6 +203,7 @@ void DrawMolMCHCircleAndLine::calcSymbolEllipse(unsigned int atomIdx, Point2D ¢re, double &xradius, double &yradius) const { + PRECONDITION(atomIdx < atCds_.size() && atomIdx < atomLabels_.size(), "") centre = atCds_[atomIdx]; getAtomRadius(atomIdx, xradius, yradius); if (drawOptions_.atomHighlightsAreCircles || !atomLabels_[atomIdx] || diff --git a/Code/GraphMol/MolDraw2D/catch_tests.cpp b/Code/GraphMol/MolDraw2D/catch_tests.cpp index 82f04c9df..18804fac1 100644 --- a/Code/GraphMol/MolDraw2D/catch_tests.cpp +++ b/Code/GraphMol/MolDraw2D/catch_tests.cpp @@ -9984,3 +9984,50 @@ TEST_CASE("Github 7739 - Bad multi-coloured wedge") { check_file_hash(fileStem + "5.svg"); } } + +TEST_CASE("idx out of bounds should not cause a segfault") { + auto m = "C"_smiles; + { + MolDraw2DSVG drawer(300, 300, -1, -1, NO_FREETYPE); + std::map> atomCols{{2, {DrawColour(0, 0, 0)}}}; + std::map> bondCols; + std::map atomRads{{2, 1.0}}; + std::map bondMults; + REQUIRE_THROWS_AS( + drawer.drawMoleculeWithHighlights(*m, "nocrash", atomCols, bondCols, + atomRads, bondMults), + Invar::Invariant); + } + { + MolDraw2DSVG drawer(300, 300, -1, -1, NO_FREETYPE); + std::map> atomCols; + std::map> bondCols{{2, {DrawColour(0, 0, 0)}}}; + std::map atomRads; + std::map bondMults; + REQUIRE_THROWS_AS( + drawer.drawMoleculeWithHighlights(*m, "nocrash", atomCols, bondCols, + atomRads, bondMults), + Invar::Invariant); + } + { + MolDraw2DSVG drawer(300, 300, -1, -1, NO_FREETYPE); + std::map> atomCols{{0, {DrawColour(0, 0, 0)}}}; + std::map> bondCols; + std::map atomRads{{2, 1.0}}; + std::map bondMults; + REQUIRE_NOTHROW( + drawer.drawMoleculeWithHighlights(*m, "nocrash", atomCols, bondCols, + atomRads, bondMults)); + } + { + MolDraw2DSVG drawer(300, 300, -1, -1, NO_FREETYPE); + std::map> atomCols; + std::map> bondCols{{0, {DrawColour(0, 0, 0)}}}; + std::map atomRads; + std::map bondMults{{2, 10}}; + REQUIRE_THROWS_AS( + drawer.drawMoleculeWithHighlights(*m, "nocrash", atomCols, bondCols, + atomRads, bondMults), + Invar::Invariant); + } +} diff --git a/Code/MinimalLib/cffi_test.c b/Code/MinimalLib/cffi_test.c index 36bcf074d..591062287 100644 --- a/Code/MinimalLib/cffi_test.c +++ b/Code/MinimalLib/cffi_test.c @@ -2295,7 +2295,8 @@ void test_assign_chiral_tags_from_mol_parity() { char *mpkl; size_t mpkl_size; char *smiles; - const char *artemisininCTAB = "68827\n\ + const char *artemisininCTAB = + "68827\n\ -OEChem-03262404452D\n\ \n\ 20 23 0 1 0 0 0 0 0999 V2000\n\ @@ -2350,10 +2351,13 @@ M END\n\ assert(!strcmp(smiles, "CC1CCC2C(C)C(=O)OC3OC4(C)CCC1C32OO4")); free(smiles); free(mpkl); - mpkl = get_mol(artemisininCTAB, &mpkl_size, "{\"assignChiralTypesFromMolParity\":true}"); + mpkl = get_mol(artemisininCTAB, &mpkl_size, + "{\"assignChiralTypesFromMolParity\":true}"); assert(mpkl); smiles = get_smiles(mpkl, mpkl_size, NULL); - assert(!strcmp(smiles, "C[C@@H]1CC[C@H]2[C@@H](C)C(=O)O[C@@H]3O[C@@]4(C)CC[C@@H]1[C@]32OO4")); + assert(!strcmp( + smiles, + "C[C@@H]1CC[C@H]2[C@@H](C)C(=O)O[C@@H]3O[C@@]4(C)CC[C@@H]1[C@]32OO4")); free(smiles); free(mpkl); } @@ -2663,9 +2667,9 @@ M END\n\ char *mpkl; char *mpkl_copy; size_t mpkl_size; - const char *mb_rdkit_wedging; - const char *mb_rdkit_wedging_post_orig; - const char *mb_orig_wedging; + char *mb_rdkit_wedging; + char *mb_rdkit_wedging_post_orig; + char *mb_orig_wedging; mpkl = get_mol(mb, &mpkl_size, ""); assert(mpkl && mpkl_size); mpkl_copy = malloc(mpkl_size); @@ -2687,6 +2691,36 @@ M END\n\ free(mpkl_copy); } +void test_multi_highlights() { + printf("--------------------------\n"); + printf(" test_multi_highlights\n"); + const char *smi = + "[H]c1cc2c(-c3ccnc(Nc4ccc(F)c(F)c4)n3)c(-c3cccc(C(F)(F)F)c3)nn2nc1C"; + const char *details = + "{\"width\":250,\"height\":200,\"highlightAtomMultipleColors\":{\"15\":[[0.941,0.894,0.259]],\"17\":[[0,0.62,0.451]],\"21\":[[0.902,0.624,0]],\"22\":[[0.902,0.624,0]],\"23\":[[0.902,0.624,0]],\"24\":[[0.902,0.624,0]],\"25\":[[0.902,0.624,0]],\"26\":[[0.902,0.624,0]],\"27\":[[0.902,0.624,0]],\"28\":[[0.902,0.624,0]],\"29\":[[0.902,0.624,0]],\"30\":[[0.902,0.624,0]],\"35\":[[0.337,0.706,0.914]]},\"highlightBondMultipleColors\":{\"14\":[[0.941,0.894,0.259]],\"16\":[[0,0.62,0.451]],\"20\":[[0.902,0.624,0]],\"21\":[[0.902,0.624,0]],\"22\":[[0.902,0.624,0]],\"23\":[[0.902,0.624,0]],\"24\":[[0.902,0.624,0]],\"25\":[[0.902,0.624,0]],\"26\":[[0.902,0.624,0]],\"27\":[[0.902,0.624,0]],\"28\":[[0.902,0.624,0]],\"29\":[[0.902,0.624,0]],\"34\":[[0.337,0.706,0.914]],\"38\":[[0.902,0.624,0]]},\"highlightAtomRadii\":{\"15\":0.4,\"17\":0.4,\"21\":0.4,\"22\":0.4,\"23\":0.4,\"24\":0.4,\"25\":0.4,\"26\":0.4,\"27\":0.4,\"28\":0.4,\"29\":0.4,\"30\":0.4,\"35\":0.4},\"highlightLineWidthMultipliers\":{\"14\":2,\"16\":2,\"20\":2,\"21\":2,\"22\":2,\"23\":2,\"24\":2,\"25\":2,\"26\":2,\"27\":2,\"28\":2,\"29\":2,\"34\":2,\"38\":2}}"; + const char *colors[] = {"#009E73", "#55B4E9", "#E69F00", "#EFE342", NULL}; + char *mpkl; + char *svg_with_details; + char *svg_without_details; + size_t mpkl_size; + int i; + mpkl = get_mol(smi, &mpkl_size, "{\"removeHs\":false}"); + assert(mpkl && mpkl_size); + svg_with_details = get_svg(mpkl, mpkl_size, details); + assert(strstr(svg_with_details, "ellipse")); + for (i = 0; colors[i]; ++i) { + assert(strstr(svg_with_details, colors[i])); + } + svg_without_details = get_svg(mpkl, mpkl_size, ""); + assert(!strstr(svg_without_details, "ellipse")); + for (i = 0; colors[i]; ++i) { + assert(!strstr(svg_without_details, colors[i])); + } + free(svg_with_details); + free(svg_without_details); + free(mpkl); +} + int main() { enable_logging(); char *vers = version(); @@ -2721,5 +2755,6 @@ int main() { test_smiles_smarts_params(); test_wedged_bond_atropisomer(); test_get_molblock_use_molblock_wedging(); + test_multi_highlights(); return 0; } diff --git a/Code/MinimalLib/common.h b/Code/MinimalLib/common.h index 542a8d60b..ae78af4e1 100644 --- a/Code/MinimalLib/common.h +++ b/Code/MinimalLib/common.h @@ -354,6 +354,41 @@ std::string parse_rgba_array(const rj::Value &val, DrawColour &color, return ""; } +std::string parse_highlight_multi_colors( + const rj::Document &doc, std::map> &colorMap, + const std::string &keyName, bool &haveMultiColors) { + const auto it = doc.FindMember(keyName.c_str()); + haveMultiColors = (it != doc.MemberEnd()); + if (!haveMultiColors) { + return ""; + } + if (!it->value.IsObject()) { + return "JSON contains '" + keyName + "' field, but it is not an object"; + } + for (const auto &entry : it->value.GetObject()) { + int idx = std::atoi(entry.name.GetString()); + auto &drawColorVect = colorMap[idx]; + if (entry.value.IsArray()) { + const auto &arr = entry.value.GetArray(); + if (std::all_of(arr.begin(), arr.end(), + [](const auto &item) { return item.IsArray(); })) { + for (const auto &item : arr) { + DrawColour color; + auto problems = parse_rgba_array(item, color, keyName); + if (!problems.empty()) { + return problems; + } + drawColorVect.push_back(std::move(color)); + } + continue; + } + } + return "JSON contains '" + keyName + + "' field, but it is not an array of R,G,B[,A] arrays"; + } + return ""; +} + std::string parse_highlight_colors(const rj::Document &doc, std::map &colorMap, const std::string &keyName) { @@ -441,19 +476,48 @@ std::string process_mol_details(const std::string &details, if (!problems.empty()) { return problems; } - - problems = parse_highlight_colors(doc, molDrawingDetails.atomMap, - "highlightAtomColors"); + bool haveAtomMultiColors; + problems = parse_highlight_multi_colors(doc, molDrawingDetails.atomMultiMap, + "highlightAtomMultipleColors", + haveAtomMultiColors); if (!problems.empty()) { return problems; } - - problems = parse_highlight_colors(doc, molDrawingDetails.bondMap, - "highlightBondColors"); + bool haveBondMultiColors; + problems = parse_highlight_multi_colors(doc, molDrawingDetails.bondMultiMap, + "highlightBondMultipleColors", + haveBondMultiColors); if (!problems.empty()) { return problems; } - + if (haveAtomMultiColors || haveBondMultiColors) { + const auto lineWidthMultiplierMapit = + doc.FindMember("highlightLineWidthMultipliers"); + if (lineWidthMultiplierMapit != doc.MemberEnd()) { + if (!lineWidthMultiplierMapit->value.IsObject()) { + return "JSON contains 'highlightLineWidthMultipliers' field, but it is not an object"; + } + for (const auto &entry : lineWidthMultiplierMapit->value.GetObject()) { + if (!entry.value.IsInt()) { + return "JSON contains 'highlightLineWidthMultipliers' field, but the multipliers" + "are not ints"; + } + int idx = std::atoi(entry.name.GetString()); + molDrawingDetails.lineWidthMultiplierMap[idx] = entry.value.GetInt(); + } + } + } else { + problems = parse_highlight_colors(doc, molDrawingDetails.atomMap, + "highlightAtomColors"); + if (!problems.empty()) { + return problems; + } + problems = parse_highlight_colors(doc, molDrawingDetails.bondMap, + "highlightBondColors"); + if (!problems.empty()) { + return problems; + } + } const auto radiiMapit = doc.FindMember("highlightAtomRadii"); if (radiiMapit != doc.MemberEnd()) { if (!radiiMapit->value.IsObject()) { diff --git a/Code/MinimalLib/common_defs.h b/Code/MinimalLib/common_defs.h index 5af00b828..074f3d6a6 100644 --- a/Code/MinimalLib/common_defs.h +++ b/Code/MinimalLib/common_defs.h @@ -39,7 +39,10 @@ struct DrawingDetails { struct MolDrawingDetails : public DrawingDetails { std::map atomMap; std::map bondMap; + std::map> atomMultiMap; + std::map> bondMultiMap; std::map radiiMap; + std::map lineWidthMultiplierMap; }; struct RxnDrawingDetails : public DrawingDetails { @@ -87,28 +90,41 @@ class DrawerFromDetails { } initDrawer(molDrawingDetails); const ROMol *molPtr = &mol; - std::unique_ptr origWedgingMol; - if (molDrawingDetails.useMolBlockWedging) { - origWedgingMol.reset(new ROMol(mol)); - Chirality::reapplyMolBlockWedging(*origWedgingMol); - molPtr = origWedgingMol.get(); - drawer().drawOptions().useMolBlockWedging = false; + std::unique_ptr drawnMol; + bool haveMultiMap = (!molDrawingDetails.atomMultiMap.empty() || + !molDrawingDetails.bondMultiMap.empty()); + if (molDrawingDetails.useMolBlockWedging || haveMultiMap) { + drawnMol.reset(new RWMol(mol)); + molPtr = static_cast(drawnMol.get()); + if (molDrawingDetails.useMolBlockWedging) { + Chirality::reapplyMolBlockWedging(*drawnMol); + drawer().drawOptions().useMolBlockWedging = false; + } } drawer().setOffset(molDrawingDetails.offsetx, molDrawingDetails.offsety); - - MolDraw2DUtils::prepareAndDrawMolecule( - drawer(), *molPtr, molDrawingDetails.legend, &molDrawingDetails.atomIds, - &molDrawingDetails.bondIds, - molDrawingDetails.atomMap.empty() ? nullptr - : &molDrawingDetails.atomMap, - molDrawingDetails.bondMap.empty() ? nullptr - : &molDrawingDetails.bondMap, - molDrawingDetails.radiiMap.empty() ? nullptr - : &molDrawingDetails.radiiMap, - -1, molDrawingDetails.kekulize, molDrawingDetails.addChiralHs, - molDrawingDetails.wedgeBonds, molDrawingDetails.forceCoords, - molDrawingDetails.wavyBonds); - + if (!haveMultiMap) { + MolDraw2DUtils::prepareAndDrawMolecule( + drawer(), *molPtr, molDrawingDetails.legend, + &molDrawingDetails.atomIds, &molDrawingDetails.bondIds, + molDrawingDetails.atomMap.empty() ? nullptr + : &molDrawingDetails.atomMap, + molDrawingDetails.bondMap.empty() ? nullptr + : &molDrawingDetails.bondMap, + molDrawingDetails.radiiMap.empty() ? nullptr + : &molDrawingDetails.radiiMap, + -1, molDrawingDetails.kekulize, molDrawingDetails.addChiralHs, + molDrawingDetails.wedgeBonds, molDrawingDetails.forceCoords, + molDrawingDetails.wavyBonds); + } else { + MolDraw2DUtils::prepareMolForDrawing( + *drawnMol, molDrawingDetails.kekulize, molDrawingDetails.addChiralHs, + molDrawingDetails.wedgeBonds, molDrawingDetails.forceCoords, + molDrawingDetails.wavyBonds); + drawer().drawMoleculeWithHighlights( + *drawnMol, molDrawingDetails.legend, molDrawingDetails.atomMultiMap, + molDrawingDetails.bondMultiMap, molDrawingDetails.radiiMap, + molDrawingDetails.lineWidthMultiplierMap); + } return finalizeDrawing(); } std::string draw_rxn(const ChemicalReaction &rxn) { diff --git a/Code/MinimalLib/tests/tests.js b/Code/MinimalLib/tests/tests.js index 4b6d0f688..3afdca559 100644 --- a/Code/MinimalLib/tests/tests.js +++ b/Code/MinimalLib/tests/tests.js @@ -3176,6 +3176,19 @@ function test_get_mol_copy() { } } +function test_multi_highlights() { + const mol = RDKitModule.get_mol('[H]c1cc2c(-c3ccnc(Nc4ccc(F)c(F)c4)n3)c(-c3cccc(C(F)(F)F)c3)nn2nc1C', JSON.stringify({removeHs: false})); + const details = '{"width":250,"height":200,"highlightAtomMultipleColors":{"15":[[0.941,0.894,0.259]],"17":[[0,0.62,0.451]],"21":[[0.902,0.624,0]],"22":[[0.902,0.624,0]],"23":[[0.902,0.624,0]],"24":[[0.902,0.624,0]],"25":[[0.902,0.624,0]],"26":[[0.902,0.624,0]],"27":[[0.902,0.624,0]],"28":[[0.902,0.624,0]],"29":[[0.902,0.624,0]],"30":[[0.902,0.624,0]],"35":[[0.337,0.706,0.914]]},"highlightBondMultipleColors":{"14":[[0.941,0.894,0.259]],"16":[[0,0.62,0.451]],"20":[[0.902,0.624,0]],"21":[[0.902,0.624,0]],"22":[[0.902,0.624,0]],"23":[[0.902,0.624,0]],"24":[[0.902,0.624,0]],"25":[[0.902,0.624,0]],"26":[[0.902,0.624,0]],"27":[[0.902,0.624,0]],"28":[[0.902,0.624,0]],"29":[[0.902,0.624,0]],"34":[[0.337,0.706,0.914]],"38":[[0.902,0.624,0]]},"highlightAtomRadii":{"15":0.4,"17":0.4,"21":0.4,"22":0.4,"23":0.4,"24":0.4,"25":0.4,"26":0.4,"27":0.4,"28":0.4,"29":0.4,"30":0.4,"35":0.4},"highlightLineWidthMultipliers":{"14":2,"16":2,"20":2,"21":2,"22":2,"23":2,"24":2,"25":2,"26":2,"27":2,"28":2,"29":2,"34":2,"38":2}}'; + const svgWithDetails = mol.get_svg_with_highlights(details); + assert(svgWithDetails.includes('ellipse')); + const COLORS = ['#009E73', '#55B4E9', '#E69F00', '#EFE342']; + assert(COLORS.every((color) => svgWithDetails.includes(color))); + const svgWithOutDetails = mol.get_svg_with_highlights(''); + assert(!svgWithOutDetails.includes('ellipse')); + assert(!COLORS.some((color) => svgWithOutDetails.includes(color))); + mol.delete(); +} + initRDKitModule().then(function(instance) { var done = {}; const waitAllTestsFinished = () => { @@ -3257,6 +3270,7 @@ initRDKitModule().then(function(instance) { test_assign_chiral_tags_from_mol_parity(); test_make_dummies_queries(); test_get_mol_copy(); + test_multi_highlights(); waitAllTestsFinished().then(() => console.log("Tests finished successfully") );