Details

Reviewers

Jeroen Bakker (jbakker)
Julien Kaspar (JulienKaspar)

Maniphest Tasks

T99203: Support Automasking For Texture Paint

Summary

This patch adds support for automasking to texture paint.

Notes:

Automasking API can now build the factor cache incrementally per PBVH node.

Diff Detail

Repository

rB Blender

Branch

temp-texpaint-automasking

Build Status

Buildable 22703
Build 22703: arc lint + arc unit

Event Timeline

Joseph Eagar (joeedh) requested review of this revision.Jun 27 2022, 7:15 AM

Joseph Eagar (joeedh) created this revision.

Harbormaster completed remote builds in B22703: Diff 52922.Jun 27 2022, 7:15 AM

Julien Kaspar (JulienKaspar) added a task: T99203: Support Automasking For Texture Paint.Jun 27 2022, 11:42 AM

Some small code style issues. I haven't checked the functionality. @Julien Kaspar (JulienKaspar) will you test-drive this patch?

The description of the patch isn't clear. See https://wiki.blender.org/wiki/Process/Contributing_Code#Ingredients_of_a_Patch

source/blender/blenkernel/BKE_pbvh.h
417	better to put them on separate lines for clarety. According to code style `unique_vert_len` would be more appropriate. `unique_verts` doesn't contain the verts.
596	Just a reminder that this should be removed before committing.
source/blender/blenkernel/intern/pbvh_intern.h
99	better remove the storage length.

Sync with master and make requested changes.

Harbormaster completed remote builds in B24227: Diff 56723.Oct 11 2022, 8:34 PM

I tested the patch and all auto-masking features work well. On a functionality level this is all good.

Code wise seems ok, just needs to be cleaned up.

Remove debug code
Remove commented out code.

source/blender/editors/sculpt_paint/sculpt_paint_image.cc
147	Remove commented out code.

Joseph Eagar (joeedh) updated this revision to Diff 56824.Oct 15 2022, 5:49 AM

Joseph Eagar (joeedh) marked 3 inline comments as done.

Harbormaster completed remote builds in B24266: Diff 56824.Oct 15 2022, 5:49 AM

Jeroen Bakker (jbakker) accepted this revision.Oct 18 2022, 9:12 AM

This revision is now accepted and ready to land.Oct 18 2022, 9:12 AM

FAILED: source/blender/editors/sculpt_paint/CMakeFiles/bf_editor_sculpt_paint.dir/sculpt_automasking.cc.o 
/Library/Developer/CommandLineTools/usr/bin/c++ -DDEBUG -DMACOSX_DEPLOYMENT_TARGET=11.00 -DWITH_ASSERT_ABORT -DWITH_INPUT_NDOF -DWITH_OPENGL -DWITH_SSE2NEON -DWITH_TBB -D_DEBUG -D__LITTLE_ENDIAN__ -I/Users/jeroen/blender-git/blender/source/blender/editors/include -I/Users/jeroen/blender-git/blender/source/blender/editors/uvedit -I/Users/jeroen/blender-git/blender/source/blender/blenkernel -I/Users/jeroen/blender-git/blender/source/blender/blenlib -I/Users/jeroen/blender-git/blender/source/blender/blentranslation -I/Users/jeroen/blender-git/blender/source/blender/bmesh -I/Users/jeroen/blender-git/blender/source/blender/depsgraph -I/Users/jeroen/blender-git/blender/source/blender/draw -I/Users/jeroen/blender-git/blender/source/blender/functions -I/Users/jeroen/blender-git/blender/source/blender/geometry -I/Users/jeroen/blender-git/blender/source/blender/gpu -I/Users/jeroen/blender-git/blender/source/blender/imbuf -I/Users/jeroen/blender-git/blender/source/blender/makesdna -I/Users/jeroen/blender-git/blender/source/blender/makesrna -I/Users/jeroen/blender-git/blender/source/blender/nodes -I/Users/jeroen/blender-git/blender/source/blender/render -I/Users/jeroen/blender-git/blender/source/blender/windowmanager -I/Users/jeroen/blender-git/blender/intern/atomic -I/Users/jeroen/blender-git/blender/intern/clog -I/Users/jeroen/blender-git/blender/intern/eigen -I/Users/jeroen/blender-git/blender/intern/guardedalloc -I/Users/jeroen/blender-git/build_darwin/source/blender/makesrna -isystem /Users/jeroen/blender-git/lib/darwin_arm64/sse2neon -isystem /Users/jeroen/blender-git/lib/darwin_arm64/tbb/include -Wall -Wc++20-designator -Wno-tautological-compare -Wno-unknown-pragmas -Wno-char-subscripts -Wno-overloaded-virtual -Wno-sign-compare -Wno-invalid-offsetof -Wno-suggest-override  -mmacosx-version-min=11.00 -ftemplate-depth=1024 -stdlib=libc++ -Xclang -fopenmp -I'/Users/jeroen/blender-git/blender/../lib/darwin_arm64/openmp/include'  -pipe -funsigned-char -fno-strict-aliasing -fmacro-prefix-map="/Users/jeroen/blender-git/blender/"="" -fmacro-prefix-map="/Users/jeroen/blender-git/build_darwin/"="" -g -arch arm64 -isysroot /Library/Developer/CommandLineTools/SDKs/MacOSX12.3.sdk -mmacosx-version-min=11.00 -std=c++17 -MD -MT source/blender/editors/sculpt_paint/CMakeFiles/bf_editor_sculpt_paint.dir/sculpt_automasking.cc.o -MF source/blender/editors/sculpt_paint/CMakeFiles/bf_editor_sculpt_paint.dir/sculpt_automasking.cc.o.d -o source/blender/editors/sculpt_paint/CMakeFiles/bf_editor_sculpt_paint.dir/sculpt_automasking.cc.o -c /Users/jeroen/blender-git/blender/source/blender/editors/sculpt_paint/sculpt_automasking.cc
In file included from /Users/jeroen/blender-git/blender/source/blender/editors/sculpt_paint/sculpt_automasking.cc:16:
In file included from /Users/jeroen/blender-git/blender/source/blender/blenlib/BLI_set.hh:61:
/Users/jeroen/blender-git/blender/source/blender/blenlib/BLI_hash.hh:93:19: error: member reference base type 'const long' is not a structure or union
      return value.hash();
             ~~~~~^~~~~
/Users/jeroen/blender-git/blender/source/blender/blenlib/BLI_set.hh:302:38: note: in instantiation of member function 'blender::DefaultHash<long>::operator()' requested here
    return this->contains__impl(key, hash_(key));
                                     ^
/Users/jeroen/blender-git/blender/source/blender/blenlib/BLI_set.hh:298:18: note: in instantiation of function template specialization 'blender::Set<long, 4, blender::PythonProbingStrategy<1, false>, blender::DefaultHash<long>, blender::DefaultEquality, blender::SimpleSetSlot<long>>::contains_as<long>' requested here
    return this->contains_as(key);
                 ^
/Users/jeroen/blender-git/blender/source/blender/editors/sculpt_paint/sculpt_automasking.cc:1035:21: note: in instantiation of member function 'blender::Set<long, 4, blender::PythonProbingStrategy<1, false>, blender::DefaultHash<long>, blender::DefaultEquality, blender::SimpleSetSlot<long>>::contains' requested here
      if (!done_set.contains(vertex.i)) {
                    ^
In file included from /Users/jeroen/blender-git/blender/source/blender/editors/sculpt_paint/sculpt_automasking.cc:16:
In file included from /Users/jeroen/blender-git/blender/source/blender/blenlib/BLI_set.hh:61:
/Users/jeroen/blender-git/blender/source/blender/blenlib/BLI_hash.hh:104:12: error: type 'long' cannot be used prior to '::' because it has no members
    return T::hash_as(value);
           ^
/Users/jeroen/blender-git/blender/source/blender/blenlib/BLI_set.hh:263:59: note: in instantiation of function template specialization 'blender::DefaultHash<long>::operator()<long>' requested here
    return this->add__impl(std::forward<ForwardKey>(key), hash_(key));
                                                          ^
/Users/jeroen/blender-git/blender/source/blender/blenlib/BLI_set.hh:255:18: note: in instantiation of function template specialization 'blender::Set<long, 4, blender::PythonProbingStrategy<1, false>, blender::DefaultHash<long>, blender::DefaultEquality, blender::SimpleSetSlot<long>>::add_as<const long &>' requested here
    return this->add_as(key);
                 ^
/Users/jeroen/blender-git/blender/source/blender/editors/sculpt_paint/sculpt_automasking.cc:1036:18: note: in instantiation of member function 'blender::Set<long, 4, blender::PythonProbingStrategy<1, false>, blender::DefaultHash<long>, blender::DefaultEquality, blender::SimpleSetSlot<long>>::add' requested here
        done_set.add(vertex.i);
                 ^
In file included from /Users/jeroen/blender-git/blender/source/blender/editors/sculpt_paint/sculpt_automasking.cc:16:
In file included from /Users/jeroen/blender-git/blender/source/blender/blenlib/BLI_set.hh:64:
/Users/jeroen/blender-git/blender/source/blender/blenlib/BLI_set_slots.hh:124:12: error: no matching function for call to object of type 'const blender::DefaultHash<long>'
    return hash(*key_buffer_);
           ^~~~
/Users/jeroen/blender-git/blender/source/blender/blenlib/BLI_set.hh:692:36: note: in instantiation of function template specialization 'blender::SimpleSetSlot<long>::get_hash<blender::DefaultHash<long>>' requested here
    const uint64_t hash = old_slot.get_hash(Hash());
                                   ^
/Users/jeroen/blender-git/blender/source/blender/blenlib/BLI_set.hh:673:17: note: in instantiation of member function 'blender::Set<long, 4, blender::PythonProbingStrategy<1, false>, blender::DefaultHash<long>, blender::DefaultEquality, blender::SimpleSetSlot<long>>::add_after_grow' requested here
          this->add_after_grow(slot, new_slots, new_slot_mask);
                ^
/Users/jeroen/blender-git/blender/source/blender/blenlib/BLI_set.hh:857:13: note: in instantiation of member function 'blender::Set<long, 4, blender::PythonProbingStrategy<1, false>, blender::DefaultHash<long>, blender::DefaultEquality, blender::SimpleSetSlot<long>>::realloc_and_reinsert' requested here
      this->realloc_and_reinsert(this->size() + 1);
            ^
/Users/jeroen/blender-git/blender/source/blender/blenlib/BLI_set.hh:774:11: note: in instantiation of member function 'blender::Set<long, 4, blender::PythonProbingStrategy<1, false>, blender::DefaultHash<long>, blender::DefaultEquality, blender::SimpleSetSlot<long>>::ensure_can_add' requested here
    this->ensure_can_add();
          ^
/Users/jeroen/blender-git/blender/source/blender/blenlib/BLI_set.hh:263:18: note: in instantiation of function template specialization 'blender::Set<long, 4, blender::PythonProbingStrategy<1, false>, blender::DefaultHash<long>, blender::DefaultEquality, blender::SimpleSetSlot<long>>::add__impl<const long &>' requested here
    return this->add__impl(std::forward<ForwardKey>(key), hash_(key));
                 ^
/Users/jeroen/blender-git/blender/source/blender/blenlib/BLI_set.hh:255:18: note: in instantiation of function template specialization 'blender::Set<long, 4, blender::PythonProbingStrategy<1, false>, blender::DefaultHash<long>, blender::DefaultEquality, blender::SimpleSetSlot<long>>::add_as<const long &>' requested here
    return this->add_as(key);
                 ^
/Users/jeroen/blender-git/blender/source/blender/editors/sculpt_paint/sculpt_automasking.cc:1036:18: note: in instantiation of member function 'blender::Set<long, 4, blender::PythonProbingStrategy<1, false>, blender::DefaultHash<long>, blender::DefaultEquality, blender::SimpleSetSlot<long>>::add' requested here
        done_set.add(vertex.i);
                 ^
/Users/jeroen/blender-git/blender/source/blender/blenlib/BLI_hash.hh:97:33: note: candidate template ignored: substitution failure [with U = long]
  template<typename U> uint64_t operator()(const U &value) const
                                ^
3 errors generated.
[785/1727] Building CXX object intern/cycles/kernel/CMakeFiles/cycles_kernel.dir/device/cpu/kernel.cpp.o
ninja: build stopped: subcommand failed.
  417.85s user 35.60s system 1457% cpu 31.105 total

Some build errors on macOS

intptr_t on macOs = long.

This revision now requires changes to proceed.Oct 18 2022, 9:14 AM

Merge from master.

Harbormaster completed remote builds in B25257: Diff 59153.Jan 6 2023, 8:00 AM

Still same issue on macOs.
I will schedule some time next week to get this sorted out and provide a patch.

This revision now requires changes to proceed.Fri, Jan 13, 7:58 AM

Perhaps for consistency we should replace intptr_t with int64_t in the PBVHVertRef/PBVHEdgeRef/PBVHFaceRef structs as well. That can be done in master outside this patch.

diff --git a/source/blender/blenkernel/intern/pbvh.c b/source/blender/blenkernel/intern/pbvh.c
index 03463ca9a5c..d5449ab3e68 100644
--- a/source/blender/blenkernel/intern/pbvh.c
+++ b/source/blender/blenkernel/intern/pbvh.c
@@ -3592,17 +3592,17 @@ void BKE_pbvh_ensure_node_loops(PBVH *pbvh)
   MEM_SAFE_FREE(visit);
 }
 
-void BKE_pbvh_node_automasking_mark(PBVH *pbvh, PBVHNode *node)
+void BKE_pbvh_node_automasking_mark(PBVH *UNUSED(pbvh), PBVHNode *node)
 {
   node->flag |= PBVH_RebuildAutomasking;
 }
 
-void BKE_pbvh_node_automasking_unmark(PBVH *pbvh, PBVHNode *node)
+void BKE_pbvh_node_automasking_unmark(PBVH *UNUSED(pbvh), PBVHNode *node)
 {
   node->flag &= ~PBVH_RebuildAutomasking;
 }
 
-bool BKE_pbvh_node_needs_automasking(PBVH *pbvh, PBVHNode *node)
+bool BKE_pbvh_node_needs_automasking(PBVH *UNUSED(pbvh), PBVHNode *node)
 {
   return node->flag & PBVH_RebuildAutomasking;
 }
diff --git a/source/blender/editors/sculpt_paint/sculpt_automasking.cc b/source/blender/editors/sculpt_paint/sculpt_automasking.cc
index 79c5a8b5f27..9d2f141f0ff 100644
--- a/source/blender/editors/sculpt_paint/sculpt_automasking.cc
+++ b/source/blender/editors/sculpt_paint/sculpt_automasking.cc
@@ -1040,7 +1040,7 @@ void SCULPT_automasking_cache_check(
     }
   });
 
-  Set<intptr_t> done_set;
+  Set<int64_t> done_set;
 
   for (int i : IndexRange(totnode)) {
     PBVHNode *node = nodes[i];

Sync with master and fix apple compile error.

Harbormaster completed remote builds in B25524: Diff 59814.Mon, Jan 23, 5:42 PM

Just a small tweak is still needed to remove the compilation warnings. We try to keep them to 0 as much as possible.

source/blender/blenkernel/intern/pbvh.c

3326

pbvh isn't used and should by surrounded by the UNUSED macro. Same needs to be applied to the functions below

[1502/2254] Building C object source/blender/blenkernel/CMakeFiles/bf_blenkernel.dir/intern/pbvh.c.o
/Users/jeroen/blender-git/blender/source/blender/blenkernel/intern/pbvh.c:3583:43: warning: unused parameter 'pbvh' [-Wunused-parameter]
void BKE_pbvh_node_automasking_mark(PBVH *pbvh, PBVHNode *node)
                                          ^
/Users/jeroen/blender-git/blender/source/blender/blenkernel/intern/pbvh.c:3588:45: warning: unused parameter 'pbvh' [-Wunused-parameter]
void BKE_pbvh_node_automasking_unmark(PBVH *pbvh, PBVHNode *node)
                                            ^
/Users/jeroen/blender-git/blender/source/blender/blenkernel/intern/pbvh.c:3593:44: warning: unused parameter 'pbvh' [-Wunused-parameter]
bool BKE_pbvh_node_needs_automasking(PBVH *pbvh, PBVHNode *node)
                                           ^
3 warnings generated.

This revision now requires changes to proceed.Tue, Jan 24, 3:48 PM

Support Automasking For Texture Paint
Needs RevisionPublic
Actions

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 52922

source/blender/blenkernel/BKE_pbvh.h

source/blender/blenkernel/intern/pbvh.c

source/blender/blenkernel/intern/pbvh_intern.h

source/blender/editors/sculpt_paint/sculpt.c

source/blender/editors/sculpt_paint/sculpt_automasking.cc

source/blender/editors/sculpt_paint/sculpt_intern.h

source/blender/editors/sculpt_paint/sculpt_paint_image.cc

Support Automasking For Texture Paint Needs RevisionPublicActions

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 52922

source/blender/blenkernel/BKE_pbvh.h

source/blender/blenkernel/intern/pbvh.c

source/blender/blenkernel/intern/pbvh_intern.h

source/blender/editors/sculpt_paint/sculpt.c

source/blender/editors/sculpt_paint/sculpt_automasking.cc

source/blender/editors/sculpt_paint/sculpt_intern.h

source/blender/editors/sculpt_paint/sculpt_paint_image.cc

Support Automasking For Texture Paint
Needs RevisionPublic
Actions

Revision Contents
Changeset List