WIP: Refactor: GPv3: Frames API #121403

Falk David · 2024-05-03T17:47:11+02:00

Falk David commented

2024-05-03 17:47:11 +02:00

The GPv3 API for frames was less than ideal.

Problems:

The drawing array was exposed through the public API leading to issues where the user count of drawings could get out of sync. E.g. Layer::add_frame and Layer::remove_frame potentially changes the user count of drawings, but doesn't actually update them (because the layer cannot access the drawings array).
Using drawing indices as a way to reference drawings is extremly fragile. If the drawing array changes in any way, indices become invalid.

Solution:

Don't expose the drawing array to the public API. Similar to how the user interacts with grease pencil, drawings are always "attached" to keyframes.
Drawings are always read/created/deleted through the keyframe API:
- To create a drawing, use Drawing *drawing = grease_pencil.insert_frame(layer, ...).
- To remove drawings, remove the keyframes grease_pencil.remove_frames(...). Drawings with no users (keyframes) get deleted.
- duplicate_keys(...) to duplicate drawings, remap_keys(...), etc.
Drawings shouldn't be accessed using the drawings array. Instead, we use the get_drawing_at, get_editable_drawing_at API to retrieve drawings on layers at a specific time.

Resolves #120221.

The GPv3 API for frames was less than ideal. Problems: * The drawing array was exposed through the public API leading to issues where the user count of drawings could get out of sync. E.g. `Layer::add_frame` and `Layer::remove_frame` potentially changes the user count of drawings, but doesn't actually update them (because the layer cannot access the drawings array). * Using drawing indices as a way to reference drawings is extremly fragile. If the drawing array changes in any way, indices become invalid. Solution: * Don't expose the drawing array to the public API. Similar to how the user interacts with grease pencil, drawings are always "attached" to keyframes. * Drawings are always read/created/deleted through the keyframe API: * To create a drawing, use `Drawing *drawing = grease_pencil.insert_frame(layer, ...)`. * To remove drawings, remove the keyframes `grease_pencil.remove_frames(...)`. Drawings with no users (keyframes) get deleted. * `duplicate_keys(...)` to duplicate drawings, `remap_keys(...)`, etc. * Drawings shouldn't be accessed using the `drawings` array. Instead, we use the `get_drawing_at`, `get_editable_drawing_at` API to retrieve drawings on layers at a specific time. Resolves #120221.

Falk David added 4 commits 2024-05-03 17:47:21 +02:00

f7482f136d Remove `drawing_index` parameter in layer frames API

d4d96a8a70 Merge branch 'main' into gpv3-refactor-frames-api

e9e49ac784 Return drawing from `insert_frame`

bd7c866abc Use `insert_frame` in legacy conversion

Falk David added 3 commits 2024-05-03 17:54:02 +02:00

c54d4a5eae Refactor `layer_duplicate_exec`

566b074000 Use `get_editable_drawing_at` in `PaintOperation::on_stroke_done`

9af0387def Use `get_editable_drawing_at` in `GrabOperation::foreach_grabbed_drawing`

Hans Goudey reviewed 2024-05-03 18:19:15 +02:00

Hans Goudey left a comment

I don't really understand the changes to the API. But the assignment operators and move constructor for Drawing look good to me

I don't really understand the changes to the API. But the assignment operators and move constructor for `Drawing` look good to me

source/blender/blenkernel/BKE_grease_pencil.hh

						
				@ -71,6 +71,9 @@ class Drawing : public ::GreasePencilDrawing {

				 public:

				  Drawing();

Hans Goudey commented

2024-05-03 17:50:55 +02:00

Typically the order is to keep assignment after constructors:

  CurvesGeometry();
  CurvesGeometry(int point_num, int curve_num);
  CurvesGeometry(const CurvesGeometry &other);
  CurvesGeometry(CurvesGeometry &&other);
  CurvesGeometry &operator=(const CurvesGeometry &other);
  CurvesGeometry &operator=(CurvesGeometry &&other);
  ~CurvesGeometry();

Typically the order is to keep assignment after constructors: ``` CurvesGeometry(); CurvesGeometry(int point_num, int curve_num); CurvesGeometry(const CurvesGeometry &other); CurvesGeometry(CurvesGeometry &&other); CurvesGeometry &operator=(const CurvesGeometry &other); CurvesGeometry &operator=(CurvesGeometry &&other); ~CurvesGeometry();

filedescriptor marked this conversation as resolved

source/blender/blenkernel/intern/grease_pencil.cc

						
				@ -315,6 +315,39 @@ Drawing::Drawing(const Drawing &other)

				  this->runtime->curve_texture_matrices = other.runtime->curve_texture_matrices;

				}

				Drawing &Drawing::operator=(const Drawing &other)

Hans Goudey commented

2024-05-03 17:51:38 +02:00

Order can change here too

filedescriptor marked this conversation as resolved

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

						
				@ -318,0 +328,4 @@

				Drawing::Drawing(Drawing &&other)

				{

				  this->base.type = other.base.type;

				  other.base.type = 0;

Hans Goudey commented

2024-05-03 17:52:19 +02:00

Would be better to use the enum type here I think

Falk David commented

2024-05-03 18:34:09 +02:00

We're just clearing the data here, so I think using the enum is a bit missleading.

Hans Goudey commented

2024-05-03 18:46:11 +02:00

I don't think it's misleading. The only reason we aren't required by the type system to use the enum type is that DNA is stuck in C rather than C++. If the value of type in the "cleared" Drawing is important, then the enum type should be used. Otherwise the type just shouldn't be reset at all.

I don't think it's misleading. The only reason we aren't required by the type system to use the enum type is that DNA is stuck in C rather than C++. If the value of `type` in the "cleared" `Drawing` is important, then the enum type should be used. Otherwise the type just shouldn't be reset at all.

👍 1

filedescriptor marked this conversation as resolved

Falk David added 1 commit 2024-05-03 18:34:25 +02:00

32a5546afd Change order of constructors

Falk David added 1 commit 2024-05-06 11:21:12 +02:00

5710577017 Use enum value in move constructor

Falk David commented

2024-05-14 15:36:41 +02:00

Most of these changes have been committed as individual commits.

Falk David closed this pull request

2024-05-14 15:36:43 +02:00