#2088 Selection Tool: use rounded rectangles for selection of bonds and atom labels #2602

KonstantinEpam23 · 2023-05-15T08:18:54Z

Closes #2088

…nd atom labels

yuleicul

Hi @KonstantinEpam23 , almost everything looks good to me.
One thing I'm not sure about is the fill of circle when I'm adding a bond or an atom. I thought the fill would be none because I was just adding a new bond instead of hovering on a selected one. What's your take on this?

yuleicul

Another issue is also about the fill of circle.

Scenario 1

Reproduction steps

Click Bond Tool
Click canvas to create a new bond
Move slowly to the closest atom

Actual behavior

Fill of circle is light green

Expected behavior

Fill of circle would be none, because the atom was not selected

Scenario 2

Reproduction steps

Click Bond Tool
Click canvas to create a new bond
Click Select Tool
Move slowly to one of the atoms

Actual behavior

Fill of circle is light green

Expected behavior

Fill of circle is none, because the atom is not selected

Nitvex · 2023-05-19T08:36:52Z

packages/ketcher-core/src/application/render/restruct/reatom.ts

-      .circle(ps.x, ps.y, options.atomSelectionPlateRadius)
-      .attr(options.hoverStyle)
+
+    const result =


May be we can move this repeating block to a separate function?

Nitvex · 2023-05-19T08:39:57Z

packages/ketcher-core/src/application/render/restruct/rebond.ts

+
+    hb1.p = shiftBondEnd(atom1, p1, hb1.dir, 2 * options.lineWidth)
+    hb2.p = shiftBondEnd(atom2, p2, hb2.dir, 2 * options.lineWidth)
+    bond.b.center = Vec2.lc2(atom1.a.pp, 0.5, atom2.a.pp, 0.5)


May be you can add a comment for lc2 (where it is defined)?
For me it is still a mystery, what it does.

@Nitvex I actually just moved the bondRecalc function so it is a static method now, its a bit of a mystery for myself what lc2 does 😄 I'll try to figure it out and a dd a comment

@KonstantinEpam23 @Nitvex I thought lc was the abbreviation of Linear Combination. The difference between lc() and lc2() was the number of arguments they could receive. lc() could combine lots of vectors, but lc2() could only combine 2 vectors, so functionality-wise lc2 could be replaced by lc.

Nitvex · 2023-05-19T08:41:24Z

packages/ketcher-core/src/application/render/restruct/rebond.ts

+    const regularSelectionThikness = doubleBondWidth + bondThickness
+
+    // half-bonds
+    const hbStart = restruct.molecule.halfBonds.get(bond.hb1!)


May be we can?

const halfBondStart = restruct.molecule.halfBonds.get(bond.hb1!).p // p at the end

Nitvex · 2023-05-19T08:46:38Z

packages/ketcher-core/src/domain/entities/vec2.ts

@@ -221,6 +232,40 @@ export class Vec2 {
    )
  }

+  rotateAroundOrigin(angle: number, origin: Vec2) {


may be we can rename a angle param to something like angleInDegrees?

KonstantinEpam23 · 2023-05-19T13:10:13Z

Another issue is also about the fill of circle.

Thx, good catch! Fixed.

KonstantinEpam23 · 2023-05-20T07:03:35Z

packages/ketcher-core/src/application/render/options.js

@@ -24,7 +24,7 @@ function defaultOptions(opt) {
  if (opt.rotationStep) utils.setFracAngle(opt.rotationStep)


In options.js we just change the values to suit the new UX

KonstantinEpam23 · 2023-05-20T07:11:23Z

packages/ketcher-core/src/application/render/restruct/rebond.ts

@@ -57,18 +57,167 @@ class ReBond extends ReObject {
    return true
  }

+  static bondRecalc(bond: ReBond, restruct: ReStruct, options: any): void {


This function is just converted from member to static function, b/c we need to recalculate bond positions after they moved (it is used later in dropAndMerge.ts)

KonstantinEpam23 · 2023-05-20T07:53:32Z

packages/ketcher-react/src/script/editor/tool/helper/dropAndMerge.ts

+  for (const bondId of bonds) {
+    const rebond = restruct.bonds.get(bondId)
+    if (rebond) {
+      ReBond.bondRecalc(rebond, restruct, editor.options())


Here we add bond recalculation b/c without it the position values for halfBonds do not update

KonstantinEpam23 · 2023-05-22T07:45:56Z

packages/ketcher-core/src/application/render/restruct/rebond.ts

  drawHover(render: Render) {
    const ret = this.makeHoverPlate(render)
    render.ctab.addReObjectPath(LayerMap.hovering, this.visel, ret)
    return ret
  }

+  getSelectionPoints(render: Render) {
+    const bond: Bond = this.b


We render both regular and stereo bonds using the same SVG path, just with slightly different parameters.
Both bond types use two lines and two bezier curves to draw their paths:

KonstantinEpam23 · 2023-05-22T07:49:17Z

packages/ketcher-core/src/application/render/restruct/rebond.ts

+      halfBondEnd,
+      -bondThickness * 2.5
+    )
+


KonstantinEpam23 · 2023-05-22T07:55:58Z

packages/ketcher-core/src/application/render/restruct/rebond.ts

+    // rotate the points +/-90 degrees to find the
+    // perpendicular points that will be used for actual drawing
+    // of selection contour on canvas
+    const startTop = startPoint.rotateAroundOrigin(


KonstantinEpam23 · 2023-05-22T07:56:20Z

packages/ketcher-core/src/application/render/restruct/rebond.ts

+      angle - 90,
+      contourPaddedEnd
+    )
+


KonstantinEpam23 added the Do not merge label May 15, 2023

KonstantinEpam23 self-assigned this May 15, 2023

KonstantinEpam23 force-pushed the 2088-selection-tool-use-rounded-rectangles-for-selection-of-bonds-and-atom-labels branch from 51e7670 to 216c45b Compare May 16, 2023 09:15

KonstantinEpam23 removed the Do not merge label May 16, 2023

KonstantinEpam23 marked this pull request as ready for review May 17, 2023 08:15

auto-assign bot requested review from Nitvex and VasilevDO May 17, 2023 08:15

#2088 Selection Tool: use rounded rectangles for selection of bonds a…

6ec58d3

…nd atom labels

KonstantinEpam23 force-pushed the 2088-selection-tool-use-rounded-rectangles-for-selection-of-bonds-and-atom-labels branch from 3c900d6 to 6ec58d3 Compare May 19, 2023 05:43

KonstantinEpam23 requested a review from yuleicul May 19, 2023 06:00

yuleicul reviewed May 19, 2023

View reviewed changes

Nitvex reviewed May 19, 2023

View reviewed changes

KonstantinEpam23 added 2 commits May 19, 2023 13:18

#2088 Selection Tool: fix bug

d41af8e

#2088 Selection Tool: fix review comments

1e19987

KonstantinEpam23 commented May 20, 2023

View reviewed changes

#2088 Selection Tool: remove unused function

f62468f

KonstantinEpam23 commented May 20, 2023

View reviewed changes

#2088 Selection Tool: update comments

596488c

KonstantinEpam23 commented May 22, 2023

View reviewed changes

KonstantinEpam23 added 2 commits May 22, 2023 10:11

#2088 Selection Tool: update pseudo atoms selection

7558370

#2088 Selection Tool: fix for single-letter atoms

0086eac

Nitvex approved these changes May 22, 2023

View reviewed changes

#2088 Selection Tool: put docs images into repo

dd137d8

KonstantinEpam23 merged commit c8f9479 into master May 22, 2023

KonstantinEpam23 deleted the 2088-selection-tool-use-rounded-rectangles-for-selection-of-bonds-and-atom-labels branch May 22, 2023 08:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

#2088 Selection Tool: use rounded rectangles for selection of bonds and atom labels #2602

#2088 Selection Tool: use rounded rectangles for selection of bonds and atom labels #2602

KonstantinEpam23 commented May 15, 2023 •

edited

Loading

yuleicul left a comment •

edited

Loading

yuleicul left a comment •

edited

Loading

Nitvex May 19, 2023

Nitvex May 19, 2023

KonstantinEpam23 May 19, 2023

yuleicul May 22, 2023 •

edited

Loading

Nitvex May 19, 2023

Nitvex May 19, 2023

KonstantinEpam23 commented May 19, 2023

KonstantinEpam23 May 20, 2023 •

edited

Loading

KonstantinEpam23 May 20, 2023

KonstantinEpam23 May 20, 2023

KonstantinEpam23 May 22, 2023

KonstantinEpam23 May 22, 2023

KonstantinEpam23 May 22, 2023

KonstantinEpam23 May 22, 2023

		@@ -24,7 +24,7 @@ function defaultOptions(opt) {
		if (opt.rotationStep) utils.setFracAngle(opt.rotationStep)

#2088 Selection Tool: use rounded rectangles for selection of bonds and atom labels #2602

#2088 Selection Tool: use rounded rectangles for selection of bonds and atom labels #2602

Conversation

KonstantinEpam23 commented May 15, 2023 • edited Loading

yuleicul left a comment • edited Loading

Choose a reason for hiding this comment

yuleicul left a comment • edited Loading

Choose a reason for hiding this comment

Scenario 1

Reproduction steps

Actual behavior

Expected behavior

Scenario 2

Reproduction steps

Actual behavior

Expected behavior

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yuleicul May 22, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KonstantinEpam23 commented May 19, 2023

KonstantinEpam23 May 20, 2023 • edited Loading

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

KonstantinEpam23 commented May 15, 2023 •

edited

Loading

yuleicul left a comment •

edited

Loading

yuleicul left a comment •

edited

Loading

yuleicul May 22, 2023 •

edited

Loading

KonstantinEpam23 May 20, 2023 •

edited

Loading