Page MenuHome

Code Quality Day Tasks
Confirmed, NormalPublicTO DO

Description

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: T75724: 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: T74491

  • Remove GPU code

    Working on: @Aaron Carlisle (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/ rBe15076b22f4f

User Interface


Automated Testing

*T84999: Add Operator tests


Clang-Tidy integration

Code linter, which diagnoses typical programming errors.

More details and coordination in T78535


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 @Sybren A. Stüvel (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 T74506 for proposed new names.

Working on this: @Sybren A. Stüvel (sybren)


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

T75516: Day of Clean Code: Split wrappers for C API functions into own files

Working on this: @Campbell Barton (campbellbarton)


Improve naming of BKE_library/BKE_main

See T72604: Proposal: BKE_library and BKE_main API naming: prefixes conventions


T71219: 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

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_arribufs_arr (or the other way around, but we have both in the same file often, e.g., sequencer.c)

See T74427: 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.

Documentation

Related Objects

StatusSubtypeAssignedTask
ConfirmedTO DONone
ResolvedTO DOAntonio Vazquez (antoniov)
ConfirmedDESIGNNone

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

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.

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 Van Lommel (brecht) (something we both considered belongs to the quality days). Really hope didn't get anything from @Campbell Barton (campbellbarton) lost :)

@Sergey Sharybin (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.

I made some short-term proposals for the UI code, T74429: UI Code Quality: bContext Management, T74430: UI Code Quality: Ghost C++ Coding-Style, T74432: 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.

This comment was removed by Jeroen Bakker (jbakker).

@Aaron Carlisle (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.

I suggest to add:

Wrap function parameters into structs (where reasonable)
Add structs to wrap arguments passed to callbacks. E.g.:
void view3d_main_region_listener(wmWindow *UNUSED(win), ScrArea *area, ARegion *region, wmNotifier *wmn, const Scene *scene)
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?

@Julian Eisel (Severin), sounds good.

@Aaron Carlisle (Blendify), indeed some code layout did change since this task was initially created. Best to get input from @Jacques Lucke (JacquesLucke) .