Details

Reviewers

Sergey Sharybin (sergey)
Hans Goudey (HooglyBoogly)
Jacques Lucke (JacquesLucke)

Maniphest Tasks

T94078: Wrong Bound Box calculated for curves

Commits

rB5560f3244717: Fix T94078: Wrong Bound Box calculated for curves

Summary

Boundbox on curves is somewhat unstable.

If some code outside the depsgraph evaluation context marks an object
as having the BoundDox dirty, the boundbox will go wrong until the
modifiers are evaluated again.

And that's what happens in the drawing loop (DEG_OBJECT_ITER_FOR_RENDER_ENGINE_BEGIN).

The same curve type object appears twice in DEG_OBJECT_ITER_FOR_RENDER_ENGINE_BEGIN:

the first time the object->data is the curve data
the second time the object->data is temporarily replaced by a mesh that has been evaluated

During this temporary replacement, the boundbox is marked as being dirty.

In the example shown in T94078, we see that the boundbox read by the
overlay engine gets wrong after the viewport is updated.

NOTE: boundbox is always required by some engines in order to detect occlusion. Marking a boundbox as dirty during the drawing loop can be bad for performance.

Solution

The solution proposed in this patch is to change the boundbox reference of the temporary objects,
so the boundbox of the non-temporary object (with the data curve) is not marked dirty.

An ideal solution might be to add a "boundbox" member to the Mesh struct.
Or replace by default the ob->data of the evaluated object by the final
mesh with modifiers.

NOTE: The same object can be represented in different ways. An object of mesh type for example has 4 different meshes (mesh, mesh->edit_mesh->bm, mesh->edit_mesh->mesh_eval_final, mesh->edit_mesh->mesh_eval_cage). This can complicate a bit when choosing the boundbox for the object.

Ref T94078

Diff Detail

Repository

rB Blender

Branch

arcpatch-D13581 (branched from master)

Build Status

Buildable 19753
Build 19753: arc lint + arc unit

Event Timeline

Germano Cavalcante (mano-wii) requested review of this revision.Dec 14 2021, 10:46 PM

Germano Cavalcante (mano-wii) created this revision.

Harbormaster completed remote builds in B19438: Diff 46056.Dec 14 2021, 10:46 PM

I'm not sure this is correct. It seems like after this, instances will have incorrect bounding boxes, especially geometry instances.

I think we should just go ahead and add the storage of min and max on mesh, curves, and point clouds. It's helpful to have that information cached when there are no objects anyway.
We've been talking about that change for a while for improving instancing performance in the viewport (it's listed on T92963).

avoid updating the boundbox of curve type objects only

I would be glad to see a more conventional and intuitive solution. But I feel that this would require much deeper changes.
I could work on something like this (add min, max members to the Curves and Meshes runtime), this would avoid setting bb dirty all the time and would solve the peformance and wrong geometry problem as well.
But does this need a well-discussed design with the module first?
Is it worth making such a complex patch to fix the bug?

For now, I've updated the proposed solution initially making it more delimited.

Harbormaster completed remote builds in B19482: Diff 46120.Dec 16 2021, 3:45 PM

I think geometry instances containing curves will still have incorrect bounding boxes with this method, since there was no "instanced object" on which we can hope the bounding box was already calculated.

I made an initial patch exploring the changes I mentioned here: D13598
I'll wait for design feedback before continuing though.

I am not convinced at this time that going such workaround approach is the proper solution. There should be a reliable solution which the overall design (with possible tweaks to the design).

This revision now requires changes to proceed.Jan 4 2022, 12:37 PM

New solution - Change the bound box reference of temporary objects

I changed the initial approach and now the "temporary" objects with different data have the boundbox reference pointed elsewhere.

Thus the boundbox of the non-temporary object is not marked as dirty and remains intact in the loop.

The disadvantage of this other solution is that the boundbox of temporary objects with different data is always recalculated.

But it's better than the current code that recalculates the boundbox of temporary and non-temporary objects.

Harbormaster completed remote builds in B19751: Diff 46598.Jan 4 2022, 3:00 PM

Germano Cavalcante (mano-wii) edited the summary of this revision. (Show Details)Jan 4 2022, 3:03 PM

Move 'bb_tmp' member from 'DupliObject' to 'DEGObjectIterData'

DupliObject is allocated for each duplicated object but bb_tmp's storage doesn't need to be in as many places.

Harbormaster completed remote builds in B19753: Diff 46600.Jan 4 2022, 3:40 PM

That's a good find. Is not good BKE_object_replace_data_on_shallow_copymodifies original object.

From the usage and name of BKE_object_replace_data_on_shallow_copy it seems that the BKE_object_replace_data_on_shallow_copy should set ob->runtime.bb to nullptr, and depsgraph iter will assign a bounding box to the temp_dupli_object after BKE_object_replace_data_on_shallow_copy. Seems to be more clear/safe way as it will take care of possible unintended modification of the original data. At that point I wouldnt' call it a workaround in the DEG code, it will become a way of proper decoupling runtime fields for a temporary storage.

Avoid workarrounds by not using common storage for bounding box

The solution now is to set the boundbox of the duplicate (temporary) objects to nullptr.
These boundsboxes are forcibly allocated inside the engine so they need to be freed too.

(It seemed strange in my opinion to set ob->runtime.bb to nullptr inside the BKE_object_replace_data_on_shallow_copy as it doesn't seem very clear there that the memory isn't owned).

Harbormaster completed remote builds in B19808: Diff 46695.Jan 6 2022, 5:16 PM

To me it makes sense to disallow depsgraph from modifying object's state when iterating. Although, second pair of eyes is super welcome here!

I think the following part can be removed from BKE_object_replace_data_on_shallow_copy now. Or maybe it should be replaced with BLI_assert(ob->runtime.bb == nullptr).

if (ob->runtime.bb != nullptr) {
  ob->runtime.bb->flag |= BOUNDBOX_DIRTY;
}

Besides that, looks good to me.

source/blender/depsgraph/intern/depsgraph_query_iter.cc
100	`verify` is an ambiguous term, it's unclear what it just checks if the data is freed or if it actually does free the data. Better use `ensure_boundbox_freed`.

This revision is now accepted and ready to land.Jan 7 2022, 12:19 PM

Closed by commit rB5560f3244717: Fix T94078: Wrong Bound Box calculated for curves (authored by Germano Cavalcante (mano-wii)). · Explain WhyJan 7 2022, 2:57 PM

This revision was automatically updated to reflect the committed changes.

Germano Cavalcante (mano-wii) added a commit: rB5560f3244717: Fix T94078: Wrong Bound Box calculated for curves.

Fix T94078: Wrong Bound Box calculated for curves
ClosedPublic
Actions

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 46600

source/blender/depsgraph/DEG_depsgraph_query.h

source/blender/depsgraph/intern/depsgraph_query_iter.cc

source/blender/draw/intern/draw_manager.c

Fix T94078: Wrong Bound Box calculated for curvesClosedPublicActions

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 46600

source/blender/depsgraph/DEG_depsgraph_query.h

source/blender/depsgraph/intern/depsgraph_query_iter.cc

source/blender/draw/intern/draw_manager.c

Fix T94078: Wrong Bound Box calculated for curves
ClosedPublic
Actions

Revision Contents
Changeset List