#6215 – Introducing the snap to angle and standard bond length for monomers connected via bonds #6570
Conversation
auto-assign
bot
requested review from
NataliaLoginova,
rrodionov91 and
Zhirnoff
svvald
changed the title
| snapPosition, | ||
| ).length(); | ||
|
|
||
| if (distanceToSnapPosition < 0.375) { |
There was a problem hiding this comment.
Maybe move 0.375 to constant and use here and I remember the same somewhere in curved bonds
| SelectRectangle.calculateAngleSnap( | ||
| cursorPositionInAngstroms, | ||
| connectedMonomer.position, | ||
| this.editor.mode.modeName === 'snake-layout-mode' ? 90 : 30, |
There was a problem hiding this comment.
Maybe use this.editor.mode instanceof SnakeMode?
| if (isAngleSnapped && isDistanceSnapped) { | ||
| snapPosition = distanceSnapPosition; | ||
| } else if (isAngleSnapped) { | ||
| snapPosition = angleSnapPosition; | ||
| } else if (isDistanceSnapped) { | ||
| snapPosition = distanceSnapPosition; | ||
| } |
There was a problem hiding this comment.
Very clear what happens in this method. Thank you
| this.topLayer.selectAll('*').remove(); | ||
| this.defaultLayer.selectAll('*').remove(); |
There was a problem hiding this comment.
Not sure how * selector works for d3, but potentially it could be a cause of performance degradation in some cases. Let's just keep it in mind for now.
There was a problem hiding this comment.
Maybe it would be better to organize elements somehow to delete only group element
There was a problem hiding this comment.
I had this concern as well but I considered it shouldn't be a bottleneck considering each layer will hardly contain more than 5-10 elements at once, so it wouldn't be an issue. Also, according to the docs, D3 selectAll function is quite efficient
We can surely keep it in mind if any performance drops will arise
svvald
force-pushed
the
6215-introducing-the-snap-to-angle-and-standard-bond-length-for-monomers-connected-via-bonds
branch
2 times, most recently
from
885f752 to
250b9f1
Compare
svvald
force-pushed
the
6215-introducing-the-snap-to-angle-and-standard-bond-length-for-monomers-connected-via-bonds
branch
from
dacf870 to
4ad7940
Compare
svvald
deleted the
6215-introducing-the-snap-to-angle-and-standard-bond-length-for-monomers-connected-via-bonds
branch
How the feature works? / How did you fix the issue?
(Screenshots, videos, or GIFs, if applicable)
Check list
#1234 – issue name