Page MenuHome

Code Quality Day Tasks
Confirmed, NormalPublicTO DO

Description

Code Quality Day

https://wiki.blender.org/wiki/Style_Guide/Code_Quality_Day

Large Refactor Projects

Those project will affect a large part of the code base, and likely require several people to work on it.

API Calling Conventions

  • Current mirror API is confusing, using the same function with NULL args and a single char.

eg:

ED_mesh_mirror_spatial_table(ob, NULL, NULL, NULL, 'e'); is used to end mirror.
ED_mesh_mirror_spatial_table(ob, NULL, mesh, vec, 'u'); to lookup an item.
ED_mesh_mirror_spatial_table(ob, em, me_eval, NULL, 's'); is used to start mirroring.

Use multiple functions - _create / _destroy / _lookup.

API Organization

  • gpuPushAttr/gpuPopAttr: Make this part of GPU_state since it's pushing attributes associated with state, use snake-case naming convention.

File Naming

  • Use "utils" instead of "util" (currently both are used 34 util, 60 utils).

eg;

  • ED_util.h
  • path_util.c
  • MOD_util.c

Function Naming

  • Use BKE_material_ naming prefix for BKE_material.h T73589

(init_def_material, test_all_objects_materials, give_current_material_p... etc).


  • Improve names of BLI_path_util API.

Without reading docstrings the purposes of the following functions is unclear.

  • BLI_make_exist
  • BLI_make_existing_file
  • 'BLI_make_file_string'

Propose to use BLI_path_ prefix (TODO, propose new names).


  • Remove uses of the term 'derived' when it doesn't apply to derived-mesh, eg: BKE_object_free_derived_caches BKE_editmesh_free_derivedmesh.

Terminology

  • Remove use of the term 'verify' since it's used ambiguously.
  • Return an item if it exists, otherwise create a new one (use ensure instead of 'verify').

eg:

  • defvert_verify_index -> defvert_ensure_index
  • BKE_image_verify_viewer -> BKE_image_ensure_viewer

Most instances can be changed, although there are some exceptions, eg: RNA_define_verify_sdna could be left as is, or renamed to something more explicit.


  • Avoid use of the term 'validate' since it's used ambiguously,

from dictionary.com "to make valid; substantiate; confirm".

We have examples that...

  • Return false, on invalid data.
  • Raise an error and early-exit on invalid data.
  • Fix the invalid data, returning true - when the data was modified.
  • Correctness check (immutable), (use is_valid instead of 'validate').
  • Make something valid (mutable) (use sanitize instead of 'validate').

For data which stores a flag for it's correctness - a function that updates this could be called 'validate', also cache validation sense, in most cases this is not what it's used for.

Note that this is used in many places over the API, although reading over the current usage confirms that this is used confusingly.


  • Use shorter terms for function names where there is no ambiguity (which are already used in the API).
  • synchronize -> sync
  • initialize -> init
  • attrib -> attr

  • Use _fn as a suffix for callbacks instead of f, cb func,

eg:

  • FileList.checkdirf -> check_dir_fn.
  • PointerPropertyRNA.typef type_fn
  • PointerPropertyRNA.itemf item_fn
  • RenderEngine.update_render_passes_cb -> update_render_passes_fn
  • bNodeType.freeexecfunc -> free_exec_fn

  • Disambiguate term "Line" in BLI_math.h

Currently it's sometimes used for a line-segment (clamped start/end points), other uses extend infinitely in both directions. eg:

closest_to_line_v2 clamps between the two points, where line_point_factor_v3_ex doesn't.

Propose for all clamping versions to use line_segment, or seg in cases where we need to include multiple in a name, eg: isect_seg_seg_v2_point_ex.

Refactoring Files

Documentation

Event Timeline

Dalai Felinto (dfelinto) changed the task status from Needs Triage to Confirmed.Tue, Feb 4, 3:06 PM
Dalai Felinto (dfelinto) created this task.

Not 100% sure, but after reading math_geom.c, I find no issue related to "Disambiguate term "Line" in BLI_math.h"
The example given:

closest_to_line_v2 clamps between the two points, where line_point_factor_v3_ex doesn't.

does not apply here: line_point_factor_v3_ex actually returns a signed factor x such that the projection c of p on the line (l1,l2) can be obtained with c = l1 + x * (l2 - l1), but it's not clamped at all by the function so it is indeed a line that the function acts on.
The functions that clamp the projection are closest_to_line_segment_v and are called by functions dist_*_to_line_segment_v*, so everything is correctly named here.
So maybe I'm missing something, but I think this task can be removed from the description.

Please don't do any mindless search/replace of symbol names or files. Please.

Renaming and refactoring things "just because" does not improve anything - in the ideal case, Blender remains unchanged, in reality there is always the risk of breaking things. Changes like that also cause lots of extra unnecessary work for contributors who maintain branches and patches that haven't been merged to master.

The move to clang-format (which at least brought convenience after it was done) and the huge commits of white space only changes, which literally and intentionally did not change (therefore not improve) Blender in any shape or form already caused me (and I would guess other contributors too) days of work just to update my feature branches and patches. If we now get commits like that every Friday, our productivity is ruined.

Ideally the renaming that occurs is not mindless and "just because", but made to improve the quality of the code - which in turn will make it easier for new and existing developers to understand the code, which will result in a better Blender. Ofc your point of increasing workload for branch/patch maintainers is valid and I can't speak to that, but I don't agree that improving code quality does not equate to improving Blender.

I don't consider renaming "attrib" to "attr" an improvement in code quality, for example. It is however an effective way to annoy developers who are maintaining custom branches and patches.

Meanwhile, I think there are much more meaningful improvements in code quality that could be done instead. Fixing compiler warnings, for example - that could unearth some hidden bugs. Right now the make files for Blender still explicitly turn off a number of compiler warnings globally instead of addressing questionable code.

If in the future, we could build with warnings like -Wswitch, -Wformat or -Wclass-memacces, we will actually prevent future bugs from even happening.