Acylation / obsidian-chem

Chemistry support for Obsidian. Rendering SMILES strings into chemistry structures.
MIT License
74 stars 4 forks source link

Please use cut-offs for line colours instead of gradients #46

Closed Alicecomma closed 6 months ago

Alicecomma commented 9 months ago

What would you like to be added?

In createGradient(line) modify the stops as below, so it shows cut-offs (PubChem-like) rather than gradients (firstStop offset => 0%, added inbetween1 and inbetween2 definitions with colour of first- and secondStop respectively, at 50% offset; gradient appends stops inbetween1 and -2).

        let firstStop = document.createElementNS("http://www.w3.org/2000/svg", "stop");
        firstStop.setAttributeNS(null, "stop-color", this.themeManager.getColor(line.getLeftElement() || this.themeManager.getColor("C")));
        firstStop.setAttributeNS(null, "offset", "0%");
        let inbetween1 = document.createElementNS("http://www.w3.org/2000/svg", "stop");
        inbetween1.setAttributeNS(null, "stop-color", this.themeManager.getColor(line.getLeftElement() || this.themeManager.getColor("C")));
        inbetween1.setAttributeNS(null, "offset", "50%");
        let inbetween2 = document.createElementNS("http://www.w3.org/2000/svg", "stop");
        inbetween2.setAttributeNS(null, "stop-color", this.themeManager.getColor(line.getRightElement() || this.themeManager.getColor("C")));
        inbetween2.setAttributeNS(null, "offset", "50%");
        let secondStop = document.createElementNS("http://www.w3.org/2000/svg", "stop");
        secondStop.setAttributeNS(null, "stop-color", this.themeManager.getColor(line.getRightElement() || this.themeManager.getColor("C")));
        secondStop.setAttributeNS(null, "offset", "100%");
        gradient.appendChild(firstStop);
        gradient.appendChild(inbetween1);
        gradient.appendChild(inbetween2);
        gradient.appendChild(secondStop);

I don't know how you would add settings to the plugin to have a checkbox for these changes.

Without change (washed-out, ugly render due to anti-aliased background interfering with an inherently poor colour gradient): image

With change (sharp cut-off, PubChem-like renders): image

Another change is also possible that only keeps the atom colours but not the bond colours. This is a much nicer option than "oldschool", again in my opinion (firstStop offset => 0%; getColor is always carbon rather than line element colors).

        let firstStop = document.createElementNS("http://www.w3.org/2000/svg", "stop");
        firstStop.setAttributeNS(null, "stop-color", this.themeManager.getColor(this.themeManager.getColor("C")));
        firstStop.setAttributeNS(null, "offset", "0%");
        let secondStop = document.createElementNS("http://www.w3.org/2000/svg", "stop");
        secondStop.setAttributeNS(null, "stop-color", this.themeManager.getColor(this.themeManager.getColor("C")));
        secondStop.setAttributeNS(null, "offset", "100%");
        gradient.appendChild(firstStop);
        gradient.appendChild(secondStop);

With change (line colour matches carbon - least cluttered): image

Why is this needed?

Having colours is very helpful in reading a structure, but gradients make it significantly worse. Introducing cut-offs or single-coloured lines like this helps readability a ton at least for me personally. The modifications are minimal to the original package used for visualization (smilesDrawer), as it only changes the createGradient method. The gradient code is simple to work with, and all this does is change the gradient stops.

Acylation commented 9 months ago

I think to implement this we would need to maintain a fork of the original package. Thanks for your experimenting codes.

There's a plan to support rdkit.js as a more robust core, and this package uses hard cut-off that you requested. Would this inclusion meet your requirements? If so we could consider prior the development and using the new core as a temporary solution.

Demo page here: https://react.rdkitjs.com/

Alicecomma commented 8 months ago

RDKit should meet these requirements, yes ✅

Acylation commented 6 months ago

Will be fixed by #61