Page MenuHome
Differential D14995

Refactor: Add CustomData_set_layer_name_index(CD, index , newname) to have a single place to do everything needed on layer renames.
AbandonedPublic
Actions

Authored by Martijn Versteegh (Baardaap) on May 20 2022, 12:02 PM.

Details

Reviewers
Hans Goudey (HooglyBoogly)
Summary

No functional changes.

This is also in preparation to possible renaming of associated data layers in the future.

This changes the functionality of CustomData_set_layer_name(), because it now calls
CustomData_set_unique_name() on every name change, while that function didn't do
that before. However at the current moment CustomData_set_layer_name() is used only
once, in versioning_270.c, to correct a faulty CD_MDEFORMVERT layer. As
CustomData_set_unique_name() ignores CD_MDEFORMVERT layers because they don't
have a LayerTypeInfo.defaultname this is no problem.

Diff Detail

Repository
rB Blender
Branch
arcpatch-D14995 (branched from master)
Build Status
Buildable 22407
Build 22407: arc lint + arc unit

Event Timeline

Martijn Versteegh (Baardaap) requested review of this revision.May 20 2022, 12:02 PM
Martijn Versteegh (Baardaap) created this revision.
Harbormaster completed remote builds in B22178: Diff 51655.May 20 2022, 12:02 PM
Martijn Versteegh (Baardaap) added a reviewer: Hans Goudey (HooglyBoogly).May 20 2022, 12:03 PM
Martijn Versteegh (Baardaap) added a comment.May 20 2022, 12:07 PM

I have a small doubt about how to handle the existing CustomData_set_layer_name(CD, type, n, newname).

  • On the one hand I think it would be more consistent to make that function call back to the new CustomData_set_layer_name_index().
  • On the other hand this might/would (can't completely oversee) change the behaviour in the single place it is used, versioning_270.c , where it is used to set the name of some layer to an empty string. Making it use the new function would cause CustomData_set_layer_unique_name() to be called on that layer as well, which sets a default name if an empty string is specified. I don't really understand (yet) if this would be bad.
Martijn Versteegh (Baardaap) updated this revision to Diff 51766.May 23 2022, 9:59 PM

Make CustomData_set_layer_name() also use the new CustomData_set_layer_name_index().

Not doing so would lead to there *still* being multiple paths to rename
layers. The *might* influence the only place this functions was called up
till now: versioning_270.c , fix for T51520 where a faulty layer name is
cleared. Now CustomData_set_layer_unique_name() gets called on the empty
layer. Which I *think* is ok, but I'm not sure.

Harbormaster completed remote builds in B22226: Diff 51766.May 23 2022, 9:59 PM
Martijn Versteegh (Baardaap) retitled this revision from Add CustomData_set_layer_name_index(CD, index , newname) to have a single place to do everything needed on layer renames. to Refactor: Add CustomData_set_layer_name_index(CD, index , newname) to have a single place to do everything needed on layer renames..May 24 2022, 9:44 AM
Martijn Versteegh (Baardaap) edited the summary of this revision. (Show Details)
Martijn Versteegh (Baardaap) updated this revision to Diff 51782.May 24 2022, 10:34 AM
  • Remove incorrect const.
Harbormaster completed remote builds in B22237: Diff 51782.May 24 2022, 10:35 AM
Hans Goudey (HooglyBoogly) added a comment.May 29 2022, 11:09 AM

I think this refactor makes sense. For a proper attribute API, I think the function would just have an old name and new name argument, but that doesn't make sense for CustomData.

However, I think the current use of the function in versioning code probably shouldn't go through this code path, in case it's changed further in the future.
Since the versioning code has access to all the struct definitions, it could just find the layer and write to the name directly.

source/blender/blenkernel/BKE_customdata.h
403

Given the second sentence, I'm not sure what "Unconditionally" means here.

Martijn Versteegh (Baardaap) added a comment.May 29 2022, 9:52 PM

Like it is now the versioning code doesn't mind the new codepath, since it has no effect on the type of layer in question.

But maybe letting it write directly to the name would be better indeed, just to be on the safe side.

source/blender/blenkernel/BKE_customdata.h
403

I mean it can't fail because the layer doesn't exist or something. No error checking is done on the layer existing or not.

That's maybe badly worded I'll try to think of a better way.

Martijn Versteegh (Baardaap) updated this revision to Diff 52132.Jun 1 2022, 7:35 PM
  • Remove confusing 'unconditionally'.
  • Let the versioningcode directly write to the layername and not use CustomData_set_layer_name()
  • Merge branch 'master' into singlelayerrename
Harbormaster completed remote builds in B22374: Diff 52132.Jun 1 2022, 7:35 PM
Martijn Versteegh (Baardaap) marked an inline comment as done.Jun 1 2022, 7:40 PM

I changed the versioning code to just directly clear the name, without going through CustomData_set_layer_name()

Hans Goudey (HooglyBoogly) added a comment.Jun 2 2022, 10:42 AM

Looks better to me, thanks! Just a small comment

source/blender/blenloader/intern/versioning_270.c
1603

Shouldn't we check if the index is -1 here?

Martijn Versteegh (Baardaap) updated this revision to Diff 52156.EditedJun 2 2022, 10:52 AM
  • Check index is not -1 and add assert.
Harbormaster completed remote builds in B22386: Diff 52156.Jun 2 2022, 10:52 AM
Martijn Versteegh (Baardaap) marked an inline comment as done.Jun 2 2022, 10:53 AM
Martijn Versteegh (Baardaap) added inline comments.
source/blender/blenloader/intern/versioning_270.c
1603

whoops! I think you're right.

Hans Goudey (HooglyBoogly) accepted this revision.Jun 2 2022, 10:55 AM

Great!

source/blender/blenloader/intern/versioning_270.c
1602

I'll make this const int when committing :)

This revision is now accepted and ready to land.Jun 2 2022, 10:55 AM
Martijn Versteegh (Baardaap) updated this revision to Diff 52157.Jun 2 2022, 11:07 AM
Martijn Versteegh (Baardaap) marked an inline comment as done.
  • add const
Harbormaster completed remote builds in B22387: Diff 52157.Jun 2 2022, 11:07 AM
Martijn Versteegh (Baardaap) updated this revision to Diff 52206.Jun 3 2022, 12:04 PM
Martijn Versteegh (Baardaap) marked an inline comment as done.
  • Fix wrong assert.
Harbormaster completed remote builds in B22407: Diff 52206.Jun 3 2022, 12:04 PM
Martijn Versteegh (Baardaap) abandoned this revision.Jun 7 2022, 12:48 PM

As discussed with @Hans Goudey (HooglyBoogly) , the current renaming doesn't take into account cross-domain name uniqueness. Because fixing that would mean rewriting this again it's better to fix that problem at the same time.

source/blender/blenloader/intern/versioning_270.c
1602

added it anyway ;-)

Revision Contents
Changeset List

PathSize
source/
blender/
blenkernel/
8 lines
intern/
3 lines
11 lines
blenloader/
intern/
7 lines
makesrna/
intern/
3 lines

Diff 52206

View Options

source/blender/blenkernel/BKE_customdata.h

Loading...
View Options

source/blender/blenkernel/intern/attribute.cc

Loading...
View Options

source/blender/blenkernel/intern/customdata.cc

Loading...
View Options

source/blender/blenloader/intern/versioning_270.c

Loading...
View Options

source/blender/makesrna/intern/rna_mesh.c

Loading...