Page MenuHome

New sculpting brush cursor
ClosedPublic

Authored by Pablo Dobarro (pablodp606) on Aug 11 2018, 1:24 AM.
Tokens
"Love" token, awarded by chardetm."Love" token, awarded by bnzs."100" token, awarded by Frozen_Death_Knight."Love" token, awarded by brilliant_ape."Love" token, awarded by franMarz."Love" token, awarded by ace_dragon."Love" token, awarded by symstract."Love" token, awarded by mankysee."Love" token, awarded by ish_bosamiya."Love" token, awarded by TheFlow."Love" token, awarded by kioku."Like" token, awarded by capnm."Burninate" token, awarded by billreynish."Love" token, awarded by Largerous."Love" token, awarded by shomyshomy."Love" token, awarded by 0o00o0oo."Love" token, awarded by svankirk."Love" token, awarded by Jaydead."Love" token, awarded by TheCharacterhero."Like" token, awarded by ebarranco."Like" token, awarded by Loner."Love" token, awarded by ofuscado."Love" token, awarded by ThinkingPolygons."Love" token, awarded by elbox01."Yellow Medal" token, awarded by KloWorks."Love" token, awarded by monio."Love" token, awarded by kyjelblue."Love" token, awarded by hjrabi."Love" token, awarded by jmztn."Like" token, awarded by gobb_blend."Love" token, awarded by d.viagi."Love" token, awarded by barracuda."Love" token, awarded by sephirothtbm."Love" token, awarded by julperado."Love" token, awarded by Noss."Mountain of Wealth" token, awarded by Zino."Love" token, awarded by JimMorren."100" token, awarded by shader."Love" token, awarded by JulienKaspar."Like" token, awarded by TheRedWaxPolice."Love" token, awarded by plyczkowski."Love" token, awarded by RyanJEC."Love" token, awarded by jonathanl."Love" token, awarded by johnsyed."Love" token, awarded by eoinoneill.yokai."Love" token, awarded by andreasus."Like" token, awarded by Schamph."Love" token, awarded by fiendish55."Love" token, awarded by xrg."Love" token, awarded by amonpaike."Love" token, awarded by KINjO."Love" token, awarded by pablovazquez."Love" token, awarded by miclack."Love" token, awarded by Alrob."Party Time" token, awarded by Scaredyfish."Mountain of Wealth" token, awarded by charlie."Love" token, awarded by wo262."Like" token, awarded by fclem."Burninate" token, awarded by DotBow."Like" token, awarded by erickblender."Like" token, awarded by Samirosman.

Details

Summary

This patch adds a new brush cursor with surface normal and vertex preview.

I've been testing this for a while and I think that using the face normal is always a better option, even with high poly meshes. The vertex normal calculation with big brushes doesn't give you any useful information about the surface. On high poly surfaces with noise, the brush moves a lot if you are using the face normal, but I think that is the expected behavior. For now, both methods are implemented in this patch for testing purposes.

Diff Detail

Repository
rB Blender

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
Pablo Dobarro (pablodp606) updated this revision to Diff 13711.

Fix cursor preview size on translated objects

Seems like most issues are solved now, looking great!

Any other code tweaks you'd like to do or is up to the developers to give another review pass on this?


Thanks for keeping the patch updated and taking feedback so well btw, the review process can be tough and can get new devs easily discouraged. You rock @Pablo Dobarro (pablodp606)! 🤘

Can someone share a build or did you guys check this in dyntopo?

@Pablo Vazquez (pablovazquez) The brush presets need to be updated to include the normal radius default value. Also, the grab brush preview on low poly meshes is a little bit weird, but it can not be done properly without an unnecessary hack.
Regarding optimizations, maybe adding Monte Carlo sampling only for the cursor is not worth it. If a computer can not run the cursor in real time it will not be able to sculpt. Furthermore, the cursor is disabled when a stroke is active, so sculpting performance should not be affected.
I think the rest of the functionality should be fine right now.

@Pablo Dobarro (pablodp606) Given the enormous value of what you are doing here, I just want to make sure this is followed up on. Do you have all you need to proceed, or do you need developers to review?

@William Reynish (billreynish) I would like to have a code review, I can always make small changes to the functionality in the future if that is needed.

Since there are still so many todo's and unfinished features for 2.8x, would hold off on this feature until after 2.80 initial release.

Since there are still so many todo's and unfinished features for 2.8x, would hold off on this feature until after 2.80 initial release.

That is, this function will not be in the initial version 2.8?

hold off on this feature until after 2.80 initial release.

Now that's what I call bad news.
It's sad, this is such an amazing improvement.

Since there are still so many todo's and unfinished features for 2.8x, would hold off on this feature until after 2.80 initial release.

I was reading this thread for a while and finally decided to register after your comment.

I'm strongly disagree with this notion and eagerly asking you to reevaluate it.

I do understand that there are many todo's and unfinished features that are waiting for their time but the difference is -- unlike todo's this feature is already implemented and sooner users start to use it sooner there will be more development in this department that was, almost literally abandoned for three years. Postponing it for another half to year(let's be realistic here: there are always more urgent things to do and the initial release itself most probably is more than three months away from now) is basically means killing it and making obsolete by the time you got your time to look at it closer(due to fatigue of the developer and neck breaking speed of development right now among other things).

Right now it's an amazing feature that make something really useful and greatly improves productivity unlike some other features that are already implemented(I totally respect and like features like shadows in workbench for example but it's usefulness kinda lacking comparing to this patch and lacking greatly). And the sooner it will be available for more users so they can give feedback and evaluate it the better.

If Pablo made the promise to fix any issues his sculpting patches bring up (as opposed to leaving it to the core team), then I don't see why it shouldn't be committed, especially as there's nothing in dev. discussion mentioning a feature freeze.

My opinion anyway.

If Pablo made the promise to fix any issues his sculpting patches bring up (as opposed to leaving it to the core team), then I don't see why it shouldn't be committed, especially as there's nothing in dev. discussion mentioning a feature freeze.
My opinion anyway.

Because it is not well tested yet, look at Pablo's video most of them are without Dyntopo, i don't know about multires. I stand by the core devs. there is no need to rush feature. @Pablo Dobarro (pablodp606) Please provide a build so people can test and give feedback.

In D3594#101059, @Erick Tukuniata (erickblender) wrote: Please provide a build so people can test and give feedback.

He already did. It's on his Twitter page.

Jun Mizutani (jmztn) added a comment.EditedMar 7 2019, 2:55 PM

This patch will make Sculpt Mode menu "Brush/Reset Brush" work as expected.

diff --git a/source/blender/blenkernel/intern/brush.c b/source/blender/blenkernel/intern/brush.c
index 346066b961a..859a2661349 100644
--- a/source/blender/blenkernel/intern/brush.c
+++ b/source/blender/blenkernel/intern/brush.c
@@ -71,6 +71,7 @@ static void brush_defaults(Brush *brush)
        brush->size = 35; /* radius of the brush in pixels */
        brush->alpha = 0.5f; /* brush strength/intensity probably variable should be renamed? */
        brush->autosmooth_factor = 0.0f;
+       brush->normal_radius_factor = 0.2f;
        brush->topology_rake_factor = 0.0f;
        brush->crease_pinch_factor = 0.5f;
        brush->sculpt_plane = SCULPT_DISP_DIR_AREA;
Jun Mizutani (jmztn) added a comment.EditedMar 9 2019, 8:55 AM
In D3594#97225, @Pablo Dobarro (pablodp606) wrote: Currently, you need to change this value for every brush, and this needs to be fixed for the final version. I don't know why the default value is not taken into account, maybe we need to update the startup.blend file with new brushes that have this value set by default?

This patch will update factory settings for normal_radius_factor. This patch was made against v2.80.47.

Affected by changes in struct Main(commit 8f817de0cbef41dac81e6c7665ada509c3fe2988), the element "brush.first" needed to be changed to "brushes.first".

@Jun Mizutani (jmztn) Thanks! I added it to the sculpt-mode-features branch

Great job overall. Thanks Pablo.
Ive tested your improvements and ive found that you have changed behavior of cursor ring.
Right now when im making stroke, the outer ring that shows size of brush dissapears. I think that this is not a good idea. I understand that you try to mimic zbrush but from user perspective its always nice to know how big brush is and how big can become with max pressure. Right now im unable to judge this. So im asking for a little revert to 2.79 behavior with new surface normal behavior (its basicly, same as 3dCoat) or a little checkbox would do the job just fine.
Another thing is that if you enable tablet pressure sensitivity button (next to radius bar), while stroking outer ring is visible (thats good) but does not follow surface normals.

Best regards.

ZQ (ZQ) removed a subscriber: ZQ (ZQ).

Almost 1 year since this patch has been sitting here, kindly reminder. Now i m running and hiding before Mods comes LOL

Patch updated including symmetry preview and normal radius

Campbell Barton (campbellbarton) requested changes to this revision.Tue, Aug 27, 5:35 AM
Campbell Barton (campbellbarton) added inline comments.
source/blender/blenkernel/BKE_paint.h
63

No need to include this, use struct RegionView3D *rv3d; instead.

source/blender/blenkernel/intern/pbvh_bmesh.c
1516

Since this is an output arg, should call r_face_normal[3]

Same for int *r_active_vertex_index

1556

BMesh indices aren't guaranteed to be valid, should call BM_mesh_elem_index_ensure(bm, BM_VERT) before accessing.

source/blender/editors/sculpt_paint/paint_cursor.c
1139–1145

Most of Blender's code (including code in this patch uses post-increment), prefer this convention unless pre-increment is necessary.

source/blender/editors/sculpt_paint/sculpt.c
98–112

Unused functions after this

[  5%: -1183] Building C object source/blender/edito...int/CMakeFiles/bf_editor_sculpt_paint.dir/sculpt.c.o
/src/blender/source/blender/editors/sculpt_paint/sculpt.c:296:13: warning: ‘sculpt_vertex_neighbors_get’ defined but not used [-Wunused-function]
  296 | static void sculpt_vertex_neighbors_get(SculptSession *ss,
      |             ^~~~~~~~~~~~~~~~~~~~~~~~~~~
/src/blender/source/blender/editors/sculpt_paint/sculpt.c:196:13: warning: ‘sculpt_vertex_tag_update’ defined but not used [-Wunused-function]
  196 | static void sculpt_vertex_tag_update(SculptSession *ss, int index)
      |             ^~~~~~~~~~~~~~~~~~~~~~~~
/src/blender/source/blender/editors/sculpt_paint/sculpt.c:180:14: warning: ‘sculpt_vertex_mask_get’ defined but not used [-Wunused-function]
  180 | static float sculpt_vertex_mask_get(SculptSession *ss, int index)
      |              ^~~~~~~~~~~~~~~~~~~~~~
/src/blender/source/blender/editors/sculpt_paint/sculpt.c:162:13: warning: ‘sculpt_vertex_mask_set’ defined but not used [-Wunused-function]
  162 | static void sculpt_vertex_mask_set(SculptSession *ss, int index, float mask)
      |             ^~~~~~~~~~~~~~~~~~~~~~
/src/blender/source/blender/editors/sculpt_paint/sculpt.c:148:13: warning: ‘sculpt_vertex_co_set’ defined but not used [-Wunused-function]
  148 | static void sculpt_vertex_co_set(SculptSession *ss, int index, float co[3])
      |             ^~~~~~~~~~~~~~~~~~~~
/src/blender/source/blender/editors/sculpt_paint/sculpt.c:123:13: warning: ‘sculpt_vertex_normal_get’ defined but not used [-Wunused-function]
  123 | static void sculpt_vertex_normal_get(SculptSession *ss, int index, float no[3])
      |             ^~~~~~~~~~~~~~~~~~~~~~~~
/src/blender/source/blender/editors/sculpt_paint/sculpt.c:111:12: warning: ‘sculpt_vertex_count_get’ defined but not used [-Wunused-function]
  111 | static int sculpt_vertex_count_get(SculptSession *ss)
      |            ^~~~~~~~~~~~~~~~~~~~~~~
/src/blender/source/blender/editors/sculpt_paint/sculpt.c:99:12: warning: ‘sculpt_active_vertex_get’ defined but not used [-Wunused-function]
   99 | static int sculpt_active_vertex_get(SculptSession *ss)
      |            ^~~~~~~~~~~~~~~~~~~~~~~~
5325

Unused variable.

source/blender/editors/sculpt_paint/sculpt_intern.h
30

Not needed.

176

Can be a bool.

This revision now requires changes to proceed.Tue, Aug 27, 5:35 AM

What is the purpose of highlighting the closest vertex?

Sculpting doesn't treat this vertex differently to others, so I'm not sure why it's necessary to draw a point on top of it.

On lower poly meshes snapping to this vertex feels jittery.

Whats the advantage of drawing a point over the vertex compared to a point at the center of the brush? (the ray-cast hit location)

Pablo Dobarro (pablodp606) marked 8 inline comments as done.Tue, Aug 27, 3:58 PM
Pablo Dobarro (pablodp606) updated this revision to Diff 17581.
  • Update for review
  • Fix bug in cursor view normal calculation

This new cursor will work in Texture Paint Mode right? I know the title says sculpt cursor but doesn't Sculpt Mode and Texture Paint mode share the same cursor? Shouldn't the new cursor be added in all relevant areas (Sculpt Mode & Texture Paint Mode) before being committed?

Just want to make sure we get this awesome new cursor in all the right places.

Brecht Van Lommel (brecht) requested changes to this revision.Fri, Aug 30, 12:29 PM

@Pablo Dobarro (pablodp606) @Campbell Barton (campbellbarton), was there a conclusion regarding displaying the active vertex? To me it also makes little sense. If there are future tools that use the active vertex then those specifically could display it.

@Ryan (RyanJEC), this is for sculpting only. In texture paint mode the painting operation doesn't actually follow the normal and just displaying it as before is more accurate.

source/blender/blenloader/intern/versioning_280.c
3694–3700

Move this code below: /* Versioning code until next subversion bump goes here. */.

Otherwise it does not get applied to recently saved files.

3695

Simpler: for (Brush *br = bmain->brushes.first; br; br = br->id.next) {

source/blender/editors/sculpt_paint/paint_cursor.c
1099–1107

Use ED_view3d_project instead of custom code.

source/blender/editors/sculpt_paint/sculpt_intern.h
49

Rename to SculptCursorGeometryInfo to match the function name? Not sure why Stroke would be in the name.

This revision now requires changes to proceed.Fri, Aug 30, 12:29 PM

@Ryan (RyanJEC), this is for sculpting only. In texture paint mode the painting operation doesn't actually follow the normal and just displaying it as before is more accurate.

Maybe following the normals should be an option in texture paint mode? I have used Substance Painter a few times and I'm pretty sure that it's what it does, and I'm pretty sure the cursor was similar to this new cursor too! This should probably have its own thread though...

Pablo Dobarro (pablodp606) marked 6 inline comments as done.Fri, Aug 30, 3:25 PM
Pablo Dobarro (pablodp606) updated this revision to Diff 17698.
  • Update for review

The active vertex is used in almost all new brush options, not only in tools. Also, there are some operators that use the active vertex that you can start directly without switching to a tool. In high poly meshes it is almost the same as having the dot centered in the cursor. In low poly meshes you can snap the maximum brush strength to the active vertex, and this improves the behavior of the brushes a lot.

Campbell Barton (campbellbarton) added inline comments.
source/blender/editors/sculpt_paint/sculpt.c
5450–5453

This could be postponed unless its needed (moved down below hit check for example).

Ok, I'm fine to have it use the active vertex and then we see how it gets used by the other tools you're adding. We can always revisit this.

The implementation looks ok to me then, but I would like @Campbell Barton (campbellbarton) to accept this as well (or request changes) to be sure.

This revision is now accepted and ready to land.Fri, Aug 30, 4:22 PM

I didn't see Campbell already accepted before me, nevermind the last part of my previous comment.

  • Remove unnecesary index ensure