Fixed drawing of O in aldehydes. #5515
Conversation
|
@DavidACosgrove Thanks for taking care of this. However, in #5511 you can see that previously the aldehyde was being drawn as if it were exocyclic, while here it is still drawn as endocyclic, thought the O is in a better position. Wouldn't it make sense to draw it as exocyclic rather than fixing how it is drawn as endocyclic? |
|
I prefer it as it's drawn now, I never liked the way the double bond straddled end of the single bond. Also, and crucially, the guys at Glysade who paid for the latest round of enhancements to the code prefer it. It's the way it's done in at least 1 widely used commercial sketcher. |
|
@DavidACosgrove You are right, ChemDraw does the same, and I agree it looks nicer. Thanks Dave for clarifying! Tomorrow I'll make some time to build your PR so I can test it locally. |
Code/GraphMol/MolDraw2D/DrawMol.cpp
Outdated
| auto l2 = (l2s - l2f).lengthSq(); | ||
| if ((bond->getBeginAtom()->getDegree() == 1 || | ||
| bond->getEndAtom()->getDegree() == 1) && | ||
| !(cols.first == cols.second) && fabs(l1 - l2) > 0.01) { |
There was a problem hiding this comment.
| !(cols.first == cols.second) && fabs(l1 - l2) > 0.01) { | |
| (cols.first != cols.second) && fabs(l1 - l2) > 0.01) { |
There was a problem hiding this comment.
Operator != isn't defined for DrawColour. I have thought about making one, but it feels like a bit of code bloat to solve something that is a only a question of ugly style, really, and not used often.
There was a problem hiding this comment.
I usually define the != operator negating the == operator as you did above. I think that makes code using the class a bit more readable as the ugliness stays in the operator definition, but it is definitely not a must.
There was a problem hiding this comment.
I will put one in. I was in two minds about it when I wrote the code, and your extra vote has convinced me.
| if ((bond->getBeginAtom()->getDegree() == 1 || | ||
| bond->getEndAtom()->getDegree() == 1) && | ||
| !(cols.first == cols.second) && fabs(l1 - l2) > 0.01) { | ||
| double midlen = sqrt(l1) / 2.0; |
There was a problem hiding this comment.
| double midlen = sqrt(l1) / 2.0; | |
| double midlen = sqrt(l1) * 0.5; |
There was a problem hiding this comment.
I always let the compiler sort out this sort of micro-optimisation. Personally I think the intention is more immediately obvious with / 2.0 than * 0.5 but that might just be me. I can change it if it's house style.
There was a problem hiding this comment.
More than a micro-optimization my suggestion was intended for consistency with the * 0.5 that is used a few lines below (line 2799), but again, not a must.
There was a problem hiding this comment.
Ah, sorry, I missed that. Consistency within a function is very desirable. I will alter it. If nothing else, that will force another run of the CI checks which will hopefully pass this time.
There was a problem hiding this comment.
At a quick scan there is pretty much a 50:50 mixture of / 2.0 and * 0.5 through this file. I think I will leave this particular instance, but aim for more consistency in future.
|
I don't think these failures are real. I do all my development on macOS, and it build and tested fine on my machine. |
|
Confilcts fixed. I'll correct the hashcodes after fixing the conflicts for #5534 after this is merged. |
|
@DavidACosgrove : I fixed the more recent conflicts on this one. |
|
Sorry, I thought that one was done.
On Sun, 11 Sep 2022 at 04:43, Greg Landrum ***@***.***> wrote:
@DavidACosgrove <https://github.com/DavidACosgrove> : I fixed the more
recent conflicts on this one.
Will merge assuming the CI builds pass.
—
Reply to this email directly, view it on GitHub
<#5515 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/ACGF2FWC47ALWMQVOSPWRJLV5VBOLANCNFSM57OQI6ZA>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
--
David Cosgrove
Freelance computational chemistry and chemoinformatics developer
http://cozchemix.co.uk
|
Reference Issue
Fixes 5511
What does this implement/fix? Explain your changes.
Moves the atom label to centre between the 2 lines of the aldehyde double bond.
Any other comments?
Also, makes the change of colour of the two lines match up, which was attempted previously but had a bug in it.
If should how look like this:
The minor residual wonkiness in the O position is due to the catch tests not using Freetype. It looks better when drawn with a Freetype font.