Code Quality Day Tasks #73586

Code Quality Day

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

Bigger impact changes

Projects which might go somewhat deeper but which have big impact on Blender code quality.

Extensible Architecture Refactoring

Task: #75724 (Extensible Architecture Refactoring)

Write a script to automatically rename DNA fields

Make it super easy to rename fields in DNA, avoiding manual work to through files and

Suggestion: look into clang-rename tool
Alternative: maybe there is IDE which has good enough refactoring functionality
Once that's done we will improve naming of all ambiguous/confusing DNA names.

Compositor

Design task: #74491

Remove GPU code

Working on: @Blendify

Patch: D7028

 Current GPU support is very limited, causes duplication of node implementation, not friendly for data transfer, does not give measurable speedup in production setups.

 Removing the code makes it easier to refactor the system to be faster and more responsive on CPU.

 GPU integration might get re-introduced later with an upgraded design.

Switch operations from pixel values to relative values

  Makes it very easy to adopt compositing for the display resolution. For example, there is no need to composite full 8K image just to have a preview on a FullHD monitor.

Dependency graph

Consolidate relations and node builders
Move dependency building from depsgraph to ID type

There are multiple possible solutions, but goal is: consolidate nodes/relations building on per-ID basis.

Refactor multilayer storage

Make it more clear and easier to follow design, which will not be dependent on Render.
Note: Needs clear design first.

Physics

Move all physics solvers to simulation/

Render module

Refactor/cleanup in render/:

Baking is split half-way between editors/object and render/. Consolidate in either of them.
Multires baking is likely broken, and can be implemented more accurately with OSD. Check on this and possibly remove.
Refactor shading/texturing (at least make it more clear API names)
Move RenderResult from pipeline.h/.c
~~Move headers files from render/extern/ to render/~~ e15076b22f

User Interface

D7611: Cleanup: split templates into multiple files

Automated Testing

#84999 (Add Operator tests)

Clang-Tidy integration

Code linter, which diagnoses typical programming errors.

More details and coordination in #78535

Smaller isolated changes

"Papercut" type of projects, which might not have that much of a difference on a local scale, but which adds up and make overall experience better.

Split big translation units

Applies to big files, big functions, big if and case statements.
While is not end of a world, makes it hard to jump into area and grasp a bigger picture.

Split f-curves, f-curve modifiers, NLA, drivers, evaluation to more descriptive files than anim.c and anim_sys.c. (being worked on by @dr.sybren).
Split big if, switch stataments in Draw Manager.
Split BKE_boundbox API out of BKE_object (possibly rename BKE_object_boundbox).

Refactor directory structure

Move subdiv and multires out of BKE, into a top-level module.
Consolidate tracking from BKE with Libmv’s C-API in a top level module.
Move sequencer to the top level module (including sequencer, effects, cache).
Use "utils" instead of "util" (Currently both are used 34 util, 60 utils. Example: ED_util.h, path_util.c, MOD_util.c).

Motion tracking

Move Libmv C-API and BKE_tracking to a top-level tracking module.
Move more logic from operators to tracking module.

Finish moving away from DerivedMesh

Rename DerivedMesh.c to something more meaningful in nowadays world.
Remove GPU side of OpenSubdiv in BKE.
Remove uses of the term 'derived' when it doesn't apply to derived-mesh, eg: BKE_object_free_derived_caches BKE_editmesh_free_derivedmesh.

After multires is fully ported to Subdiv:

Remove CCGDerivedMesh and SubdivCCG

Code modernize/readability

Replace list iteration with LISTBASE_FOREACH in C code (e.g., D7320)

DNA: Replace manual offset calculation with offsetof()

Simplifies code
Makes it work on all platforms and memory models
Avoids almost all needs in padding (unless it’s something special like float3 which needs to be padded to float4)

GHOST: Naming

getAllDisplayDimensions currently returns dimensions of the main monitor, should be renamed or made to work as named.

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. See #74506 for proposed new names.

Working on this: @dr.sybren

Split out operating system wrappers (BLI_path_util, BLI_storage) own API

#75516 (Day of Clean Code: Split wrappers for C API functions into own files)

Working on this: @ideasman42

Improve naming of BKE_library/BKE_main

See #72604 (Proposal: BKE_library and BKE_main API naming: prefixes conventions)

#71219 (Refactor out BKE ID copy functions)

Refactor GPU attributes

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

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.

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

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

Working on this: @sebbas

Use shorter commonly used naming

Use shorter terms for function names where there is no ambiguity (which are already used in the API).

synchronize -> sync
initialize -> init
attrib -> attr

Terminology

ibuf_arr → ibufs_arr (or the other way around, but we have both in the same file often, e.g., sequencer.c)

See #74427 (Code Quality: Terminology)

FFmpeg API update

We are using some fields and functions which are declared as deprecated in FFmpeg. Is better to update code to avoid use deprecated fields, as it will minimize frustration when deprecated symbols are actually removed.

The easiest way to see code which needs to be updated is to compile with strict)er) warnings with GCC or CLang. Not sure yet how to see such warnings when using MSVC.

Review task: D10338

Modernize OpenColorIO C-API

It can benefit from using C++11/C++17 features like string_view. Also, can do more proper virtual/override semantic.

#86960 (Fix/Refactor IDPStringProperty code)

Documentation

Document how to add a new property on a high level (DNA, RNA, default values, versioning, preferences, ...).
blender/documentation#73275 (Wiki: Migrating and Integrating Design Documentation)
#70016 (Document performance profiling Blender)
blender/documentation#55361 (Blender development todo list (Parent task))

# Code Quality Day https://wiki.blender.org/wiki/Style_Guide/Code_Quality_Day ## Bigger impact changes Projects which might go somewhat deeper but which have big impact on Blender code quality. ---- **Extensible Architecture Refactoring** Task: #75724 (Extensible Architecture Refactoring) ---- **Write a script to automatically rename DNA fields** Make it super easy to rename fields in DNA, avoiding manual work to through files and Suggestion: look into clang-rename tool Alternative: maybe there is IDE which has good enough refactoring functionality Once that's done we will improve naming of all ambiguous/confusing DNA names. ---- **Compositor** Design task: #74491 * Remove GPU code ``` Working on: @Blendify ``` ``` Patch: D7028 ``` ``` Current GPU support is very limited, causes duplication of node implementation, not friendly for data transfer, does not give measurable speedup in production setups. ``` ``` Removing the code makes it easier to refactor the system to be faster and more responsive on CPU. ``` ``` GPU integration might get re-introduced later with an upgraded design. ``` * Switch operations from pixel values to relative values ``` Makes it very easy to adopt compositing for the display resolution. For example, there is no need to composite full 8K image just to have a preview on a FullHD monitor. ``` ---- **Dependency graph** * Consolidate relations and node builders * Move dependency building from depsgraph to ID type There are multiple possible solutions, but goal is: consolidate nodes/relations building on per-ID basis. ---- **Refactor multilayer storage** Make it more clear and easier to follow design, which will not be dependent on Render. Note: Needs clear design first. ---- **Physics** * Move all physics solvers to `simulation/` ---- **Render module** Refactor/cleanup in `render/`: * Baking is split half-way between editors/object and render/. Consolidate in either of them. * Multires baking is likely broken, and can be implemented more accurately with OSD. Check on this and possibly remove. * Refactor shading/texturing (at least make it more clear API names) * Move RenderResult from `pipeline.h/.c` * ~~Move headers files from `render/extern/` to `render/`~~ e15076b22f ---- **User Interface** * [D7611: Cleanup: split templates into multiple files](https://archive.blender.org/developer/D7611) ---- **Automated Testing** * #84999 (Add Operator tests) ---- **Clang-Tidy integration** Code linter, which diagnoses typical programming errors. More details and coordination in #78535 ---- ## Smaller isolated changes "Papercut" type of projects, which might not have that much of a difference on a local scale, but which adds up and make overall experience better. **Split big translation units** Applies to big files, big functions, big `if` and `case` statements. While is not end of a world, makes it hard to jump into area and grasp a bigger picture. * Split f-curves, f-curve modifiers, NLA, drivers, evaluation to more descriptive files than `anim.c` and `anim_sys.c`. (being worked on by @dr.sybren). * Split big `if`, `switch` stataments in Draw Manager. * Split `BKE_boundbox` API out of `BKE_object` (possibly rename `BKE_object_boundbox`). ---- **Refactor directory structure** * Move subdiv and multires out of BKE, into a top-level module. * Consolidate tracking from BKE with Libmv’s C-API in a top level module. * Move sequencer to the top level module (including sequencer, effects, cache). * Use "utils" instead of "util" (Currently both are used 34 `util`, 60 `utils`. Example: `ED_util.h`, `path_util.c`, `MOD_util.c`). ---- **Motion tracking** * Move Libmv C-API and BKE_tracking to a top-level tracking module. * Move more logic from operators to tracking module. ---- **Finish moving away from DerivedMesh** * Rename DerivedMesh.c to something more meaningful in nowadays world. * Remove GPU side of OpenSubdiv in BKE. * Remove uses of the term 'derived' when it doesn't apply to derived-mesh, eg: `BKE_object_free_derived_caches` `BKE_editmesh_free_derivedmesh`. After multires is fully ported to Subdiv: * Remove CCGDerivedMesh and SubdivCCG ---- **Code modernize/readability** * Replace list iteration with `LISTBASE_FOREACH` in C code (e.g., [D7320](https://archive.blender.org/developer/D7320)) ---- **DNA: Replace manual offset calculation with offsetof()** * Simplifies code * Makes it work on all platforms and memory models * Avoids almost all needs in padding (unless it’s something special like float3 which needs to be padded to float4) ---- **GHOST: Naming** * `getAllDisplayDimensions` currently returns dimensions of the main monitor, should be renamed or made to work as named. ---- **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. See #74506 for proposed new names. Working on this: @dr.sybren ---- **Split out operating system wrappers (`BLI_path_util`, `BLI_storage`) own API** #75516 (Day of Clean Code: Split wrappers for C API functions into own files) Working on this: @ideasman42 ---- **Improve naming of `BKE_library`/`BKE_main`** See #72604 (Proposal: BKE_library and BKE_main API naming: prefixes conventions) ---- #71219 (Refactor out BKE ID copy functions) ---- **Refactor GPU attributes** `gpuPushAttr`/`gpuPopAttr`: Make this part of `GPU_state` since it's pushing attributes associated with state, use snake-case naming convention. ---- **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`. ---- **Use `_fn` as a suffix for callbacks instead of `f`, `cb` `func`** - `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` Working on this: @sebbas ---- **Use shorter commonly used naming** Use shorter terms for function names where there is no ambiguity (which are already used in the API). - `synchronize` -> `sync` - `initialize` -> `init` - `attrib` -> `attr` ---- **Terminology** - `ibuf_arr` → `ibufs_arr` (or the other way around, but we have both in the same file often, e.g., sequencer.c) See #74427 (Code Quality: Terminology) ---- **FFmpeg API update** We are using some fields and functions which are declared as deprecated in FFmpeg. Is better to update code to avoid use deprecated fields, as it will minimize frustration when deprecated symbols are actually removed. The easiest way to see code which needs to be updated is to compile with strict)er) warnings with GCC or CLang. Not sure yet how to see such warnings when using MSVC. Review task: [D10338](https://archive.blender.org/developer/D10338) ---- **Modernize OpenColorIO C-API** It can benefit from using C++11/C++17 features like `string_view`. Also, can do more proper `virtual`/`override` semantic. ---- #86960 (Fix/Refactor IDPStringProperty code) ## Documentation - [ ] Document how to add a new property on a high level (DNA, RNA, default values, versioning, preferences, ...). - [ ] blender/documentation#73275 (Wiki: Migrating and Integrating Design Documentation) - [ ] #70016 (Document performance profiling Blender) - [ ] blender/documentation#55361 (Blender development todo list (Parent task))

Changed status from 'Needs Triage' to: 'Confirmed'

Added subscriber: @dfelinto

Added subscriber: @EAW

Added subscriber: @antoniov

Added subscriber: @Mets

Added subscriber: @JacquesLucke

Added subscriber: @rjg

Added subscriber: @lichtwerk

Added subscriber: @Poulpator

Added subscriber: @c2ba

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.

Added subscriber: @Stefan_Werner

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.

Added subscriber: @fclem

Added subscribers: @ideasman42, @brecht, @Sergey

Moved terminology to own subtask. It deserves to be have more clear and permanent explanation, while tasks here are more like "remove me once done".
Arguably, terminology should be in a Wiki page so it's persistent and available. However, is risking to have a very long document where it will be very difficult to find bit of information you need right now.

Updated description with tasks discussed with @brecht (something we both considered belongs to the quality days). Really hope didn't get anything from @ideasman42 lost :)

@Sergey you added to use message bus or remove it. It shouldn't be half done, yet there is basically no motivation for anyone to spend time here.
Said differently, we're prioritizing user visible stuff but not un-finished back-end work.

Also, this isn't a simple removal, removing will need to have a way for gizmos to know when properties they use change.
So don't think this makes sense to have in code-quality page, it's more general issue that time should be spent to finish replacing notifier system.

Edit, removed this, it's out of scope for a cleanup since it's currently used and removing would require a replacement.

Added subscriber: @JulianEisel

I made some short-term proposals for the UI code, #74429 (UI Code Quality: bContext Management), #74430 (UI Code Quality: Ghost C++ Coding-Style), #74432 (UI Code Quality: General, Smaller Changes). They may need further discussion before being added to this task, I'll let that up to the admins.
There are many other things we could do there, this is just for a start.

Added subscriber: @Jeroen-Bakker

This comment was removed by @Jeroen-Bakker

Added subscriber: @Blendify

@Blendify, i would advice against checkboxes, and instead just do two things:

Once you're working on something, add your name in the "paragraph". For example: Working on: @Blendify.
Remove the entire paragraph once task is done.

Keeps it clear and avoids a long lists of "scratched out" tasks after a while.

Added subscriber: @dr.sybren

Added subscriber: @ankitm

I suggest to add:

Wrap function parameters into structs (where reasonable)
Add structs to wrap arguments passed to callbacks. E.g.:
{icon minus} void view3d_main_region_listener(wmWindow *UNUSED(win), ScrArea *area, ARegion *region, wmNotifier *wmn, const Scene *scene)
{icon plus} void view3d_main_region_listener(wmNotifierRegionListenerParams *)

This makes it easier to add parameters if needed and helps readability.
Also, a caller may pass a derived type with additional parameters, reducing the need for custom-data via void *.

Move all physics solvers to physics/

Are we moving solves to physics/ or simulation/? simulation exists currently but it is confusing with simulation nodes.

Remove GPU code

I think it was decided to put this on hold for now right?

@JulianEisel, sounds good.

@Blendify, indeed some code layout did change since this task was initially created. Best to get input from @JacquesLucke .

Added subscriber: @Raimund58

Added subscriber: @sebbas

Changed status from 'Confirmed' to: 'Archived'

Archiving. See https://lists.blender.org/pipermail/bf-committers/2021-March/050956.html

Modules re encouraged to organize work without a central hub.

Download

What's New

Blender Studio

Manual

Developers Blog

Documentation

Benchmark

Blender Conference

Development Fund

One-time Donations

Code Quality Day Tasks #73586

Code Quality Day

Bigger impact changes

Smaller isolated changes

Documentation