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

Partial 2D Layout #205

Merged
merged 36 commits into from Jul 8, 2016
Merged

Partial 2D Layout #205

merged 36 commits into from Jul 8, 2016

Conversation

johnmay
Copy link
Member

@johnmay johnmay commented Jun 8, 2016

Allows laying out of a structure when part of it has already been laid out. Or to put it another way, we can set the coordinates of a substructure and lock those in place. This allows structures to be aligned to a common substructure. There's also a critical patch in there that i'll make a release for right away.

This one has been a long time coming and there might be a few more bits to round up but seems to be working well. Something we can do automatically is align reaction reactants/products, with a incorrect atom-atom mapping (i.e. automatic from MCS) it can look awful and can be disabled, it generally works well.

Before:
image
After:
image

Before:
image
After:
image
image

johnmay added 26 commits June 5, 2016 18:18
…istic. Also we sort the ring set a few lines later on by bond count.
…hod call without the Java bean style getter/setter.

addAngle = addAngle * direction;
atomPlacer.populatePolygonCorners(atomsToDraw, ringCenter, startAngle, addAngle, radius);
logger.debug("placeBridgedRing->atomsToPlace: " + AtomPlacer.listNumbers(molecule, atoms));
Copy link
Member

@egonw egonw Jul 8, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can use a comma instead of a "+" here, which makes the code run faster when debugging is turned of:

logger.debug("placeBridgedRing->atomsToPlace: ", AtomPlacer.listNumbers(molecule, atoms));

It saves a bit of time but mostly a unneeded extra String in that case.

(This can be cleaned up later... it won't block merging the patch in...)

(BTW, not sure how fast the listNumbers() call is, but if that's relatively slow, you can even wrap the logger.debug() calls in an if logger.debugEnabled() {} or so...)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will update, but not my code :-) .. but yes I know the API but guess this was written before the logger. Incidentally string concatenation isn't super bad, the byte code inserts a string builder.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, it's a bit hard to see what code in the rendering stack is not yours by now :)

But then please don't worry! Let me fix it then.

@egonw egonw merged commit bc1c053 into master Jul 8, 2016
@johnmay johnmay deleted the patch/sdgsublayout branch August 18, 2016 14:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants