Merge origin/main into wip/217-geomtrace

Author: andy

docs/claude/wx-viewer/bond-cap-test-was-quadratic.md

@@ -0,0 +1,33 @@
+---
+type: pitfall
+title: "Ball and Stick spent most of its frame in the bond-cap test, O(atoms x bonds); the test itself is not a start/count range test"
+area: wx-viewer
+paths: [src/inv/moiv/ChemUnitCylinder.C, src/inv/moiv/ChemDisplay.C, include/inv/ChemKit/ChemDisplay.H, src/viz/sgcommands/SGContainer.C, tools/viewer-bench/viewer-bench.C]
+issues: [224]
+---
+`ChemUnitCylinder::render`, `renderHalfBonded` and
+`renderWithInterpolatedColor` ask, per bond end, whether to draw the
+cylinder's end cap (`PRE_RENDER_CAP`). That test scanned every entry of
+`ChemDisplay::atomIndex`, and `SGContainer` fills `atomIndex` with one
+`(atom, 1)` entry per atom, so a frame cost atoms x bonds field reads:
+about 800M instructions per frame for the 5184-atom water box, which is
+the gap between Ball and Stick (171 ms on an RTX 3050) and CPK (20 ms).
+Now `ChemDisplay::GLRender` reduces `atomIndex` to two bounds once per
+frame and `bondCapAtAtom()` answers in O(1).
+
+The old test matches an entry when `count == -1 && atom >= start`, or
+when `atom <= start && atom <= count`, which is not "atom lies in
+[start, start+count)". With per-atom entries only atoms 0 and 1 lose their
+caps; every other cap is drawn inside its sphere. `bondCapAtAtom` keeps
+that result exactly; fixing the test would draw less but can change pixels,
+so check it with `tools/coin/scenes/styles.scene` (`waterbox` too) first.
+
+The sibling `ADJUST_CYLINDER` in `ChemDisplayCylinders*.C` has the same
+range test but breaks on the first non-matching entry, so it is cheap.
+
+Measuring: `BENCH_STYLES="CPK,Ball And Stick,..."` runs only the water
+box. On llvmpipe the frame time is dominated by rasterisation and is too
+noisy under load to show this; count instructions instead
+(`valgrind --tool=callgrind --toggle-collect='ChemDisplay::GLRender*'`).
+`perf` is not available to users on Debian (`perf_event_paranoid=3`).
+`GALLIUM_NOOP=1` makes the benchmark exit silently.

docs/claude/wx-viewer/index.md

@@ -35,6 +35,7 @@ Read this before changing any dialog, panel, sizer, grid, ewx control or the Ope
 - [`SoWxRenderArea::renderCB` during a paint defers the redraw through `p_redrawPending`](sowxrenderarea-rendercb-silently-drops-a-redraw.md)
 - [A static `EVT_RADIOBOX` entry never reaches an `ewxRadioBox`'s panel; Bind on the widget (#81)](resolved-81-fixed-9a3004e-confirmed-live-2026.md)
 - [ChemDisplay's `glPopAttrib` undoes what Coin's lazy element sent inside it; the first offscreen render drew an ESP surface unlit](chemdisplay-glpopattrib-undoes-lazy-element-sends.md)
+- [Ball and Stick spent most of its frame in the bond-cap test, O(atoms x bonds) (#224)](bond-cap-test-was-quadratic.md)
 - [The unit cell is drawn only while the Periodic Builder panel is open](unit-cell-drawn-only-by-the-periodic-builder.md)
 - [The Vibrational Frequencies graph is `SpectrumCanvas` over the wx-free `VibSpectrum` model (#214)](vibrational-spectrum-canvas-214.md)
 - [Builder panel layouts (View > Panel layout): every pane stays an AUI pane; the one-column modes hide the inactive tab's panes](builder-panel-layouts-one-column.md)

include/inv/ChemKit/ChemDisplay.H

@@ -215,6 +215,11 @@ class CHEMKIT_DLL_API ChemDisplay : public SoNonIndexedShape {
     const SbMatrix   &getCurrentMVPMatrix() const
         { return currentMVP; }
 
+    // Whether a bond cylinder draws its end cap at this atom (not for
+    // DISPLAY_STICK, which always does). Valid during GLRender only.
+    SbBool           bondCapAtAtom(int32_t atom) const
+        { return !(atom >= capRestStart || atom <= capLowMax); }
+
   protected:
     virtual void    generatePrimitives(SoAction *action);
     virtual void    computeBBox(SoAction *action, SbBox3f &box,
@@ -1551,6 +1556,10 @@ private:
 	MFVec2i vnormalResidueLabelIndex,vhighlightResidueLabelIndex;
 	friend class ChemOctreeNode;
 
+  // atomIndex reduced for bondCapAtAtom, once per GLRender
+  int32_t capRestStart, capLowMax;
+  void summarizeAtomIndexForCaps();
+
   // duplicates
   bool* renderedBonds;
   bool* renderedResidues;

src/inv/moiv/ChemDisplay.C

@@ -1,5 +1,6 @@
 #include <stdio.h>
 #include <stdlib.h>
+#include <stdint.h>
 #include <iostream>
 using namespace std;
 /*
@@ -384,6 +385,8 @@ ChemDisplay::ChemDisplay()
 	octreenode = new ChemOctreeNode();
   // duplicates
   renderedBonds = NULL;
+  capRestStart = INT32_MAX;
+  capLowMax = INT32_MIN;
   renderedResidues = NULL;
 // <-- octree culling
 // --> improving ribbon speed
@@ -816,6 +819,7 @@ ChemDisplay::GLRender(SoGLRenderAction *action)
 	if (!action->isRenderingDelayedPaths()) {
 		generateIndices(action);
 	}
+	summarizeAtomIndexForCaps();
 
 // --> EGB && SGB
 	lodSelector->resetAtoms(chemData->getNumberOfAtoms());
@@ -6512,3 +6516,34 @@ void ChemDisplay::eachBBoxResiduesAsCylinders(SoState *state, ChemDisplayParam *
 #undef ATOMLOOP_END
 #undef BONDLOOP_START
 #undef BONDLOOP_END
+
+
+////////////////////////////////////////////////////////////////////////
+//
+// Description:
+//    Reduces atomIndex to two bounds so that bondCapAtAtom is O(1).
+//    The cylinder code asked this per bond end by scanning every
+//    atomIndex entry (one per atom in the Builder), O(atoms x bonds) per
+//    frame. The result is exactly the old scan's: a cap is drawn unless
+//    some entry matches, where an entry matches if count == -1 and
+//    atom >= start, or else if atom <= start and atom <= count. (That is
+//    not the start/count range test; changing it would change the image.)
+//
+// Use: private
+
+void
+ChemDisplay::summarizeAtomIndexForCaps()
+{
+	capRestStart = INT32_MAX;
+	capLowMax = INT32_MIN;
+	const SbVec2i *ranges = atomIndex.getValues(0);
+	for (int i = 0, n = atomIndex.getNum(); i < n; i++) {
+		int32_t start = ranges[i][0], count = ranges[i][1];
+		if (count == -1) {
+			if (start < capRestStart) capRestStart = start;
+		} else {
+			int32_t low = (start < count) ? start : count;
+			if (low > capLowMax) capLowMax = low;
+		}
+	}
+}

src/inv/moiv/ChemUnitCylinder.C

@@ -228,18 +228,8 @@ renderSoCylinder(SoCylinder *cyl, SoGLRenderAction *action)
 
 // --> roundcap optimization
 #define PRE_RENDER_CAP(ATOM,BOOLEAN) \
-BOOLEAN = true;  \
-if (cdp->displayStyle.getValue() != ChemDisplayParam::DISPLAY_STICK) \
-{ \
-int __i; \
-for (__i=0; __i<cd->atomIndex.getNum();__i++) \
-{ \
-const SbVec2i range = *cd->atomIndex.getValues(__i); \
-if (range[1] == -1) BOOLEAN  = !(ATOM >= range[0]); \
-else BOOLEAN = !(range[0]>=ATOM && ATOM<=range[1]); \
-if (!BOOLEAN) break; \
-} \
-}
+BOOLEAN = (cdp->displayStyle.getValue() == ChemDisplayParam::DISPLAY_STICK) \
+	|| cd->bondCapAtAtom(ATOM);
 // <-- roundcap optimization
 
 ////////////////////////////////////////////////////////////////////////
@@ -2555,7 +2545,6 @@ void ChemUnitCylinder::renderHalfBonded(
 // --> cap optimization
 	ChemDisplayParam* cdp = ChemDisplayParamElement::get(action->getState());
 	bool renderFrom, renderTo;
-        //KLS VERY SLOW
 	PRE_RENDER_CAP(fromTo[0],renderFrom);
 	PRE_RENDER_CAP(fromTo[1],renderTo);
 // <-- cap optimization

tools/viewer-bench/README.md

@@ -25,6 +25,8 @@ build-cmake/viewer-bench-<host>-<date>.txt
 Send that file. Leave the window uncovered while it runs. Optional:
 `BENCH_FRAMES=360 BENCH_SECONDS=8 tools/viewer-bench/run.sh` (frames per run,
 time cap per run; defaults 180 and 4 s, 10 warm-up frames not counted).
+`BENCH_STYLES="CPK,Ball And Stick,Stick,Wireframe,Ball And Wireframe"` times
+only the water box, in those styles, with caching AUTO.
 
 ## What is timed
 

tools/viewer-bench/viewer-bench.C

@@ -438,17 +438,29 @@ int BenchApp::OnRun()
     {"water box", "Ball And Stick", false}, {"water box", "CPK", false},
   };
 
+  //  BENCH_STYLES="CPK,Stick,...": only the water box, in those styles,
+  //  caching AUTO (for profiling one style at a time).
+  vector<Config> todo(configs, configs + sizeof configs / sizeof configs[0]);
+  const char *only = getenv("BENCH_STYLES");
+  if (only) {
+    todo.clear();
+    std::istringstream ss(only);
+    string st;
+    while (std::getline(ss, st, ','))
+      if (!st.empty()) todo.push_back(Config{"water box", st, false});
+  }
+
   vector<Result> results;
-  for (size_t i = 0; i < sizeof configs / sizeof configs[0]; i++) {
+  for (size_t i = 0; i < todo.size(); i++) {
     string err;
-    if (!loadScene(configs[i], err)) {
-      fprintf(stderr, "skipping %s: %s\n", configs[i].system.c_str(),
+    if (!loadScene(todo[i], err)) {
+      fprintf(stderr, "skipping %s: %s\n", todo[i].system.c_str(),
               err.c_str());
       continue;
     }
-    for (int cache = 1; cache >= 0; cache--) {
+    for (int cache = 1; cache >= (only ? 1 : 0); cache--) {
       Result r;
-      if (measure(configs[i], cache != 0, warm, frames, budget, r))
+      if (measure(todo[i], cache != 0, warm, frames, budget, r))
         results.push_back(r);
       Yield(true);
     }