Sculpt: Add brush settings for view & normal automasking values #117433

Sean Kim · 2024-01-23T03:10:37+01:00

Sean Kim commented

2024-01-23 03:10:37 +01:00

This pull request adds the ability for the Limit and Falloff values for the View and Normal automask modes to be applied per-brush instead of modifying the global tool value.

Previously, while the flag settings were stored at the brush and global level, the values would always change the global value even when using the advanced tab of the brush menu.

Testing

make test results in 3 failures (49 - io_wavefront, 134 - compositor_distort_cpu, 323 - imbuf_save), but these tests appear to fail even prior to any changes being applied when running locally
Manual testing with older version blend files to ensure that the value is defaulted correctly.
Visual testing to verify that brush values are favored over global values.

Addresses #115174

This pull request adds the ability for the `Limit` and `Falloff` values for the View and Normal automask modes to be applied per-brush instead of modifying the global tool value. Previously, while the flag settings were stored at the brush and global level, the values would always change the global value even when using the advanced tab of the brush menu. ## Testing * `make test` results in 3 failures (49 - io_wavefront, 134 - compositor_distort_cpu, 323 - imbuf_save), but these tests appear to fail even prior to any changes being applied when running locally * Manual testing with older version blend files to ensure that the value is defaulted correctly. * Visual testing to verify that brush values are favored over global values. Addresses #115174

Sean Kim added 1 commit 2024-01-23 03:10:42 +01:00

ca1dc4b4e8 Sculpt: Add brush settings for view & normal automasking values

Adds data for brush level values and ensures that brush values are
preferred over tool-level values.

Ref \#115174

Sean Kim requested review from Sergey Sharybin 2024-01-23 03:11:17 +01:00

Sean Kim requested review from Julien Kaspar 2024-01-23 03:11:25 +01:00

Sean Kim requested review from Hans Goudey 2024-01-23 03:11:53 +01:00

Hans Goudey reviewed 2024-01-23 03:58:13 +01:00

Hans Goudey left a comment

I have no opinion about scene vs. brush values being better than one another. If Julien prefers this, I'm happy to go with it. Thanks for the PR. Just a couple comments inline.

source/blender/blenloader/intern/versioning_defaults.cc

						
				@ -827,1 +827,4 @@

				  }

				  {

				    const Brush *default_brush = DNA_struct_default_get(Brush);

Hans Goudey commented

2024-01-23 03:55:26 +01:00

I would have expected startup.blend is versioned by blo_do_versions_400, maybe I'm wrong though. Did you check that this was necessary?

I would have expected `startup.blend` is versioned by `blo_do_versions_400`, maybe I'm wrong though. Did you check that this was necessary?

Sean Kim commented

2024-01-23 04:28:26 +01:00

According to the comment at the top of the file & dna_defaults.c this is where the startup.blend updating happens, not sure why blo_do_versions_400 doesn't apply to it, but when I've done manual testing before for other issues the value wasn't updated for new files unless I made these corresponding changes.

According to the comment at the top of the file & `dna_defaults.c` this is where the `startup.blend` updating happens, not sure why `blo_do_versions_400` doesn't apply to it, but when I've done manual testing before for other issues the value wasn't updated for new files unless I made these corresponding changes.

👍 1

Sean-Kim marked this conversation as resolved

source/blender/makesdna/DNA_brush_defaults.h Outdated

						
				@ -93,2 +93,4 @@

				    .pose_ik_segments = 1, \

				    .hardness = 0.0f, \

				 \

				    /* automasking */ \

Hans Goudey commented

2024-01-23 03:56:39 +01:00

This automasking comment is unnecessary IMO, all the next settings start with that word anyway

This `automasking` comment is unnecessary IMO, all the next settings start with that word anyway

👍 1

Sean-Kim marked this conversation as resolved

source/blender/makesdna/DNA_brush_defaults.h

						
				@ -95,1 +97,4 @@

				    .automasking_boundary_edges_propagation_steps = 1, \

				    .automasking_start_normal_limit = 0.34906585f, /* 20 / 180 * pi. */ \

				    .automasking_start_normal_falloff = 0.25f, \

				    .automasking_view_normal_limit = 1.570796, /* 0.5 * pi. */ \

Hans Goudey commented

2024-01-23 03:57:04 +01:00

For the comments, how about writing these values in degrees?

👍 1

Sean Kim commented

2024-01-23 04:30:06 +01:00

Agreed, left it like this originally to make it match the value in DNA_scene_defaults.h, but degrees makes more sense as a comment.

Agreed, left it like this originally to make it match the value in `DNA_scene_defaults.h`, but degrees makes more sense as a comment.

Sean-Kim marked this conversation as resolved

source/blender/makesdna/DNA_brush_types.h Outdated

						
				@ -317,6 +317,9 @@ typedef struct Brush {

				  int automasking_flags;

				  int automasking_boundary_edges_propagation_steps;

				  float automasking_start_normal_limit, automasking_start_normal_falloff;

Hans Goudey commented

2024-01-23 03:56:16 +01:00

These days each value is typically declared on its own line. Just easier to read that way.

👍 1

Sean-Kim marked this conversation as resolved

Sean Kim added 1 commit 2024-01-23 04:35:40 +01:00

e0d5905986 Minor comment cleanup for CR

Hans Goudey approved these changes 2024-01-23 05:02:44 +01:00

Sergey Sharybin commented

2024-01-23 09:50:17 +01:00

Do we expect the automasking to behave similar to other unified settings of the brushes?

If yes, then it seems something is missing in the UI and in the cache_settings_update to choose either of those settings.
If no, we should not have unaccessible Sculpt-level properties for those values, including rna_def_sculpt(), and Sculpt.

Do we expect the automasking to behave similar to other unified settings of the brushes? If yes, then it seems something is missing in the UI and in the `cache_settings_update` to choose either of those settings. If no, we should not have unaccessible Sculpt-level properties for those values, including `rna_def_sculpt()`, and `Sculpt`.

Julien Kaspar approved these changes 2024-01-23 09:56:07 +01:00

Julien Kaspar left a comment

On the user side this fixes the issue. Thanks for the PR!

Sergey Sharybin approved these changes 2024-01-24 09:47:09 +01:00

Sergey Sharybin left a comment

Sean pointer out in the chat that the setting is actually exposed in the Auto-Smooth pop-over. To me the current UI quite confusing, as it is not clear what the relation between global sculpt and brush settings is. However, it is something that was there already, with the cavity auto-masking behavior.

Would be very nice to work on this, but it is not for the scope of this patch.

If Julien is happy with the current state of the PR lets land it. The code seems fine.

Sean pointer out in the chat that the setting is actually exposed in the Auto-Smooth pop-over. To me the current UI quite confusing, as it is not clear what the relation between global sculpt and brush settings is. However, it is something that was there already, with the cavity auto-masking behavior. Would be very nice to work on this, but it is not for the scope of this patch. If Julien is happy with the current state of the PR lets land it. The code seems fine.

Sergey Sharybin merged commit 1e4f133950 into main

2024-01-24 10:59:59 +01:00

Sergey Sharybin referenced this issue from a commit

2024-01-24 11:00:00 +01:00

Sculpt: Add brush settings for view & normal automasking values

Sergey Sharybin deleted branch 115174-automask-limit-falloff

2024-01-24 11:00:02 +01:00

Jonas Dichelle referenced this issue from a commit

2024-02-08 17:29:05 +01:00

Sculpt: Add brush settings for view & normal automasking values

Sign in to join this conversation.

No reviewers

No Label

Download

What's New

Blender Studio

Manual

Developers Blog

Documentation

Benchmark

Blender Conference

Development Fund

One-time Donations

Sculpt: Add brush settings for view & normal automasking values #117433

Testing