Sculpt: Add per-brush input samples #117080

Sean Kim · 2024-01-13T00:13:50+01:00

Sean Kim commented

2024-01-13 00:13:50 +01:00

This pull request adds the ability for users to specify input_samples on a per brush basis.

The existing field in the main Paint struct forces all brushes of a particular tool type to use the same value. A new field was added to the Brush struct to allow for this value to be specified there instead, and a corresponding unified value in UnifiedPaintSettings has been created to allow users to use the same value across all brushes.

Limitations

While the previous functionality allowed for storing data on a per-tool basis, the current implementation in this pull request only provides users the ability to specify input_samples on a brush or overall basis. Therefore, if any user happens to currently specify different values for different tool modes, they will be unable to after this pull request is merged. Additionally, in the case that multiple non-default values exist for this field currently in a blend file, the current implementation does not favor a particular tool's value for migrating data into the UnifiedPaintSettings and instead leaves it at the default value of 1.

Testing

Added minor unit tests
make test results in 3 failures (46 - gpu, 49 - io_wavefront, 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 the existing data is migrated appropriately

Appearance

Addresses #108109

This pull request adds the ability for users to specify `input_samples` on a per brush basis. The existing field in the main `Paint` struct forces all brushes of a particular tool type to use the same value. A new field was added to the `Brush` struct to allow for this value to be specified there instead, and a corresponding unified value in `UnifiedPaintSettings` has been created to allow users to use the same value across all brushes. ## Limitations While the previous functionality allowed for storing data on a per-tool basis, the current implementation in this pull request only provides users the ability to specify `input_samples` on a brush or overall basis. Therefore, if any user happens to currently specify different values for different tool modes, they will be unable to after this pull request is merged. Additionally, in the case that multiple non-default values exist for this field currently in a blend file, the current implementation does not favor a particular tool's value for migrating data into the `UnifiedPaintSettings` and instead leaves it at the default value of 1. ## Testing * Added minor unit tests * `make test` results in 3 failures (46 - gpu, 49 - io_wavefront, 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 the existing data is migrated appropriately ## Appearance ![image](/attachments/097c9fc7-f34e-4219-92e9-4225a116206a) Addresses #108109

image.png

56 KiB

Sean Kim commented

2024-01-13 00:15:35 +01:00

Tagging @Sergey @HooglyBoogly and @JulienKaspar as specified in the original issue for review.

Sean Kim requested review from Sergey Sharybin 2024-01-13 00:16:20 +01:00

Sean Kim requested review from Julien Kaspar 2024-01-13 00:16:21 +01:00

Sean Kim requested review from Hans Goudey 2024-01-13 00:16:21 +01:00

Iliya Katushenock added this to the Sculpt, Paint & Texture project 2024-01-13 11:29:20 +01:00

Julien Kaspar approved these changes 2024-01-15 11:46:38 +01:00

Julien Kaspar left a comment

On the user side this works exactly like expected 👍
This will be very useful for the upcoming essential brushes.

For now I'm fine with the loss of per mode Input Samples. At some point the Unified Settings should be stored per mode anyway, which would improve this further.

On the user side this works exactly like expected 👍 This will be very useful for the upcoming essential brushes. For now I'm fine with the loss of per mode Input Samples. At some point the Unified Settings should be stored per mode anyway, which would improve this further.

Julien Kaspar added the

Module

Sculpt, Paint & Texture

label 2024-01-16 10:17:30 +01:00

Sean Kim force-pushed 108109-unified-input-samples from 6509ffeda8 to 7ebb780257

2024-01-17 03:59:37 +01:00

Compare

Hans Goudey requested changes 2024-01-19 19:55:48 +01:00

Hans Goudey left a comment

Looks good, just a few comments.

source/blender/blenkernel/intern/brush_test.cc Outdated

						
				@ -0,0 +10,4 @@

				#include "BKE_brush.hh"

				namespace blender::bke::tests {

				TEST(brush, BKE_brush_input_samples_get_unified)

Hans Goudey commented

2024-01-19 19:55:33 +01:00

IMO these tests add more trouble than they're worth. The functions being tested are just a few lines long, with extremely simple logic. I'd suggest skipping them, and maybe instead spend time adding tests for more complex areas of the code :)

Sean Kim commented

2024-01-19 21:42:15 +01:00

Will do, I was on the fence about adding them, but I decided to keep it in the PR when submitting just to err on the side of more rather than less.

Hans Goudey commented

2024-01-19 21:44:29 +01:00

You made the right call if you were on the fence :) I love to see the excitement about testing, just have to be pragmatic too I think

Sean-Kim marked this conversation as resolved

source/blender/blenloader/intern/versioning_300.cc Outdated

						
				@ -4559,6 +4559,61 @@ void blo_do_versions_300(FileData *fd, Library * /*lib*/, Main *bmain)

				    FOREACH_NODETREE_END;

				  }

				  if (!MAIN_VERSION_FILE_ATLEAST(bmain, 306, 12)) {

Hans Goudey commented

2024-01-19 19:54:21 +01:00

Only the versioning in the 400.cc file should be necessary as far as I know. That will always be applied.

👍 1

Sean-Kim marked this conversation as resolved

source/blender/makesdna/DNA_scene_types.h

						
				@ -1339,2 +1343,4 @@

				  float secondary_rgb[3];

				  /** Unified brush stroke input samples. */

				  int input_samples;

Hans Goudey commented

2024-01-19 19:53:51 +01:00

Could we just change the meaning of num_input_samples instead of adding a new field here? That seems simpler in the long run.

Could we just change the meaning of `num_input_samples` instead of adding a new field here? That seems simpler in the long run.

Sean Kim commented

2024-01-19 21:50:40 +01:00

That seems like it would add more logic onto the python side of things to handle this instead of using the pattern that already exists for the UnifiedPaintSettings, and I think that having the flag stored with the other unified attributes while having the value stored in the Paint struct will add to confusion down the line. I'm not strictly opposed, but I think changing it this way groups the attribute nicely with other similar concepts.

That seems like it would add more logic onto the python side of things to handle this instead of using the pattern that already exists for the `UnifiedPaintSettings`, and I think that having the flag stored with the other unified attributes while having the value stored in the `Paint` struct will add to confusion down the line. I'm not strictly opposed, but I think changing it this way groups the attribute nicely with other similar concepts.

Hans Goudey commented

2024-01-25 14:41:56 +01:00

Hmm, what I mean is just using the same integer in Paint. Why would that add logic in Python?

Hmm, what I mean is just using the same integer in `Paint`. Why would that add logic in Python?

Sean Kim commented

2024-01-25 21:57:40 +01:00

It's not a big change, but this part of UnifiedPaintPanel.prop_unified :

        ups = context.tool_settings.unified_paint_settings
        prop_owner = brush
        if unified_name and getattr(ups, unified_name):
            prop_owner = ups

        row.prop(prop_owner, prop_name, icon=icon, text=text, slider=slider)

assumes that the only possible owners of a property are either the brush or the UnifiedPaintSettings struct. Keeping the value in Paint would either mean adding a property check against num_input_samples or adding a new optional property to the method signature to allow passing in the Paint object. In my opinion this seems like adding clutter to this method.

To me the added complexity of moving this attribute to UnifiedPaintSettings and deprecating the old one is worth it primarily due to future readability of the struct, instead of having the flag in one place and the actual value that it corresponds to somewhere else.

It's not a big change, but [this part](https://projects.blender.org/blender/blender/src/commit/b61496c5089260a5a35ed4863685ef072e7c5e79/scripts/startup/bl_ui/properties_paint_common.py#L89-L119 ) of `UnifiedPaintPanel.prop_unified` : ``` ups = context.tool_settings.unified_paint_settings prop_owner = brush if unified_name and getattr(ups, unified_name): prop_owner = ups row.prop(prop_owner, prop_name, icon=icon, text=text, slider=slider) ``` assumes that the only possible owners of a property are either the brush or the `UnifiedPaintSettings` struct. Keeping the value in `Paint` would either mean adding a property check against `num_input_samples` or adding a new optional property to the method signature to allow passing in the `Paint` object. In my opinion this seems like adding clutter to this method. To me the added complexity of moving this attribute to `UnifiedPaintSettings` and deprecating the old one is worth it primarily due to future readability of the struct, instead of having the flag in one place and the actual value that it corresponds to somewhere else.

Hans Goudey commented

2024-01-25 22:02:28 +01:00

Aah, oops! I missed the separation between the two changes in this file, that they were in different structs. Sorry for wasting your time!

Sean Kim commented

2024-01-25 22:04:26 +01:00

No worries, it was a fair question to ask! 😄

Sean-Kim marked this conversation as resolved

Sean Kim added 2 commits 2024-01-20 00:42:44 +01:00

3c888eefc4 Merge branch 'main' of projects.blender.org:blender/blender into 108109-unified-input-samples

65c312aaa1 Address CR comments

Sean Kim requested review from Hans Goudey 2024-01-20 00:42:59 +01:00

Sean Kim added 1 commit 2024-01-25 03:52:19 +01:00