Page MenuHome

Annotations: Use flag to determine if the layer is a Ruler
ClosedPublic

Authored by Antonio Vazquez (antoniov) on Wed, Oct 9, 4:02 PM.

Details

Summary

Proposed fix for T70141.

Before, the ruler was using the name of the layer as key, but this is very weak because if the layer name changes, the layer gets an annotation layer.

Now, the layer is marked using a flag and now it's possible to rename it.

Diff Detail

Repository
rB Blender

Event Timeline

Dalai Felinto (dfelinto) requested changes to this revision.Wed, Oct 9, 5:16 PM

We need a doversion code for this as well, otherwise what happens with old files?

source/blender/editors/space_view3d/view3d_gizmo_ruler.c
411

Nitpick:

- view3d_ruler_get_layer
+ view3d_ruler_layer_get

We tend to have get/set in the end to group functions together with only their suffix changing and to help IDEs autocomplete. Even if it "sounds wrong" from the English point of view.

448

No need to spam flag setting into multiple lines, and your comment really is redundant. A good comment would focus more on the "why" not so much on the "what'.

In this case you covered the "what" but it is really redundant, the one line of code says the same thing here.

My suggestion:

- gpl->flag |= GP_LAYER_HIDE;
- /* Enable ruler flag. */
- gpl->flag |= GP_LAYER_IS_RULER;
+ gpl->flag |= GP_LAYER_HIDE | GP_LAYER_IS_RULER;`
source/blender/makesrna/intern/rna_gpencil.c
1461

Shouldn't this be readonly?

Nitpick, what about is_ruler instead?

This revision now requires changes to proceed.Wed, Oct 9, 5:16 PM
  • Annotations: Fix review problems
source/blender/blenloader/intern/versioning_280.c
1278

Not sure if use this define. I could add the string directly, but as this name is used in other areas I wanted to keep the define to make easier if we need to find where the ruler name is used.

We could move the define into shared place, but not sure where and/or if it worth it.

Some extra suggested changes, but if they get addressed, LGTM

source/blender/blenloader/intern/versioning_280.c
1277

No need to mention the task, git blame will give that already.

1278

doversion can be its own mad house, no need to elegant, shared, ... Honestly, I would probably just add it to the test itself:

- if (STREQ(gpl->info, ruler_name)) {
+ if (STREQ(gpl->info, "RulerData3D")) {
1282

Alternative, to avoid extra nesting by continuing the for loop earlier.

if (gpd == NULL) {
    continue;
}
1285

Am I right that can be only one ruler layer? If so I would expect a break; after we found a match.

source/blender/makesrna/intern/rna_gpencil.c
1463

While we follow "is_*" for rna names, the UI text should be "Ruler" (also change on tooltip):

- RNA_def_property_ui_text(prop, "Is Ruler", "This layer is a special ruler layer");
+ RNA_def_property_ui_text(prop, "Ruler", "This is a special ruler layer");
This revision is now accepted and ready to land.Wed, Oct 9, 8:52 PM
  • Cleanup: Small tweaks