Fix #117338: Texture paint sampling broken with modifiers #120259

Hans Goudey · 2024-04-04T17:15:38+02:00

Hans Goudey commented

2024-04-04 17:15:38 +02:00

e64b3c8212 worked to complete a transition started during 2.8
development to avoid dynamically re-evaluating objects outside of the
dependency graph's control. However, that behavior was used to make
sure the original index mapping was available for the texture paint
sample operator.

Conceptually, sampling a texture on an arbitrary evaluated mesh should
not require original indices at all. All we need to know is the UV map
value under the mouse. This commit changes from using GPU index textures
to a BVH tree raycast. This significantly simplifies the code and makes
it work with GPU subdivision too. Though it may be slower to build the
BVH tree, that should be okay because it's cached and only needs to be
built once, and that's something we're want to optimize anyway.

Removing the reliance on original indices also means we could paint
textures on completely procedurally generated meshes, which may be
an interesting feature in the future.

e64b3c821252fc8707b4 worked to complete a transition started during 2.8 development to avoid dynamically re-evaluating objects outside of the dependency graph's control. However, that behavior was used to make sure the original index mapping was available for the texture paint sample operator. Conceptually, sampling a texture on an arbitrary evaluated mesh should not require original indices at all. All we need to know is the UV map value under the mouse. This commit changes from using GPU index textures to a BVH tree raycast. This significantly simplifies the code and makes it work with GPU subdivision too. Though it may be slower to build the BVH tree, that should be okay because it's cached and only needs to be built once, and that's something we're want to optimize anyway. Removing the reliance on original indices also means we could paint textures on completely procedurally generated meshes, which may be an interesting feature in the future.

Hans Goudey added 1 commit 2024-04-04 17:15:46 +02:00

buildbot/vexp-code-patch-lint Build done. Details

buildbot/vexp-code-patch-linux-x86_64 Build done. Details

buildbot/vexp-code-patch-darwin-x86_64 Build done. Details

buildbot/vexp-code-patch-windows-amd64 Build done. Details

buildbot/vexp-code-patch-darwin-arm64 Build done. Details

buildbot/vexp-code-patch-coordinator Build done. Details

6bde217966 Fix #117338 : Texture paint sampling broken with modifiers

e64b3c8212 worked to complete a transition started during 2.8
development to avoid dynamically re-evaluating objects outside of the
dependency graph's control. However, that behavior was used to make
sure the original index mapping was available for the texture paint
sample operator.

Conceptually, sampling a texture on an arbitrary evaluated mesh should
not require original indices at all though. All we need to know is the
UV map value under the mouse. This commit changes from using GPU index
textures to a BVH tree raycast. This significantly simplifies the code
and makes it work with GPU subdivision too. Though it may be slower to
build the BVH tree, that should be okay because it's cached and only
needs to be built once, and that's something we're looking to optimize
in the future anyway.

Removing the reliance on original indices also means we could paint
textures on completely procedurally generated meshes, which may be
an interesting feature in the future.

Hans Goudey commented

2024-04-04 17:16:29 +02:00

@blender-bot build

Hans Goudey requested review from Sergey Sharybin 2024-04-04 17:16:37 +02:00

Hans Goudey commented

2024-04-04 18:38:56 +02:00

Some performance numbers with a 6 million face mesh (AFAIK that's a very high resolution for texture painting workflows):

For the first sampling operation, creating the BVH tree took 1.2s.
The whole sample function only takes a few ms more than that
Every subsequent sample operation takes less than a millisecond

For a 400k face mesh, creating the BVH tree took 57ms.

The BVH tree build time is a bit longer than I'd like. However, this method doesn't require reevaluating the object to add the original index layer. Considering that could be arbitrarily slow, overall I like this tradeoff. Especially given the possibility of texture painting on procedural objects without original indices in the future. Switching to a different BVH tree implementation (as investigated in #108148) would significantly improve the BVH tree build performance too.

I also realized a change like #119977 might be used to solve this too. But it doesn't remove the complexity of finding the UV within the sampled face. So I still think the approach in this PR is preferable.

Some performance numbers with a 6 million face mesh (AFAIK that's a very high resolution for texture painting workflows): - For the first sampling operation, creating the BVH tree took 1.2s. - The whole sample function only takes a few ms more than that - Every subsequent sample operation takes less than a millisecond For a 400k face mesh, creating the BVH tree took 57ms. The BVH tree build time is a bit longer than I'd like. However, this method doesn't require reevaluating the object to add the original index layer. Considering that could be arbitrarily slow, overall I like this tradeoff. Especially given the possibility of texture painting on procedural objects without original indices in the future. Switching to a different BVH tree implementation (as investigated in #108148) would significantly improve the BVH tree build performance too. --- I also realized a change like #119977 might be used to solve this too. But it doesn't remove the complexity of finding the UV within the sampled face. So I still think the approach in this PR is preferable.

Hans Goudey added 1 commit 2024-04-04 18:51:42 +02:00

62cb8ac91b Merge branch 'main' into fix-texture-paint-sample

Sergey Sharybin approved these changes 2024-04-05 13:01:15 +02:00

Sergey Sharybin left a comment

It seems to work quite well, and did not see anything obvious in the functional part of the code. There is some weird comment, which i think could just be removed now.

I could not test GPU subdivision case, as I do not think it is supported on macOS.

As for the approach, I think it is clearer if we stick to a CPU only solution, rather than mixing the old approach with some computationally intense parts done on CPU and some other stuff done on GPU.

Marking as a green, so that it can go in as soon as the inlined comment is looked into and possibly addressed.

It seems to work quite well, and did not see anything obvious in the functional part of the code. There is some weird comment, which i think could just be removed now. I could not test GPU subdivision case, as I do not think it is supported on macOS. As for the approach, I think it is clearer if we stick to a CPU only solution, rather than mixing the old approach with some computationally intense parts done on CPU and some other stuff done on GPU. Marking as a green, so that it can go in as soon as the inlined comment is looked into and possibly addressed.

source/blender/editors/sculpt_paint/paint_utils.cc

						
				@ -293,4 +216,4 @@

				  const int *material_indices = (const int *)CustomData_get_layer_named(

				      &mesh_eval->face_data, CD_PROP_INT32, "material_index");

				  /* test all faces in the derivedmesh with the original index of the picked face */