It seems like the proper way to differentiate between these functions would be calling them BKE_object_boundbox_original and BKE_object_boundbox_evaluated

Neither of them are correct though, I don't think, for a few reasons:

• An evaluated object can evaluated to more than one type of data, with the result stored in geometry_set_eval
• The displist result for curve and text objects isn't so relevant anymore:
  • The evaluated mesh is the proper place to look for any curve that has a surface when evalauted
  • When the curve does not have a surface (i.e. no curve bevel), the proper place to look is in the CurveEval output, since that takes into account data from geometry nodes.
• Then there are instances, which an evaluated object might also have-- if those aren't considered it should at least be in a comment.
• Should the "evaluated" version really look at the original data as a backup? That seems wrong, since what if the data type or size of the evaluated result has changed.
• BKE_object_boundbox_get now doesn't consider other types like point clouds, volumes, or grease pencil.

Besides that, I think the best way to avoid recalculating bound boxes for instances is moving their cache to object data rather than objects. That's outside the scope of this patch, just wanted to mention it.

• The displist result for curve and text objects isn't so relevant anymore

I see the point, but I don't see why we should prevent displist from still being used as a second alternative (as a security measure).
The evaluated mesh is still the priority.

• BKE_object_boundbox_get now doesn't consider other types like point clouds, volumes, or grease pencil.

How so? Do these objects generate an evaluated dummy mesh that prevents the BKE_object_data_boundbox_get from being called at the end?
(I need to investigate...)

• BKE_object_boundbox_get now doesn't consider other types like point clouds, volumes, or grease pencil.

Ah, I get the point now!
It refers to the nomenclature. In fact the BKE_object_boundbox_original still reads the evaluated date for those objects types.
I'll see how I can make it more conventional...

I agree with what Bastien said, but the evaluated curve is CurveEval, rather than the displist. Since that is implemented in C++, curve.c could be moved to C++ too, or a wrapper BKE_curve_eval_bounds_min_max could be added.

In D12282#341345, @Germano Cavalcante (mano-wii) wrote:

I'm not sure how to use CurveEval. This appears to store spline data (without tris?)

Exactly, and that's the nice simplification-- curve data is only the splines, and if it has a surface (triangles, etc.) than the data is a mesh.
But keep in mind that any object can evaluate to both a mesh and a curve, or even a point cloud and a volume too. All of that data should be in geometry_set_eval.

Does this mean that displists should now only be used for metaballs?

Yes (and it would even be nice to change that!)

In a more ideal world I would imaging the bounding box to be calculated as part of dependency graph update and not being lazily calculated. It is possible to copy evaluated bounding box to the original object for ease of access (if needed) in the similar way how we do it for animation system.

From this point of view I'd expect BKE_object_boundbox_get to be a simple return statement, and things like BKE_displist_minmax and BKE_mesh_wrapper_minmax will happen in a more localized places after the mesh/curve modifier stack is evaluated.

Generally the direction should be in building more of a black-boxed proper API, so that, for example, adding new object type does not require of finding all places where modifications are to be made.

One thing that is unclear to me with this patch is, is it applicable to 3.0 and 2.93 (and ideally 2.83)? T90808 is a regression that also needs to be fixed in the LTS releases (as much as possible), and mandatory fix for 3.0 release.

In D12282#348984, @Bastien Montagne (mont29) wrote:

One thing that is unclear to me with this patch is, is it applicable to 3.0 and 2.93 (and ideally 2.83)? T90808 is a regression that also needs to be fixed in the LTS releases (as much as possible), and mandatory fix for 3.0 release.

Although the existing code is not ideal. The problem was only evident after rBb9febb54a492: Geometry Nodes: Support modifier on curve objects (committed in 3.0).
So even though it might be somewhat reproducible in 2.93, it's not a major issue as it has apparently never been reported for that version.

Since the code needs to work with C++ in object.c, it's a little tricky to replicate the same solution in the 2.93.

My remaining question is-- why does BKE_object_boundbox_calc_from_mesh still exist? I would think that object_sync_boundbox_to_original and BKE_object_boundbox_calc_from_evaluated_geometry would handle all object types now.

Other than that, the code looks good.

Okay, the changes here look good now. But my questions about BKE_object_boundbox_calc_from_mesh still remains-- that still seems incorrect to me, at least relative to this change.

Oops, I missed the question.
The question is a bit complex.
BKE_object_boundbox_calc_from_mesh cannot be simply replaced since its usage is quite specific (computing a BoundBox for the object based on the mesh that is passed as a parameter).
This function is called at the end of the modifier evaluation function and works for any type of object.
In fact, its existence somewhat complicates the answer to the question "what does the BoundBox encompass?".

I'm not sure if it can be removed as its call tree has branches outside the depsgraph evaluation which may end up leaving the BoundBox uncalculated (and the BoundBox based on ob->data can be used instead of the evaluated geometry).
Removing/replacing it, although it would make the code easier to understand (imho), seems to be an issue for another patch.