Fix: GPv3: Retrieving drawings for layer returns invalid layer index #120722

Sietse Brouwer · 2024-04-16T22:44:10+02:00

Sietse Brouwer commented

2024-04-16 22:44:10 +02:00

The utility function retrieve_editable_drawings_from_layer() returned
a drawing index instead of a layer index. This PR fixes that.

The utility function `retrieve_editable_drawings_from_layer()` returned a drawing index instead of a layer index. This PR fixes that.

Sietse Brouwer added 1 commit 2024-04-16 22:44:19 +02:00

buildbot/vexp-code-patch-lint Build done. Details

buildbot/vexp-code-patch-linux-x86_64 Build done. Details

buildbot/vexp-code-patch-darwin-x86_64 Build done. Details

buildbot/vexp-code-patch-darwin-arm64 Build done. Details

buildbot/vexp-code-patch-windows-amd64 Build done. Details

buildbot/vexp-code-patch-coordinator Build done. Details

ba260841d0 Use layer index in `retrieve_editable_drawings_from_layer()`

Sietse Brouwer added this to the Grease Pencil project 2024-04-16 22:44:49 +02:00

Sietse Brouwer requested review from Falk David 2024-04-16 22:45:00 +02:00

Pratik Borhade approved these changes 2024-04-17 12:24:16 +02:00

Pratik Borhade left a comment

Hi, looks correct.
Falk is on holidays this week. Fix is quite straightforward so I think we can merge this directly :)

Hi, looks correct. Falk is on holidays this week. Fix is quite straightforward so I think we can merge this directly :)

Pratik Borhade commented

2024-04-17 12:24:32 +02:00

@blender-bot build

Pratik Borhade merged commit 91e50380d3 into main

2024-04-17 13:15:58 +02:00

Pratik Borhade referenced this issue from a commit

2024-04-17 13:15:59 +02:00

Fix: GPv3: Retrieving drawings for layer returns invalid layer index

Matias Mendiola commented

2024-04-17 14:53:06 +02:00

@PratikPB2123 @SietseB please revert this. This change breaks Separate by Layer operator (grease_pencil_separate_layer)
The operator needs all the drawings in a particular layer not the layer index.

I think the function name retrieve_editable_drawings_from_layer is clear. it is not?

@PratikPB2123 @SietseB please revert this. This change breaks Separate by Layer operator (grease_pencil_separate_layer) The operator needs all the drawings in a particular layer not the layer index. I think the function name _retrieve_editable_drawings_from_layer_ is clear. it is not?

Sietse Brouwer commented

2024-04-17 15:47:35 +02:00

@mendio I don't have the rights to revert, we need @PratikPB2123 for that.
But what is the issue in grease_pencil_separate_layer?

The definition of MutableDrawingInfo is this:

struct MutableDrawingInfo {
  bke::greasepencil::Drawing &drawing;
  const int layer_index;
  const int frame_number;
  const float multi_frame_falloff;
};

So the second field should be filled with the layer index. E.g. the Tint tool uses that and crashes when the second field is filled with the drawing index.
If grease_pencil_separate_layer is somehow expecting that drawing_info.layer_index holds a drawing index, it should be fixed in there.

@mendio I don't have the rights to revert, we need @PratikPB2123 for that. But what is the issue in `grease_pencil_separate_layer`? The definition of `MutableDrawingInfo` is this: ``` struct MutableDrawingInfo { bke::greasepencil::Drawing &drawing; const int layer_index; const int frame_number; const float multi_frame_falloff; }; ``` So the second field should be filled with the layer index. E.g. the Tint tool uses that and crashes when the second field is filled with the drawing index. If `grease_pencil_separate_layer` is somehow expecting that `drawing_info.layer_index` holds a drawing index, it should be fixed in there.

Pratik Borhade commented

2024-04-17 17:17:49 +02:00

Hi, I dont understand why we need to revert this. As @SietseB explained, second member of MutableDrawingInfo is layer_index.
On phone right now, as far as I remember grease_pencil_separate_layer did not use layer_index

Hi, I dont understand why we need to revert this. As @SietseB explained, second member of MutableDrawingInfo is layer_index. On phone right now, as far as I remember `grease_pencil_separate_layer` did not use `layer_index`

Matias Mendiola commented

2024-04-17 20:23:56 +02:00

I'll check grease_pencil_separate_layer tomorrow and let you know

I'll check `grease_pencil_separate_layer` tomorrow and let you know

Matias Mendiola commented

2024-04-18 02:19:54 +02:00

You were right, it's not an issue generated by this change. Sorry for the confussion.

I get an assert in all the Separate operators (selected, by layer and by material) but only with drawings automatically converted from GPv2. The Separate operators work well for GPv3 objects created from scratch. I'd need to investigate what causes it

You were right, it's not an issue generated by this change. Sorry for the confussion. I get an assert in all the Separate operators (selected, by layer and by material) but only with drawings automatically converted from GPv2. The Separate operators work well for GPv3 objects created from scratch. I'd need to investigate what causes it

Pratik Borhade commented

2024-04-18 11:54:41 +02:00

No problem. At least we don't have to revert this commit 😃

😆 1

Sietse Brouwer deleted branch gpv3-fix-invalid-layer-index

2024-04-21 21:03:29 +02:00

Falk David commented

2024-04-22 11:46:39 +02:00

I get an assert in all the Separate operators (selected, by layer and by material) but only with drawings automatically converted from GPv2. The Separate operators work well for GPv3 objects created from scratch. I'd need to investigate what causes it

@mendio Could you create a bug report for it? Then we don't loose track of the bug and can investigate together :)

> I get an assert in all the Separate operators (selected, by layer and by material) but only with drawings automatically converted from GPv2. The Separate operators work well for GPv3 objects created from scratch. I'd need to investigate what causes it @mendio Could you create a bug report for it? Then we don't loose track of the bug and can investigate together :)

👍 1

Matias Mendiola commented

2024-04-22 13:11:14 +02:00

Bug report: #120926: GPv3: Blender crash using Separate Operators on converted files from GPv2

Bug report: [#120926: GPv3: Blender crash using Separate Operators on converted files from GPv2](https://projects.blender.org/blender/blender/issues/120926)

👍 1

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

Fix: GPv3: Retrieving drawings for layer returns invalid layer index #120722