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.

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

Incorrect positioning of data s-group labels #2009

Closed
Zhirnoff opened this issue Dec 27, 2022 · 3 comments · Fixed by #2732
Closed

Incorrect positioning of data s-group labels #2009

Zhirnoff opened this issue Dec 27, 2022 · 3 comments · Fixed by #2732
Assignees
Labels

Comments

@Zhirnoff
Copy link
Collaborator

Zhirnoff commented Dec 27, 2022

Steps to Reproduce

  1. Launch Ketcher
  2. Create chain structure or Benzene
  3. Highlight all structure by Selection tool
  4. Click on 'Data S-Group' button
  5. In 'S-Group Properties' window select from drop-down menu: 'Context-Atom or Bond', 'Field name-A', 'Field value-2', 'Radio button-Absolute or Relative'
  6. Click 'Apply' button
  7. Look at structure

Actual behavior
Adding to Atom or Bond a Data S-Group the attachment point on the structure is incorrect

Expected behavior
Adding to Atom or Bond a Data S-Group the attachment point on the structure is correct: Atom value on atom, Bond value on bond
Mapping for Atoms:
2022-12-27_14h51_44
Mapping for Bonds:
2022-12-27_14h53_31

Screenshots
2022-12-27_15h00_11
2022-12-27_15h02_09

2022-12-27_15h10_53.mp4

Desktop (please complete the following information):

  • OS: Windows 10
  • Browser Chrome
  • Version 108.0.5359.125 (Official Build) (64-bit)

Ketcher version
Version 2.7.0-rc.7

@Zhirnoff Zhirnoff added the bug label Dec 27, 2022
@Zhirnoff Zhirnoff added this to the Release 2.8.0 milestone Dec 27, 2022
@Nitvex
Copy link
Collaborator

Nitvex commented Jan 16, 2023

Issue is reproduced on 2.7.0-rc.9, moving to 2.9.0

@KonstantinEpam23 KonstantinEpam23 changed the title Data S-Group: Adding to Atom or Bond a Data S-Group values the attachment point on the structure is incorrect Incorrect positioning of data s-group labels Feb 24, 2023
@KonstantinEpam23 KonstantinEpam23 changed the title Incorrect positioning of data s-group labels [IMPORTANT] Incorrect positioning of data s-group labels Apr 27, 2023
@KonstantinEpam23 KonstantinEpam23 changed the title [IMPORTANT] Incorrect positioning of data s-group labels Incorrect positioning of data s-group labels May 25, 2023
@gairon gairon assigned gairon and unassigned Nitvex May 26, 2023
@KonstantinEpam23
Copy link
Collaborator

KonstantinEpam23 commented May 30, 2023

Bond data labels should be drawn in the middle of the bond over it.
Atom label positions should stay as they are now.

gairon added a commit that referenced this issue Jun 1, 2023
* Fix Box2Abs.segmentIntersection to check collinial segments those might not intersect
* Draw bond data labels in the middle of the bond over it
@gairon
Copy link
Contributor

gairon commented Jun 1, 2023

Box2Abs.segmentIntersection was fixed a bit to exclude situations when segments collinear and might or might not have a common point without a real intersection. It potentially might affect Struct.loopHasSelfIntersections results but I have no idea how to check if it's still correct or not. This function is used in Struct.findLoops and triggered in situations when a user is trying to intersect a loop by its own bond:
image
Do we need to consider a common point as an intersection here or not?

gairon added a commit that referenced this issue Jul 10, 2023
* Fix Box2Abs.segmentIntersection to check collinial segments those might not intersect
* Draw bond data labels in the middle of the bond over it
gairon added a commit that referenced this issue Jul 10, 2023
* Fix Box2Abs.segmentIntersection to check collinial segments those might not intersect
* Draw bond data labels in the middle of the bond over it
gairon added a commit that referenced this issue Jul 10, 2023
* Fix Box2Abs.segmentIntersection to check collinial segments those might not intersect
* Draw bond data labels in the middle of the bond over it
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants