Finish user-defined colour leak fix; add --by-name test option

bcc3af8 stopped user-defined atom colours leaking into other colouring
modes in atom_colour(), but two other code paths still applied them
regardless of mode:

- make_colour_table() (coot-molecule-bonds.cc) overrode the colour table
  with user-defined bond colours for any matching colour index in every
  mode. This is what made user colour 51 appear on chain A in
  COLOUR-BY-CHAIN-AND-DICTIONARY. Now gated on the user-defined bonds-box
  type.
- the get_atom_colour_index lambda in do_Ca_or_P_bonds_internal()
  (Bond_lines.cc) read the user-colour UDD unconditionally. Now gated on
  bond_colour_type == COLOUR_BY_USER_DEFINED_COLOURS.

test-molecules-container: add a "--by-name <test-name>" option to run a
single test (with a no-match guard), and update the three user-defined
colour tests (v2, v3, "New colour test") to assert that user-defined
colours do NOT leak into chain/VDW modes - the behaviour they previously
relied on. All 137 tests pass.

Author: pemsley

api/coot-molecule-bonds.cc

@@ -1492,8 +1492,14 @@ coot::molecule_t::make_colour_table(bool dark_bg_flag) const {
             colour_table[icol] = cc.to_glm();
             done_colours.insert(icol);
 
-            // was there a user-defined bond colour that superceeds this for this colour index?
-            if (! user_defined_bond_colours.empty()) {
+            // Was there a user-defined bond colour that supersedes this for this colour
+            // index? Apply it only in a user-defined colouring mode - user-defined colours
+            // must not leak into other colouring modes (e.g. colour-by-chain).
+            // c.f. Bond_lines_container::atom_colour().
+            bool user_defined_colour_mode =
+               (bonds_box_type == coot::api_bond_colour_t::COLOUR_BY_USER_DEFINED_COLOURS____BONDS) ||
+               (bonds_box_type == coot::api_bond_colour_t::COLOUR_BY_USER_DEFINED_COLOURS_CA_BONDS);
+            if (user_defined_colour_mode && ! user_defined_bond_colours.empty()) {
 
                std::map<unsigned int, colour_holder>::const_iterator it;
                it = user_defined_bond_colours.find(icol);

api/test-molecules-container.cc

@@ -5684,19 +5684,20 @@ int test_user_defined_bond_colours_v2(molecules_container_t &mc) {
       if (ig.instancing_data_A.size() == 25) {
          // as it should be!
 
-         // The colour of the [11]th atom ("CA") should be 1,0,1
+         // VDW-BALLS is not a user-defined-colour mode, so the [11]th atom ("CA") must
+         // NOT pick up the user-defined colour (1,0,1) - user colours no longer leak
+         // into other colouring modes (see Bond_lines_container::atom_colour()).
 
          for (unsigned int i=0; i<25; i++) {
             const auto &sphere = ig.instancing_data_A[i];
             if (false)
                std::cout << "sphere " << i << " pos " << glm::to_string(sphere.position)
                          << " colour " << glm::to_string(sphere.colour) << std::endl;
             if (i == 11) { // the is "CA" the CA in the first residue (strangely)
-               status = 0;
-               if (close_float(sphere.colour[0], 1.0))
-                  if (close_float(sphere.colour[1], 0.0))
-                     if (close_float(sphere.colour[2], 1.0))
-                        status = 1;
+               bool is_user_colour = close_float(sphere.colour[0], 1.0) &&
+                                     close_float(sphere.colour[1], 0.0) &&
+                                     close_float(sphere.colour[2], 1.0);
+               status = is_user_colour ? 0 : 1;
             }
          }
 
@@ -5745,7 +5746,7 @@ int test_user_defined_bond_colours_v3(molecules_container_t &mc) {
 
    if (mc.is_valid_model_molecule(imol)) {
       std::map<unsigned int, std::array<float, 4> > colour_map;
-      colour_map[51] = {0.627, 0.529, 0.400};
+      colour_map[51] = {0.627, 0.529, 0.401};
       colour_map[52] = {0.424, 0.627, 0.400};
       colour_map[53] = {0.957, 0.263, 0.212};
       bool C_only = false;
@@ -5759,11 +5760,11 @@ int test_user_defined_bond_colours_v3(molecules_container_t &mc) {
 
       // now test the colours:
       auto bonds = mc.get_bonds_mesh_for_selection_instanced(imol, "/", mode, false, 0.2, 1.0, false, false, false, true, 1);
-      auto &geom = bonds.geom;
       auto ca = get_colour_analysis(bonds);
 
-      // colour 53 supercedes/replaces the others
-      //
+      // User-defined colours must NOT leak into COLOUR-BY-CHAIN-AND-DICTIONARY mode;
+      // they apply only in USER-DEFINED-COLOURS mode (see Bond_lines_container::atom_colour()).
+      // So none of the user-defined colours should appear here.
       bool col_51 = false;
       bool col_52 = false;
       bool col_53 = false;
@@ -5772,10 +5773,17 @@ int test_user_defined_bond_colours_v3(molecules_container_t &mc) {
          if (is_near_colour(ca_row.col, colour_map[51])) col_51 = true;
          if (is_near_colour(ca_row.col, colour_map[52])) col_52 = true;
          if (is_near_colour(ca_row.col, colour_map[53])) col_53 = true;
+         if (is_near_colour(ca_row.col, colour_map[51])) {
+            std::cout << "colour " << glm::to_string(ca_row.col) << " with " << ca_row.count
+                      << "counts is close to col 51" << std::endl;
+         }
       }
+      std::cout << "debug: test_user_defined_bond_colours_v3: col_51 " << col_51 << std::endl;
+      std::cout << "debug: test_user_defined_bond_colours_v3: col_52 " << col_52 << std::endl;
+      std::cout << "debug: test_user_defined_bond_colours_v3: col_53 " << col_53 << std::endl;
       if (col_51 == false)
          if (col_52 == false)
-            if (col_53 == true)
+            if (col_53 == false)
                status = true;
    }
 
@@ -5853,12 +5861,18 @@ int test_other_user_defined_colours_other(molecules_container_t &mc) {
          }
          std::vector<colour_analysis_row> ca_1 = get_colour_analysis(bonds_1);
          std::vector<colour_analysis_row> ca_3 = get_colour_analysis(bonds_3);
-         // different vec indices because UD colour becomes the first colour.
-         // This is weird. When run in "single test only" the indices need to
-         // be 4 and 3. It might have something to do with reading the ATP at the start.
-         //
-         if (ca_3[3].count == (ca_1[2].count - 85))
-            status = 1;
+         // Setting user-defined atom colours must NOT change COLOUR-BY-CHAIN-AND-DICTIONARY
+         // rendering - user-defined colours no longer leak into other colouring modes
+         // (see Bond_lines_container::atom_colour()). So the colour analysis before (ca_1)
+         // and after (ca_3) setting the user colours should be identical.
+         if (ca_1.size() == ca_3.size()) {
+            bool all_same = true;
+            for (unsigned int i=0; i<ca_1.size(); i++)
+               if (ca_1[i].count != ca_3[i].count)
+                  all_same = false;
+            if (all_same)
+               status = 1;
+         }
 
       }
    }
@@ -8192,6 +8206,9 @@ int test_template(molecules_container_t &mc) {
 int n_tests = 0;
 static std::vector<std::pair<std::string, int> > test_results;
 
+// If set (via "--by-name <test-name>"), only the test with this exact name is run.
+static std::string g_only_test_name;
+
 void
 write_test_name(const std::string &test_name) {
 
@@ -8203,6 +8220,9 @@ write_test_name(const std::string &test_name) {
 int
 run_test(int (*test_func) (molecules_container_t &mc), const std::string &test_name, molecules_container_t &mc) {
 
+   if (! g_only_test_name.empty() && test_name != g_only_test_name)
+      return 0; // --by-name: skip this test (not run, not counted)
+
    n_tests++;
    write_test_name(test_name);
    int status = test_func(mc);
@@ -8267,6 +8287,14 @@ int main(int argc, char **argv) {
       std::string arg(argv[1]);
       if (arg == "last-test-only")
          last_test_only = true;
+      if (arg == "--by-name") {
+         if (argc > 2) {
+            g_only_test_name = argv[2];
+         } else {
+            std::cout << "Usage: " << argv[0] << " --by-name <test-name>" << std::endl;
+            return 1;
+         }
+      }
    }
 
    int all_tests_status = 1; // fail!
@@ -8515,6 +8543,11 @@ int main(int argc, char **argv) {
          // status += run_test(test_density_mesh,          "density mesh",             mc);
          // status += run_test(test_molecular_placement_pipeline_r_chain, "MR R-chain", mc);
          status += run_test(test_molecular_placement_pipeline, "MR pipeline", mc);
+
+         if (! g_only_test_name.empty() && n_tests == 0) {
+            std::cout << "ERROR:: --by-name: no test matched \"" << g_only_test_name << "\"" << std::endl;
+            return 1;
+         }
          if (status == n_tests) all_tests_status = 0;
 
          print_results_summary();

coords/Bond_lines.cc

@@ -5039,13 +5039,17 @@ Bond_lines_container::do_Ca_or_P_bonds_internal(atom_selection_container_t SelAt
    int udd_user_defined_atom_colour_index_handle = SelAtom.mol->GetUDDHandle(mmdb::UDR_ATOM,
                                                                              "user-defined-atom-colour-index");
 
-   auto get_atom_colour_index = [udd_user_defined_atom_colour_index_handle] (mmdb:: Atom *at,
+   auto get_atom_colour_index = [udd_user_defined_atom_colour_index_handle, bond_colour_type] (mmdb:: Atom *at,
                                                                              const std::string &chain_id,
                                                                              coot::my_atom_colour_map_t &atom_colour_map) {
       int idx_col = atom_colour_map.index_for_chain(chain_id);
-      int idx_col_udd;
-      if (at->GetUDData(udd_user_defined_atom_colour_index_handle, idx_col_udd) == mmdb::UDDATA_Ok) {
-         idx_col = idx_col_udd;
+      // Only honour the user-defined per-atom colour in a user-defined colouring mode,
+      // otherwise it leaks into other modes (e.g. colour-by-chain). c.f. atom_colour().
+      if (bond_colour_type == coot::COLOUR_BY_USER_DEFINED_COLOURS) {
+         int idx_col_udd;
+         if (at->GetUDData(udd_user_defined_atom_colour_index_handle, idx_col_udd) == mmdb::UDDATA_Ok) {
+            idx_col = idx_col_udd;
+         }
       }
       return idx_col;
    };