There are a few pull requests that I’m not necessarily inclined to merge roughly one week before 2.0:
- When changing atoms, bonds, etc. invalidate surfaces, spectra, etc. by ghutchis · Pull Request #2752 · OpenChemistry/avogadrolibs · GitHub
- This fixes a bug report #2618 - but has the potential to introduce possible race conditions if surfaces or other behavior is running
- IMHO we should wait until after 2.0 to merge and test more carefully
- Current behavior is wrong, but much worse to introduce potential crashes, etc.
- Improve Gaussian basis calculations with class to store metadata, inc. cutoff by ghutchis · Pull Request #2726 · OpenChemistry/avogadrolibs · GitHub
- This significantly improves speed for orbitals and density ~25x faster on my laptop
- Passes unit tests, so likely correct, but I’m careful about introducing subtle bugs before the 2.0 release
- IMHO we can wait until after 2.0 - current performance for orbitals is still pretty fast
- Calculating electron density is currently slow, but a less common task
- Fix wireframe render by ghutchis · Pull Request #2753 · OpenChemistry/avogadrolibs · GitHub
- This fixes a bug with thin wireframe lines after migrating to OpenGL 4.0 core profile
- Evidently, even though OpenGL has a line width call, implementations can basically ignore it
- I’m not thrilled with the look, but IMHO it’s better than having super-thin lines that can’t be adjusted in the display options
1 Like
I think the first is not that high of a priority since it has known bugs, but the second and third I think are worth discussing.
Wireframes are a pretty core feature, they live right in with all of the other display types, but to be honest I rarely find myself using them. I think they live mostly in the biochem community, though I have no clue how large that actually is. I’ve seen both the old and new versions, and while the new version is definitely nicer in the sense that trying to change the thickness actually does change the thickness, I’m not sure if it is visually much better. That being said, I still think it’s worth merging.
The electron density is actually something I personally would really like to see improved. I think that while a 20-30x speedup is definitely worth it, it would definitely not be great to introduce potentially subtle bugs that might affect orbitals, which are far, far more common. Overall I feel pretty 50:50 about this one. I’d love for some more extensive testing to get done, but I won’t lose any sleep (any more sleep than I already do) if it doesn’t get merged.
It’s not just the wireframe render style. The underlying rendering is also used in the Close Contact and Non-Covalent styles (e.g., hydrogen bonds) the cell borders for the Crystal Lattice, and the wireframe style for surfaces. I probably wouldn’t bother, but we have a variety of controls for changing the line thickness which currently don’t do anything.
As far as which ones get merged, that’s basically my thought as well (wireframes = useful merge) but the other two are probably safer to merge after the 2.0 release to ensure better testing.
1 Like