Fix #105152: Removing color attribute doesn't update active #105871

Jorijn de Graaf · 2023-03-18T04:15:32+01:00

Jorijn de Graaf commented

2023-03-18 04:15:32 +01:00

It now gets the index before deleting the attribute,
then sets the attribute at that index as active.

It now gets the index before deleting the attribute, then sets the attribute at that index as active.

Jorijn de Graaf added 2 commits 2023-03-18 04:15:42 +01:00

10ab93a095 Restore active/default color attribute using index

Find the indices for the active and default color attributes, remove the chosen attribute, find the color attributes that go with those indices now, and make them active/default.
This code is not exactly elegant but it does work.
A utility function would be nice, instead of copy pasting the same code several times.
But it's late and this is good enough for a proof of concept.

9ecb895c92 Don't use next_color_attribute

BKE_id_attribute_remove handles this now, so there's no need to do it here.

Jorijn de Graaf requested review from Hans Goudey 2023-03-18 04:17:05 +01:00

Hans Goudey requested changes 2023-03-18 14:51:14 +01:00

Hans Goudey left a comment

Thanks for looking into this. Based on the code, it looks like it's missing moving to the previous active/default index when removing the last color attribute.

Jorijn de Graaf commented

2023-03-18 15:08:43 +01:00

Thanks for looking into this. Based on the code, it looks like it's missing moving to the previous active/default index when removing the last color attribute.

Correct, I didn't get around to implementing that last night.
Just doing max(index, count-1) should do it I think.
No need to check for negative values, because the function I send the index to already checks for that.
I also want to make a utility function to use in the 4 places with copy pasted code.
As well as remove some utility functions that are not used anymore, though perhaps that needs a separate PR.

> Thanks for looking into this. Based on the code, it looks like it's missing moving to the previous active/default index when removing the last color attribute. Correct, I didn't get around to implementing that last night. Just doing `max(index, count-1)` should do it I think. No need to check for negative values, because the function I send the index to already checks for that. I also want to make a utility function to use in the 4 places with copy pasted code. As well as remove some utility functions that are not used anymore, though perhaps that needs a separate PR.

Hans Goudey commented

2023-03-18 15:39:02 +01:00

That sounds great, thanks!

Jorijn de Graaf added 1 commit 2023-03-18 19:14:26 +01:00

395ae6a504 Use utility functions to restore color attributes

I think this is a lot cleaner.
Also the index is clamped now so deleting the last item in the list works too.

Jorijn de Graaf commented

2023-03-18 19:21:19 +01:00

@HooglyBoogly Alright done. This works as it should, but only for meshes; it doesn't affect behavior for Point Cloud or the new Curves type.
I've removed the now unused utility functions in a separate PR #105886.

@HooglyBoogly Alright done. This works as it should, but only for meshes; it doesn't affect behavior for Point Cloud or the new Curves type. I've removed the now unused utility functions in a separate PR #105886.

Jorijn de Graaf changed title from ~~WIP: Fix #105152 Removing a color attribute may leave a mesh without an active color attribute~~ to Fix #105152 Removing a color attribute may leave a mesh without an active color attribute

2023-03-18 19:21:34 +01:00

Jorijn de Graaf requested review from Hans Goudey 2023-03-18 19:21:42 +01:00

Jorijn de Graaf referenced this pull request

2023-03-18 21:31:29 +01:00

WIP: Cleanup: Remove unused color attribute utilities #105886

Hans Goudey requested changes 2023-03-18 22:03:04 +01:00

Hans Goudey left a comment

This works great. This patch should remove next_color_attributes itself to avoid adding warnings, even temporarily. That isn't really a separate IMO cleanup since that implementation is being replaced here.

This works great. This patch should remove `next_color_attributes` itself to avoid adding warnings, even temporarily. That isn't really a separate IMO cleanup since that implementation is being replaced here.

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

						
				@ -441,0 +466,4 @@

				    const bool is_default_color_attribute = name == StringRef(mesh->default_color_attribute);

				    const int active_index = color_name_to_index(id, mesh->active_color_attribute);

				    const int default_index = color_name_to_index(id, mesh->default_color_attribute);

				    if (attributes->remove(name)) {

Hans Goudey commented

2023-03-18 22:02:06 +01:00

I'd suggest flipping the condition here and returning early instead of indenting the rest of the logic.

bonj marked this conversation as resolved

Jorijn de Graaf added 1 commit 2023-03-18 22:44:12 +01:00

f3be034b63 Remove next_color_attribute utilities

These utility functions are no longer used.

Jorijn de Graaf added 1 commit 2023-03-18 22:48:21 +01:00

09d0349c43 Invert attribute remove check and use early return

This way the rest of the code can be tabbed back once.

Jorijn de Graaf added 1 commit 2023-03-18 22:49:51 +01:00

a6712399c0 Merge branch 'main' into fix/remove-color-attribute

Jorijn de Graaf requested review from Hans Goudey 2023-03-18 22:52:48 +01:00

Jorijn de Graaf commented

2023-03-18 22:53:48 +01:00

@HooglyBoogly Okay it uses an early return in both places now, and I've added the cleanup commit.

Hans Goudey commented

2023-03-18 23:27:12 +01:00

Great, the last remaining thing is updating the description/title to match https://wiki.blender.org/wiki/Style_Guide/Commit_Messages

Jorijn de Graaf changed title from ~~Fix #105152 Removing a color attribute may leave a mesh without an active color attribute~~ to Fix #105152 Removing color attribute doesn't update active

2023-03-18 23:47:26 +01:00

Jorijn de Graaf commented

2023-03-19 00:14:08 +01:00

@HooglyBoogly I've changed the title and description to hopefully be suitable.
Sorry it took so long, I got distracted.

@HooglyBoogly I've changed the title and description to hopefully be suitable. Sorry it took so long, I got distracted.

Jorijn de Graaf changed title from ~~Fix #105152 Removing color attribute doesn't update active~~ to Fix #105152: Removing color attribute doesn't update active

2023-03-19 00:15:16 +01:00

Hans Goudey referenced this issue from a commit

2023-03-19 00:57:38 +01:00

Fix #105152: Removing color attribute doesn't update active

Hans Goudey commented

2023-03-19 00:58:39 +01:00

I committed this manually to the 3.5 branch: ea019cf01e

I committed this manually to the 3.5 branch: ea019cf01ee91ae1c09e88baffcfbca10b3be9ee

Hans Goudey closed this pull request

2023-03-19 00:58:40 +01:00

Jorijn de Graaf commented

2023-03-19 01:30:15 +01:00

Oh should I have targeted the 3.5 branch?
Anyway thanks!

Oh should I have targeted the 3.5 branch? Anyway thanks!

Brecht Van Lommel commented

2023-03-19 10:19:55 +01:00

This caused a failure in the operators test:
https://builder.blender.org/admin/#/builders/166/builds/243

@HooglyBoogly if you manually commit something, there is a "Manually merged" option to link the PR to the commit.

This caused a failure in the operators test: https://builder.blender.org/admin/#/builders/166/builds/243 @HooglyBoogly if you manually commit something, there is a "Manually merged" option to link the PR to the commit.

Brecht Van Lommel reopened this pull request

2023-03-19 10:20:02 +01:00

Brecht Van Lommel manually merged commit ea019cf01e into main

2023-03-19 10:28:23 +01:00

Brecht Van Lommel commented

2023-03-19 10:29:36 +01:00

The message is confusing but I just marked this as manually merged, not merged it a second time.

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 #105152: Removing color attribute doesn't update active #105871