From 8ca0e4158c3c0d615a4080bd45d800583975de73 Mon Sep 17 00:00:00 2001 From: Chris Von Bargen Date: Fri, 22 May 2026 19:09:40 -0400 Subject: [PATCH] SKETCH-2378: Use getSGroupDataLabels() for DAT SGroup label positioning Replace manual FIELDDISP parsing in SGroupItem with the new getSGroupDataLabels() API from RDKit (rdkit/rdkit#9189), which properly handles absolute vs relative positioning and edge cases. Co-Authored-By: Claude Opus 4.6 --- CMakeLists.txt | 1 + .../sketcher/molviewer/sgroup_item.cpp | 72 +++++++------------ .../sketcher/molviewer/sgroup_item.h | 9 --- 3 files changed, 26 insertions(+), 56 deletions(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index 0a2e99ab3..356bdd541 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -226,6 +226,7 @@ target_link_libraries( Qt6::Widgets RDKit::CIPLabeler RDKit::ChemReactions + RDKit::MolDraw2D ${RDKIT_EXTENSIONS_TARGET}) # Configure and include version information for the sketcher configure_file(${PROJECT_SOURCE_DIR}/include/schrodinger/sketcher/version.h.in diff --git a/src/schrodinger/sketcher/molviewer/sgroup_item.cpp b/src/schrodinger/sketcher/molviewer/sgroup_item.cpp index 4fcc18e65..e365ce7e1 100644 --- a/src/schrodinger/sketcher/molviewer/sgroup_item.cpp +++ b/src/schrodinger/sketcher/molviewer/sgroup_item.cpp @@ -7,6 +7,7 @@ #include "schrodinger/sketcher/molviewer/scene_utils.h" #include "schrodinger/sketcher/molviewer/sgroup_item.h" #include "schrodinger/sketcher/rdkit/s_group_constants.h" +#include #include #include #include @@ -27,20 +28,6 @@ SGroupItem::SGroupItem(const RDKit::SubstanceGroup& sgroup, const Fonts& fonts, show(); } -std::pair SGroupItem::getFieldDataDisplayInfo() const -{ - std::string fieldDisp; - if (m_sgroup.getPropIfPresent("FIELDDISP", fieldDisp)) { - QString displacement = QString::fromStdString(fieldDisp); - auto disp_x = displacement.sliced(0, 10).toFloat(); - auto disp_y = displacement.sliced(10, 10).toFloat(); - // if the string was not a valid float, the toFloat() function will - // return 0, which is what we want, so no need to check for errors - return {QPointF(disp_x, disp_y), displacement[26] == 'R'}; - } - return {QPointF(), false}; -} - /** * @return The repeat pattern label text for the SGroup, if any. * NOTE: Users expect head-to-tail (ht) to not be rendered. @@ -80,26 +67,6 @@ static QString get_repeat_label_text(const RDKit::SubstanceGroup& sgroup) return text; } -/** - * @return The FIELDDATA text for the SGroup, if any. This text is displayed - * next to the SGroup atoms. - */ -static QString get_field_data_text(const RDKit::SubstanceGroup& sgroup) -{ - QString text; - std::string typ; - if (sgroup.getPropIfPresent("TYPE", typ) && typ == "DAT") { - if (sgroup.hasProp("DATAFIELDS")) { - auto dfs = sgroup.getProp>("DATAFIELDS"); - for (const auto& df : dfs) { - text += df + "|"; - } - text.chop(1); - } - } - return text; -} - const RDKit::SubstanceGroup* SGroupItem::getSubstanceGroup() const { return &m_sgroup; @@ -136,20 +103,31 @@ void SGroupItem::updateCachedData() m_repeat = get_repeat_connect_text(m_sgroup); m_label = get_repeat_label_text(m_sgroup); - m_field_data_text = get_field_data_text(m_sgroup); + m_field_data_text.clear(); + m_field_data_text_rect = QRectF(); - // until https://github.com/rdkit/rdkit/issues/7829 is resolved, we need to - // place the field data text manually - auto [field_data_coords, field_data_coords_are_relative] = - getFieldDataDisplayInfo(); - m_field_data_text_rect = - m_fonts.m_sgroup_fm.tightBoundingRect(m_field_data_text); - // for now we ignore the relative flag and always place the field data text - // relative to the first atom in the sgroup - Q_UNUSED(field_data_coords_are_relative); - auto center = m_sgroup.getOwningMol().getConformer().getAtomPos( - m_sgroup.getAtoms()[0]); - m_field_data_text_rect.moveCenter(to_scene_xy(center) + field_data_coords); + int our_atom = m_sgroup.getAtoms().empty() + ? -1 + : static_cast(m_sgroup.getAtoms()[0]); + for (const auto& lbl : RDKit::MolDraw2D_detail::getSGroupDataLabels( + m_sgroup.getOwningMol())) { + if (lbl.atomIdx != our_atom) { + continue; + } + m_field_data_text = QString::fromStdString(lbl.text); + m_field_data_text_rect = + m_fonts.m_sgroup_fm.tightBoundingRect(m_field_data_text); + if (lbl.positioned) { + // pos is in Y-inverted drawing coords; scale to scene coords + m_field_data_text_rect.moveCenter( + QPointF(lbl.pos.x * VIEW_SCALE, lbl.pos.y * VIEW_SCALE)); + } else { + // pos is in conformer coords; convert to scene coords + m_field_data_text_rect.moveCenter( + to_scene_xy(RDGeom::Point3D(lbl.pos.x, lbl.pos.y, 0))); + } + break; + } auto [positions, displacement] = getPositionsForLabels(); auto brackets_half_height = positions.length() * 0.5; auto translation_offset = (positions.p1() + positions.p2()) * 0.5; diff --git a/src/schrodinger/sketcher/molviewer/sgroup_item.h b/src/schrodinger/sketcher/molviewer/sgroup_item.h index e572abfda..d845fcd0b 100644 --- a/src/schrodinger/sketcher/molviewer/sgroup_item.h +++ b/src/schrodinger/sketcher/molviewer/sgroup_item.h @@ -1,7 +1,5 @@ #pragma once -#include - #include "schrodinger/sketcher/definitions.h" #include "schrodinger/sketcher/molviewer/abstract_graphics_item.h" #include "schrodinger/sketcher/molviewer/fonts.h" @@ -88,13 +86,6 @@ class SKETCHER_API SGroupItem : public AbstractGraphicsItem */ QPainterPath getBracketPath() const; - /** - * @return The coordinates of the field data text and whether they are - * relative to the SGroup (as opposed to absolute). We assume the - * coordinates to be in scene units. - */ - std::pair getFieldDataDisplayInfo() const; - const RDKit::SubstanceGroup& m_sgroup; QPainterPath m_brackets_path; const Fonts& m_fonts;