GPv3: Fix bugs in find_layer_insertion_index() and some of its usages #113874

Douglas Paul · 2023-10-18T07:50:27+02:00

Douglas Paul commented

2023-10-18 07:50:27 +02:00

The recent PR #113852 fixed crashes by clamping the values returned by find_layer_insertion_index() to ensure they were not out of bounds, however the index being returned was still generally wrong. This would lead to the customdata being reordered incorrectly.

Specifically, the implementation of find_layer_insertion_index() was generally not returning the correct insertion index in the case where the target group was empty (i.e. the latter half of the function). This fixes those issues, as well as some issues in the way some callers were treating the returned index.

The first commit contains only the fixes with minimal restructuring, to make it easier to review the diff. The second commit is some refactoring to reduce redundancy and the risk of handling the insertion index incorrectly.

I included a lot of comments in the code to help explain the logic. I'm not attached to keeping them if they're too verbose.

I also pushed a third commit with copious debug logging to a separate branch. It's useful for local testing, but obviously not intended to be merged: fix-gpv3-layer-reordering-customdata-bugs-with-logging.

The attached .blend file is what I used for most of my testing. I temporarily saved it as my startup file to allow me to quickly start from a blank slate after each test.

The recent PR #113852 fixed crashes by clamping the values returned by find_layer_insertion_index() to ensure they were not out of bounds, however the index being returned was still generally wrong. This would lead to the customdata being reordered incorrectly. Specifically, the implementation of find_layer_insertion_index() was generally not returning the correct insertion index in the case where the target group was empty (i.e. the latter half of the function). This fixes those issues, as well as some issues in the way some callers were treating the returned index. --- The first commit contains only the fixes with minimal restructuring, to make it easier to review the diff. The second commit is some refactoring to reduce redundancy and the risk of handling the insertion index incorrectly. I included a lot of comments in the code to help explain the logic. I'm not attached to keeping them if they're too verbose. I also pushed a third commit with copious debug logging to a separate branch. It's useful for local testing, but obviously not intended to be merged: [fix-gpv3-layer-reordering-customdata-bugs-with-logging](https://projects.blender.org/Douglas-Paul/blender/commits/branch/fix-gpv3-layer-reordering-customdata-bugs-with-logging). The attached `.blend` file is what I used for most of my testing. I temporarily saved it as my startup file to allow me to quickly start from a blank slate after each test.

gpv3-fix-layer-reordering-bugs-testing.blend

847 KiB

Douglas Paul added 2 commits 2023-10-18 07:50:43 +02:00

5497bdff10 GPv3: Fix bugs in find_layer_insertion_index() and some of its usages

135b848cd9 GPv3: Refactor to reduce redundancy & risk

Iliya Katushenock added this to the Grease Pencil project 2023-10-18 08:26:49 +02:00

Falk David requested review from Falk David 2023-10-18 21:39:45 +02:00

Douglas Paul commented

2023-10-19 14:04:16 +02:00

Thanks for adding yourself as a reviewer, @filedescriptor. But FYI, I plan to file a new PR within the next couple days that would supersede this one, because I thought of a different approach for handling the custom data changes that I think will be much simpler to understand and maintain. So just in case you were planning to review this within the next couple days, you may want to hold off for now.

I will leave this PR open as a "fallback" in case my other PR isn't suitable or doesn't work out for some reason.

Thanks for adding yourself as a reviewer, @filedescriptor. But FYI, I plan to file a new PR within the next couple days that would supersede this one, because I thought of a different approach for handling the custom data changes that I think will be much simpler to understand and maintain. So just in case you were planning to review this within the next couple days, you may want to hold off for now. I will leave this PR open as a "fallback" in case my other PR isn't suitable or doesn't work out for some reason.

Falk David commented

2023-10-19 14:05:35 +02:00

@Douglas-Paul Would you mind sharing your idea in text form? Would be good to know about it before you jump into coding!

I already took a peek at this PR and I really like the way you restructured it (+1 for adding helpful comments too).

@Douglas-Paul Would you mind sharing your idea in text form? Would be good to know about it before you jump into coding! I already took a peek at this PR and I really like the way you restructured it (+1 for adding helpful comments too).

Douglas Paul referenced this pull request

2023-10-20 08:45:38 +02:00

GPv3: Reimplement how customdata is updated when layers are re-ordered #113962

Douglas Paul commented

2023-10-20 09:15:31 +02:00

@filedescriptor I ended up jumping right into the coding anyway, because I'm not yet familiar enough with the codebase to have much confidence that an idea will work out. It also wasn't much work in this case.

Anyway, you'll find a description of the idea on the new PR, #113962. 🙂

@filedescriptor I ended up jumping right into the coding anyway, because I'm not yet familiar enough with the codebase to have much confidence that an idea will work out. It also wasn't much work in this case. Anyway, you'll find a description of the idea on the new PR, #113962. 🙂

Falk David commented

2023-10-21 16:48:57 +02:00

f5c9acc154 makes this PR now obsolete. Closing.

f5c9acc154 makes this PR now obsolete. Closing.

Falk David closed this pull request

2023-10-21 16:48:59 +02:00

Pull request closed

This pull request cannot be reopened because the branch was deleted.

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

GPv3: Fix bugs in find_layer_insertion_index() and some of its usages #113874

Pull request closed