Checkpoint an rt_comb_tess experiment

This sets up (more or less) what an rt_comb_tess callback for ft_tessellate
would look like, but also makes it clear why this hasn't been done previously -
due to the function signature of the RT_DECLARE_INTERFACE tess callback it
requires rt_db_internal to have awareness of at least dbip and (at the moment)
dp as well.  While this is not the first time I've wanted those, my
understanding is that conceptually the rt_db_internal entities are normally
"detached" from their source db_i and directory containers.  If they are
supposed to be prepared to persist beyond the survival or updating of their
sources, then the stashed pointers required by this approach may eventually, in
some usage scenario, become pointers to invalid data.

This is something of a dilemma - from an API cleanliness standpoint, an
rt_comb_tess function is the right place to put the "translate a comb object to
NMG" logic.  However, unless we clutter the rt_db_internal struct as we have
here, moving the logic also requires inputs not available in the callback
signature.  My current next step is to consider a change the
RTFUNCTAB_FUNC_TESS signature to pass in a dbip and dp as well.  The
import/export function signatures at least offer a precident for passing in a
dbip to one of the RT_DECLARE_INTERFACE functions, but none of them accepts a
struct directory.  Needs discussion before redoing all of the rt_*_tess
function signatures.

include/rt/db_internal.h

@@ -50,6 +50,31 @@ struct rt_db_internal {
     const struct rt_functab *idb_meth;  /**< @brief for ft_ifree(), etc. */
     void *              idb_ptr;
     struct bu_attribute_value_set idb_avs;
+
+    /* NOTE - src_dbip is usable as long as the rt_db_internal is used within a
+     * context that hasn't invalidated its source database instance.
+     * Technically that CAN happen without invalidating the rt_db_internal
+     * itself (although most normal use cases of rt_db_internal make it
+     * unlikely.) In the event the source db_i context IS destroyed AND the
+     * rt_db_internal continues to be used, operations requiring this pointer
+     * to be current (such as rt_comb_tess) will not work until the calling
+     * code updates src_dbip to a new parent.
+     *
+     * If there is another way to obtain dbip other than referencing this
+     * variable, that's what callers should do.  Use this ONLY when you're sure
+     * it is the only way to get the job done, and ALWAYS validate it before
+     * use with RT_CK_DBI. */
+    const struct db_i         *src_dbip;
+
+    /* The directory pointer has the same weaknesses as src_dbip, only more so
+     * - once the rt_db_internal is created, there is no longer any guarantee
+     * whatsoever that the dp itself remains valid or (even if it does) that
+     * the geometry represented by the dp still aligns with the values in the
+     * rt_db_internal.  Hence the variable name "src_dp".
+     *
+     * Users of this pointer should be VERY sure they know what they are doing
+     * and validate it with RT_CK_DIR. */
+    const struct directory    *src_dp;
 };
 #define idb_type                idb_minor_type
 #define RT_DB_INTERNAL_INIT(_p) { \
@@ -59,8 +84,10 @@ struct rt_db_internal {
 	(_p)->idb_meth = (const struct rt_functab *) ((void *)0); \
 	(_p)->idb_ptr = ((void *)0); \
 	bu_avs_init_empty(&(_p)->idb_avs); \
+	(_p)->src_dbip = NULL; \
+	(_p)->src_dp = NULL; \
     }
-#define RT_DB_INTERNAL_INIT_ZERO {RT_DB_INTERNAL_MAGIC, -1, -1, NULL, NULL, BU_AVS_INIT_ZERO}
+#define RT_DB_INTERNAL_INIT_ZERO {RT_DB_INTERNAL_MAGIC, -1, -1, NULL, NULL, BU_AVS_INIT_ZERO, NULL, NULL}
 #define RT_CK_DB_INTERNAL(_p) BU_CKMAG(_p, RT_DB_INTERNAL_MAGIC, "rt_db_internal")
 
 /**

src/librt/comb/comb.c

@@ -1136,6 +1136,135 @@ rt_comb_make(const struct rt_functab *UNUSED(ftp), struct rt_db_internal *intern
 }
 
 
+
+
+
+static union tree *
+facetize_region_end(struct db_tree_state *tsp,
+                    const struct db_full_path *pathp,
+                    union tree *curtree,
+                    void *client_data)
+{
+    union tree **facetize_tree;
+
+    if (tsp) RT_CK_DBTS(tsp);
+    if (pathp) RT_CK_FULL_PATH(pathp);
+
+    facetize_tree = (union tree **)client_data;
+
+    if (curtree->tr_op == OP_NOP) return curtree;
+
+    if (*facetize_tree) {
+        union tree *tr;
+        BU_ALLOC(tr, union tree);
+        RT_TREE_INIT(tr);
+        tr->tr_op = OP_UNION;
+        tr->tr_b.tb_regionp = REGION_NULL;
+        tr->tr_b.tb_left = *facetize_tree;
+        tr->tr_b.tb_right = curtree;
+        *facetize_tree = tr;
+    } else {
+        *facetize_tree = curtree;
+    }
+
+    /* Tree has been saved, and will be freed later */
+    return TREE_NULL;
+}
+
+
+
+/**
+ * Execute NMG tessellation routines on the tree to produce an explicitly
+ * bounded, evaluated version of the geometry.
+ *
+ * NOTE - we're faced with a dilemma here.  From an API standpoint, this is the
+ * logical place for the comb -> NMG tessellation logic.  However, putting it
+ * here requires the rt_db_internal to have knowledge of it's parent dbip and
+ * dp, and for those to be current.  That is not something historically
+ * required for rt_db_internal.
+ *
+ * Using another tree walker it might be possible to avoid the need for src_dp
+ * here, but a valid dbip is a hard requirement since tessellating a comb
+ * requires knowing about the geometry definitions of its leaves.
+ *
+ * Although to my knowledge there aren't any real world scenarios requiring a
+ * current rt_db_internal that persists after its dbip is invalid, there is no
+ * API guarantee that such a rt_db_internal couldn't be created.  The struct
+ * slots in rt_db_internal strongly caution against casual use of ip->src_*
+ * vars, and anyone reading this code should also take note - ONLY use src_dbip
+ * and src_dp when that's the only way and ALWAYS validate with RT_CK_*.
+ */
+int
+rt_comb_tess(struct nmgregion **r, struct model *m, struct rt_db_internal *ip, const struct bg_tess_tol *ttol, const struct bn_tol *tol)
+{
+    if (!r || !m  || !ip || !ttol || !tol)
+	return BRLCAD_ERROR;
+
+    RT_CK_DBI(ip->src_dbip);
+    RT_CK_DIR(ip->src_dp);
+
+    int i = 0;
+    int failed = 0;
+    union tree *facetize_tree = (union tree *)0;
+    struct db_tree_state init_state;
+    db_init_db_tree_state(&init_state, (struct db_i *)ip->src_dbip, &rt_uniresource);
+
+    /* Establish tolerances */
+    init_state.ts_ttol = ttol;
+    init_state.ts_tol = tol;
+    init_state.ts_m = &m;
+
+
+    const char *argv[2];
+    argv[0] = ip->src_dp->d_namep;
+    argv[1] = NULL;
+
+
+    if (!BU_SETJUMP) {
+        /* try */
+        i = db_walk_tree((struct db_i *)ip->src_dbip, 1, (const char **)argv,
+                         1,
+                        &init_state,
+                         0,                     /* take all regions */
+                         facetize_region_end,
+                         rt_booltree_leaf_tess,
+                         (void *)&facetize_tree
+                        );
+    } else {
+        /* catch */
+        BU_UNSETJUMP;
+        return BRLCAD_ERROR;
+    } BU_UNSETJUMP;
+
+    if (i < 0)
+        return BRLCAD_ERROR;
+
+    if (facetize_tree) {
+        if (!BU_SETJUMP) {
+            /* try */
+            failed = nmg_boolean(facetize_tree, m, &RTG.rtg_vlfree, tol, &rt_uniresource);
+        } else {
+            /* catch */
+            BU_UNSETJUMP;
+            return BRLCAD_ERROR;
+        } BU_UNSETJUMP;
+
+    } else {
+        failed = 1;
+    }
+
+    if (!failed && facetize_tree) {
+        NMG_CK_REGION(facetize_tree->tr_d.td_r);
+        facetize_tree->tr_d.td_r = (struct nmgregion *)NULL;
+    }
+
+    if (facetize_tree) {
+        db_free_tree(facetize_tree, &rt_uniresource);
+    }
+
+    return (failed) ? BRLCAD_ERROR : BRLCAD_OK;
+}
+
 /*
  * Local Variables:
  * mode: C

src/librt/dir.c

@@ -127,6 +127,13 @@ rt_db_get_internal(
 
     RT_DB_INTERNAL_INIT(ip);
 
+    // Store our source dp and dbip for reference.  These may become invalid
+    // without the rt_db_internal itself being invalid - there are no
+    // guarantees made by the dbio layer to update all rt_db_internal data
+    // containers, as the internal data is "detached" from its source.
+    ip->src_dbip = dbip;
+    ip->src_dp = dp;
+
     if (dbip->dbi_version > 4)
 	return rt_db_get_internal5(ip, dp, dbip, mat, resp);
 
@@ -187,6 +194,10 @@ rt_db_put_internal(
 
     RT_CK_DB_INTERNAL(ip);
 
+    /* Note - deliberately not checking ip's dbip pointer against the supplied
+     * dbip - we may be writing out to a different database than we read from,
+     * and that's fine. */
+
     if (db_version(dbip) > 4)
 	return rt_db_put_internal5(dp, dbip, ip, resp,
 				   ip->idb_major_type);

src/librt/primitives/table.cpp

@@ -143,7 +143,7 @@ extern void rt_binunif_make(const struct rt_functab *ftp, struct rt_db_internal
 extern int rt_binunif_get(struct bu_vls *logstr, const struct rt_db_internal *intern, const char *attr);
 extern int rt_binunif_adjust(struct bu_vls *logstr, struct rt_db_internal *intern, int argc, const char **argv);
 
-/* from tcl.c and db5_comb.c */
+/* from comb/comb.c and comb/db_comb.c */
 extern int rt_comb_export5(struct bu_external *ep, const struct rt_db_internal *ip, double local2mm, const struct db_i *dbip, struct resource *resp);
 extern int rt_comb_import5(struct rt_db_internal *ip, const struct bu_external *ep, const mat_t mat, const struct db_i *dbip, struct resource *resp);
 extern int rt_comb_get(struct bu_vls *logstr, const struct rt_db_internal *intern, const char *item);
@@ -151,6 +151,7 @@ extern int rt_comb_adjust(struct bu_vls *logstr, struct rt_db_internal *intern,
 extern int rt_comb_form(struct bu_vls *logstr, const struct rt_functab *ftp);
 extern void rt_comb_make(const struct rt_functab *ftp, struct rt_db_internal *intern);
 extern void rt_comb_ifree(struct rt_db_internal *ip);
+extern int rt_comb_tess(struct nmgregion **r, struct model *m, struct rt_db_internal *ip, const struct bg_tess_tol *ttol, const struct bn_tol *tol);
 
 extern int rt_annot_form(struct bu_vls *logstr, const struct rt_functab *ftp);
 extern int rt_bot_form(struct bu_vls *logstr, const struct rt_functab *ftp);
@@ -1681,7 +1682,7 @@ const struct rt_functab OBJ[] = {
 	NULL, /* plot */
 	NULL, /* adaptive_plot */
 	NULL, /* vshot */
-	NULL, /* tess - TODO - is there any reason we can't do the treewalk evaluation as a rt_comb_tess? */
+	RTFUNCTAB_FUNC_TESS_CAST(rt_comb_tess),
 	NULL, /* tnurb */
 	NULL, /* brep */
 	RTFUNCTAB_FUNC_IMPORT5_CAST(rt_comb_import5),