Page MenuHome
Differential D14479

Cleanup/Refactor: remove cache parameters from `bvhtree_from_` functions
ClosedPublic
Actions

Authored by Germano Cavalcante (mano-wii) on Mar 28 2022, 10:39 PM.

Details

Reviewers
Brecht Van Lommel (brecht)
Commits
rB684b95804e8f: Refactor: remove cache parameters from `bvhtree_from_` functions
Summary

The BVHCacheType bvh_cache_type parameter should be read only as it defines very specific BVHTrees that cannot be customized.

So it doesn't make sense to pass this value to any *bvhtree_from_[...]_ex function as the BVHTrees created in these cases are custom and cannot be saved in the cache.

This also resulted in a nice cleanup in the code.

(Inviting @Brecht Van Lommel (brecht) as reviewer due the work on D11603).

Diff Detail

Repository
rB Blender

Event Timeline

Germano Cavalcante (mano-wii) requested review of this revision.Mar 28 2022, 10:39 PM
Germano Cavalcante (mano-wii) created this revision.
Harbormaster completed remote builds in B21290: Diff 49795.Mar 28 2022, 10:39 PM
Germano Cavalcante (mano-wii) updated this revision to Diff 49798.Mar 28 2022, 10:48 PM
  • Missed parameters on removal
Harbormaster completed remote builds in B21291: Diff 49798.Mar 28 2022, 10:48 PM
Brecht Van Lommel (brecht) requested changes to this revision.Mar 29 2022, 10:08 PM
Brecht Van Lommel (brecht) added inline comments.
source/blender/blenkernel/intern/bvhutils.cc
30

Don't use global variables, seems like this could just be a function parameter?

1267–1289

Is unlocking needed here? In the previous code I don't see it being conditional on is_cached && tree == nullptr.

1410

Is this re-inserting already cached BVHs?

1412

This should be set before unlocking.

This revision now requires changes to proceed.Mar 29 2022, 10:08 PM
Germano Cavalcante (mano-wii) updated this revision to Diff 49877.Mar 30 2022, 2:36 AM
Germano Cavalcante (mano-wii) marked 4 inline comments as done.
  • Remove thread_local bool g_isolate = false;
  • Merge all bvhtree_from_mesh_*.*_setup_data in a single utilitie
  • Fix lock and unlock uses
Harbormaster completed remote builds in B21338: Diff 49877.Mar 30 2022, 2:36 AM
Germano Cavalcante (mano-wii) added inline comments.Mar 30 2022, 2:42 AM
source/blender/blenkernel/intern/bvhutils.cc
30

The intent of the patch is to make the API more readable, and since the isolate parameter has a restricted and somewhat complicated usage, I thought it best to make it an internal local variable.

But I edited the code and made it a parameter.

1267–1289

Not necessary. If the tree is already in cache, the lock has not started.

1410

Oh yeah! Terrible mistake! Fixed!

1412

data->cache? I'm not sure if it is affected by the lock. Moved.

Brecht Van Lommel (brecht) requested changes to this revision.Apr 4 2022, 7:02 PM

Please don't add additional refactoring in this patch by unifying bvhtree_from_mesh_setup_data, reviewing the correctness of this change is too hard that way.

It's also not obviously an improvement to me. Parts of data are now getting initialized in various different places, it's difficult to understand.

This revision now requires changes to proceed.Apr 4 2022, 7:02 PM
Germano Cavalcante (mano-wii) added a comment.Apr 4 2022, 10:45 PM

In fact I got carried away and ended up making more changes than necessary (besides forgetting to set the callbacks to the editmesh data).
I separated the not directly related changes in another patch D14549: Refactor: Deduplicate and simplify BVH Utils code

Germano Cavalcante (mano-wii) updated this revision to Diff 50135.Apr 5 2022, 11:34 PM
  • Rebase onto master

Applied changes from D14549

Harbormaster completed remote builds in B21478: Diff 50135.Apr 5 2022, 11:34 PM
Brecht Van Lommel (brecht) accepted this revision.Apr 5 2022, 11:54 PM
This revision is now accepted and ready to land.Apr 5 2022, 11:54 PM
Closed by commit rB684b95804e8f: Refactor: remove cache parameters from `bvhtree_from_` functions (authored by Germano Cavalcante (mano-wii)). · Explain WhyApr 6 2022, 12:33 AM
This revision was automatically updated to reflect the committed changes.
Germano Cavalcante (mano-wii) added a commit: rB684b95804e8f: Refactor: remove cache parameters from `bvhtree_from_` functions.

Revision Contents
Changeset List

PathSize
source/
blender/
blenkernel/
28 lines
intern/
323 lines
8 lines
editors/
transform/
33 lines

Diff 50137

View Options

source/blender/blenkernel/BKE_bvhutils.h

Loading...
View Options

source/blender/blenkernel/intern/bvhutils.cc

Loading...
View Options

source/blender/blenkernel/intern/mesh_remap.c

Loading...
View Options

source/blender/editors/transform/transform_snap_object.cc

Loading...