GPv3: Add access functions for opacity and radius attributes #109733

Amélie Fondevilla · 2023-07-05T12:15:33+02:00

2023-07-05 12:15:33 +02:00

We need setters and getters for these two very commonly accessed attributes of grease pencil.
The code is based on the implementation of CurvesGeometry::positions() and CurvesGeometry::positions_for_write().

We need setters and getters for these two very commonly accessed attributes of grease pencil. The code is based on the implementation of `CurvesGeometry::positions()` and `CurvesGeometry::positions_for_write()`.

Amélie Fondevilla added 1 commit 2023-07-05 12:15:44 +02:00

0d4632f327 Add getters and setters for opacity and radius of a grease pencil drawing

Amélie Fondevilla changed title from ~~Add getters and setters for opacity and radius of a grease pencil drawing~~ to GPv3 : Add getters and setters for opacity and radius of a grease pencil drawing

2023-07-05 12:15:56 +02:00

Amélie Fondevilla added this to the Grease Pencil project 2023-07-05 12:16:04 +02:00

Amélie Fondevilla commented

2023-07-05 12:20:58 +02:00

Although, we needed to copy/paste the functions get_span_attribute, get_mutable_attribute, domain_num, domain_custom_data that are only defined in curves_geometry.cc and not visible from the API.
Is that ok, or should we make these functions visibles instead ?

Although, we needed to copy/paste the functions `get_span_attribute`, `get_mutable_attribute`, `domain_num`, `domain_custom_data` that are only defined in `curves_geometry.cc` and not visible from the API. Is that ok, or should we make these functions visibles instead ?

Amélie Fondevilla requested review from Hans Goudey 2023-07-05 12:21:22 +02:00

Amélie Fondevilla added 1 commit 2023-07-05 12:23:40 +02:00

d1d5c62564 Changed comment format. No functional changes.

Amélie Fondevilla referenced this pull request

2023-07-05 14:29:29 +02:00

GPv3: Stroke smoothing operator #109635

Iliya Katushenock commented

2023-07-05 15:51:43 +02:00

Make domain_num, domain_custom_data, domain_custom_data functions through a switch. This should return 0 for an unsupported domains. Now it will be the size of the curves, which is not always correct.. This may not work correctly if used in conjunction with other attribute components in geometry processing code.
Also, make sure you don't use C-cast (https://wiki.blender.org/wiki/Style_Guide/C_Cpp).

Make `domain_num`, `domain_custom_data`, `domain_custom_data` functions through a switch. This should return 0 for an unsupported domains. Now it will be the size of the curves, which is not always correct.. This may not work correctly if used in conjunction with other attribute components in geometry processing code. Also, make sure you don't use C-cast (https://wiki.blender.org/wiki/Style_Guide/C_Cpp).

Hans Goudey commented

2023-07-05 20:44:26 +02:00

The radii accessor makes just as much sense on the base CurvesGeometry class. We didn't add it yet though because unfortunately different code uses different defaults when the radii don't exist, this hasn't been standardized yet.

Maybe you could add an example of before and after?

The following isn't too bad when you get used to it IMO. But I understand the reasoning for avoiding writing it.
const VArray<float> opacity = *attributes.lookup_default<float>("opacity", ATTR_DOMAIN_POINT);

The radii accessor makes just as much sense on the base `CurvesGeometry` class. We didn't add it yet though because unfortunately different code uses different defaults when the radii don't exist, this hasn't been standardized yet. Maybe you could add an example of before and after? The following isn't too bad when you get used to it IMO. But I understand the reasoning for avoiding writing it. `const VArray<float> opacity = *attributes.lookup_default<float>("opacity", ATTR_DOMAIN_POINT);`

Falk David commented

2023-07-06 10:26:31 +02:00

@HooglyBoogly For the radii default value, I'm wondering if it would make sense to have a parameter in the function with a sensible default value. E.g. something like Span<float> radii(float default_radius = 1.0f) const;. So you can still just call curves.radii().

@HooglyBoogly For the radii default value, I'm wondering if it would make sense to have a parameter in the function with a sensible default value. E.g. something like `Span<float> radii(float default_radius = 1.0f) const;`. So you can still just call `curves.radii()`.

Falk David commented

2023-07-06 10:28:32 +02:00

Maybe you could add an example of before and after?

I'd like to have those function on the drawing, because opacity should really be a default attribute. And it's nice to not have to think about how the attribute is spelled, what the default should be etc.

> Maybe you could add an example of before and after? I'd like to have those function on the drawing, because `opacity` should really be a default attribute. And it's nice to not have to think about how the attribute is spelled, what the default should be etc.

Hans Goudey commented

2023-07-06 14:31:39 +02:00

The idea behind unifying the radius attribute was that every place shouldn't have to manage its own default. But that is a bit tricky since it may not be intuitive for the curve to mesh node main input, for example. I still think its worth exploring though.

What do you mean by "default attribute"? Makes sense though!

The idea behind unifying the radius attribute was that every place shouldn't have to manage its own default. But that is a bit tricky since it may not be intuitive for the curve to mesh node main input, for example. I still think its worth exploring though. What do you mean by "default attribute"? Makes sense though!

Falk David commented

2023-07-06 18:45:49 +02:00

@HooglyBoogly Something that we expect every stroke to have.

Hans Goudey commented

2023-07-06 19:04:25 +02:00

@HooglyBoogly Something that we expect every stroke to have.

That seems a bit vague to me honestly, maybe it can be "expected" but code still has to handle the case where it doesn't exist, in which case the default should probably be 1.0, right? If so, the opacities() function should return a single-value virtual array.

> @HooglyBoogly Something that we expect every stroke to have. That seems a bit vague to me honestly, maybe it can be "expected" but code still has to handle the case where it doesn't exist, in which case the default should probably be 1.0, right? If so, the `opacities()` function should return a single-value virtual array.

Falk David commented

2023-07-06 19:44:22 +02:00

@HooglyBoogly Ok I see. Yes, we should instead use what CurvesGeometry is using now: get_varray_attribute. And the _for_write() functions can stay the same I think?

@HooglyBoogly Ok I see. Yes, we should instead use what `CurvesGeometry` is using now: `get_varray_attribute`. And the `_for_write()` functions can stay the same I think?

Hans Goudey changed title from ~~GPv3 : Add getters and setters for opacity and radius of a grease pencil drawing~~ to GPv3: Add getters and setters for opacity and radius of a grease pencil drawing

2023-07-10 13:44:00 +02:00

Amélie Fondevilla added 1 commit 2023-07-10 15:11:25 +02:00

c3ee7beb0d Fix the implementation of the getters

Hans Goudey reviewed 2023-07-10 16:02:12 +02:00

source/blender/blenkernel/BKE_grease_pencil.hh Outdated

						
				@ -79,0 +81,4 @@

				  /**

				   * Per-point opacity value

				   */

				  const VArray<float> opacities() const;

Hans Goudey commented

2023-07-10 16:01:24 +02:00

Making a return value const when it's not a reference or a pointer doesn't have any effect. For example, you can still do this:

VArray<float> value = thing.opacities();
value = VArray<float>>::ForSingle(1,0.0f);

Making a return value `const` when it's not a reference or a pointer doesn't have any effect. For example, you can still do this: ``` VArray<float> value = thing.opacities(); value = VArray<float>>::ForSingle(1,0.0f); ```

👍 1

filedescriptor marked this conversation as resolved

source/blender/blenkernel/BKE_grease_pencil.hh Outdated

						
				@ -79,0 +87,4 @@

				  /**

				   * Per-point radius value

				   */

				  const VArray<float> radii() const;

Hans Goudey commented

2023-07-10 16:02:07 +02:00

Hmm, my point with my comment about radii earlier is that we should wait to add this to CurvesGeometry directly when the current inconsistent situation is figured out.

Hmm, my point with my comment about radii earlier is that we should wait to add this to `CurvesGeometry` directly when the current inconsistent situation is figured out.

Falk David commented

2023-07-10 16:49:12 +02:00

Could we refactor that together with when that PR is happening?

Hans Goudey commented

2023-07-10 16:58:13 +02:00

Sure, I guess that's fine too

Amélie Fondevilla commented

2023-07-10 17:41:33 +02:00

Yes, because we actually need that for the smoothing algorithm and we are gonna need it for further development in the next weeks.

filedescriptor marked this conversation as resolved

Falk David added 2 commits 2023-07-11 10:48:10 +02:00

104f036f42 Merge branch 'main' into gpv3-attributes-api

ce0bc77d37 Remove unnecessary const

Falk David added 3 commits 2023-07-11 11:22:52 +02:00

c3eb36c91d Refactor: Use the radii and opacity accessors

77e145ad5d Move the const strings to the greasepencil namespace

8bf1098ca1 Cleanup

Hans Goudey reviewed 2023-07-11 13:46:40 +02:00

Hans Goudey left a comment

Would you consider using 0.01 for the default radius? That's the "most consistent" with curves, and it probably makes more sense at the scale most people work at anyway.

Would you consider using `0.01` for the default radius? That's the "most consistent" with curves, and it probably makes more sense at the scale most people work at anyway.

source/blender/blenkernel/BKE_grease_pencil.hh Outdated

						
				@ -78,1 +79,4 @@

				  void tag_positions_changed();

				  /**

				   * Per-point radius value.

Hans Goudey commented

2023-07-11 13:43:52 +02:00

For these high-level doc strings, I'd suggest going beyond the bare minimum of documentation. For example-- "the radius is mainly used to determine the thickness of the stroke when rendering" or "how transparent each point is, also affecting the influence of the blah blah"

If there really is nothing more to add that's less basic, maybe that's fine, but it's nice to help out all future readers of this header file :)

For these high-level doc strings, I'd suggest going beyond the bare minimum of documentation. For example-- "the radius is mainly used to determine the thickness of the stroke when rendering" or "how transparent each point is, also affecting the influence of the blah blah" If there really is nothing more to add that's less basic, maybe that's fine, but it's nice to help out all future readers of this header file :)

filedescriptor marked this conversation as resolved

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

						
				@ -346,1 +381,4 @@

				VArray<float> Drawing::radii() const

				{

				  return *strokes().attributes().lookup_or_default<float>(ATTR_RADIUS, ATTR_DOMAIN_POINT, 1.0f);

Hans Goudey commented

2023-07-11 13:44:58 +02:00

this->strokes()

Generally class methods are referred to with that prefix.

`this->strokes()` Generally class methods are referred to with that prefix.

filedescriptor marked this conversation as resolved

Falk David added 3 commits 2023-07-11 14:26:01 +02:00

ecf120b511 Use `this`

49989c556f Change the docstring to be a bit more helpful

b7a60db6b8 Use 0.01 as the default radius.

Iliya Katushenock commented

2023-07-11 14:30:07 +02:00

const int num = domain_num(curves, domain); -> curves.attributes().domain_size(domain)
const int num -> const int domain_size
MutableSpan<T> span = {data, num}; -> MutableSpan<T> values(data, num);
T *data = (T *)Cu.. -> T *data = static_cast<T *>(Cu
num > 0 really necessary?
span.first() != default_value why might this be so? There after all or garbage, or zeros?
template<typename T> static MutableSpan<T> get_mutable_attribute can be generic?

`const int num = domain_num(curves, domain);` -> `curves.attributes().domain_size(domain)` `const int num` -> `const int domain_size` `MutableSpan<T> span = {data, num};` -> `MutableSpan<T> values(data, num);` `T *data = (T *)Cu..` -> `T *data = static_cast<T *>(Cu` `num > 0` really necessary? `span.first() != default_value` why might this be so? There after all or garbage, or zeros? `template<typename T> static MutableSpan<T> get_mutable_attribute` can be generic?

Falk David commented

2023-07-11 14:40:24 +02:00

@mod_moder I appreciate the feedback, but it would be nice if you could add these comments to the lines in the diff.

Iliya Katushenock commented

2023-07-11 14:43:22 +02:00

@filedescriptor After issuecomment i'm trying to be more careful in this part

@filedescriptor After [issuecomment](https://projects.blender.org/blender/blender/pulls/109405#issuecomment-968538) i'm trying to be more careful in this part

Falk David commented

2023-07-11 14:45:36 +02:00

@mod_moder Right, you can use the Start Review button instead of adding single comments, to bunch them all up into one.

@mod_moder Right, you can use the `Start Review` button instead of adding single comments, to bunch them all up into one.

👍 1

Hans Goudey approved these changes 2023-07-11 14:50:10 +02:00

Hans Goudey left a comment

Noted one mistake inline, other than that looks good. Some of Iliya's comments are reasonable too, though I understand this code is a copy & paste so that could be handled separately.

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

						
				@ -347,0 +386,4 @@

				MutableSpan<float> Drawing::radii_for_write()

				{

				  return get_mutable_attribute<float>(this->geometry.wrap(), ATTR_DOMAIN_POINT, ATTR_RADIUS);

Hans Goudey commented

2023-07-11 14:48:57 +02:00

specify the default value here too

filedescriptor marked this conversation as resolved

Hans Goudey changed title from ~~GPv3: Add getters and setters for opacity and radius of a grease pencil drawing~~ to GPv3: Add access functions for opacity and radius attributes

2023-07-11 14:50:40 +02:00

Falk David added 1 commit 2023-07-11 14:56:41 +02:00

54c3b5f9a2 Default values

Falk David merged commit f2d3d321bb into main

2023-07-11 15:11:21 +02:00

Falk David referenced this issue from a commit

2023-07-11 15:11:21 +02:00

GPv3: Add access functions for opacity and radius attributes

Falk David deleted branch gpv3-attributes-api

2023-07-11 15:11:22 +02:00

Andras-Borsanyi referenced this issue from a commit

2023-07-12 17:32:38 +02:00