Page MenuHome
Differential D11384

Asset Browser Poselib Branch
Needs ReviewPublic
Actions

Authored by Sybren A. Stüvel (sybren) on May 25 2021, 3:00 PM.
This revision needs review, but there are no reviewers specified.

Details

Reviewers
None
Summary

This diff contains all differences between the master and the
asset-browser-poselib branches.

It is not meant for review, just for getting an overview of the
differences.

Diff Detail

Repository
rB Blender
Branch
asset-browser-poselib
Build Status
Buildable 15213
Build 15213: arc lint + arc unit

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
Sybren A. Stüvel (sybren) added inline comments.Jun 15 2021, 10:51 AM
source/blender/editors/armature/armature_intern.h
206–209

The operator is actually used in the pose library Asset View (release/scripts/addons/pose_library/gui.py), to make a distinction between "immediately apply" and "interactively blend". Since it's actually in use, I think it's fien to keep around.

source/blender/editors/asset/asset_edit.cc
84–135

I don't think functionality like this should be in RNA. IMO it should be part of the asset system, so either stay here or even be part of BKE.

89

This should declare the enum type as return type, and not just int. If that's impossible, it should at least document this clearly.

The _to_enum_value suffix isn't the most descriptive either. Maybe _get_library_type?

108

Same issue as above with _from_enum_value and having int as the only indication of the enum type.

source/blender/editors/asset/asset_list.cc
315–318

This is just return ELEM(notifier.action, NA_RENAME);

321–327

This is just:

return ELEM(notifier.data, ND_ASSET_LIST, ND_ASSET_LIST_READING, ND_ASSET_LIST_PREVIEW) ||
  ELEM(notifier.action, NA_ADDED, NA_REMOVED, NA_EDITED);

I hope Clang-Format would spread that over multiple lines in a readable way, though (like above). If not, I prefer the current code.

443–444

Shouldn't this handle all cases where library_reference.type >= ASSET_LIBRARY_CUSTOM? If so, I think it's cleaner to have some function is_custom_library(library_reference) somwhere, such that the concept of "anything over ASSET_LIBRARY_CUSTOM is a custom library" doesn't have to be copied all over the place. Come to think of it, I think such a function is a good idea anyway ;-)

475–477

I think that sentence misses a "++" somewhere.

In any case, I would just remove the C API if it's not in use. It's quite a bit of code, and if it sits unused it'll rot.

source/blender/editors/asset/asset_ops.cc
257

Does this work in the Asset View as well as the Asset Browser?

source/blender/editors/include/UI_interface.h
2261–2263

That could work, but only if the "activate operator" is executed on activation, and invoked on drag. This is currently not the case (you can test by changing activate_operator="poselib.apply_pose_asset" → "poselib.blend_pose_asset" in VIEW3D_PT_pose_library).

Sybren A. Stüvel (sybren) updated this revision to Diff 38323.Jun 15 2021, 11:19 AM
Sybren A. Stüvel (sybren) marked 2 inline comments as done.
  • Cleanup: Move version patch to correct file
  • Cleanup: Remove unnecessary includes
  • Cleanup: Minor code-style cleanup
  • Cleanup: Move temporary ID consumer class for assets into own file
  • Merge remote-tracking branch 'origin/master' into asset-browser-poselib
  • Cleanup: assets, remove unused code
  • Cleanup: pose library, use LISTBASE_FOREACH_MUTABLE macro
  • Merge remote-tracking branch 'origin/master' into asset-browser-poselib
Harbormaster completed remote builds in B15197: Diff 38323.Jun 15 2021, 11:19 AM
Sybren A. Stüvel (sybren) added a comment.Jun 15 2021, 5:00 PM

By now I've gone over all files. The interface_... files are not my cup of tea, so I've only glanced over them.

source/blender/blenloader/intern/versioning_300.c
25

File LGTM

source/blender/editors/include/ED_asset.h
87

Would that be something like ED_asset_list.hh?

source/blender/editors/include/ED_fileselect.h
139–141

These changes come from rB063c248cb706, and from the look of it, the non-asset-specific stuff could go directly into master AFAIC.

source/blender/editors/include/UI_interface_icons.h
108–112

Maybe then also change const bool to bool? bool is passed by value, so const in the declaration has no meaning.

source/blender/editors/interface/interface.c
126

File LGTM

source/blender/editors/interface/interface_context_menu.c
23

Why is stdio.h needed here? I don't see it being required by the other changes in the file.

source/blender/editors/interface/interface_handlers.c
8790–8791

Does this describe the workings of the function? Or is it a note that should be marked with a TODO?

source/blender/editors/interface/interface_intern.h
932–948

Why are these functions marked extern? According to these two random search hits (one, two) all functions are implicitly marked as extern, and adding the keyword explicitly has no meaning.

source/blender/editors/interface/interface_ops.c
83–88

This all sounds rather dodgy. But, at least the dodgyness has been moved from somewhere-in-the-middle-of-some-function to a function by itself, and described in a appropriately-dodgy way, so I guess it's a step forward!

source/blender/editors/interface/interface_template_asset_view.cc
58–59

I'd just add a return; in the if-block, remove the else, and unindent the rest of the function.

100–101

const?

source/blender/editors/space_api/spacetypes.c
124

LGTM

source/blender/editors/space_file/file_draw.c
43

File LGTM

source/blender/editors/space_file/file_intern.h
108

File LGTM

source/blender/editors/space_file/file_ops.c
1877

File LGTM

source/blender/editors/space_file/filelist.c
330

File LGTM except where noted

338

This should get a comment to explain what it means.

1055

Same issue as earlier; library_a->type > ASSET_LIBRARY_CUSTOM is also an indication it's a custom library.

If this is not the case here, but is the case in other uses of exactly the same enum values, this should be redesigned.

2095

This should be abstracted away into some filelist_entry_has_uuid(entry, uuid) function.

2113–2116

This still doesn't make sense to me. It really should have an explanation as to why it's ok to leave r_uuid[1..3] untouched.

2126

What guarantees that some_ID->session_uuid != FILE_UUID_UNSET?

2509

Couldn't this also mean that there are no file list entries, and thus that the caching of the previews is actually trivially done?

3230–3239

This really belongs in its own function.

3363

The "uuid" naming is getting messed up. So far these are all used interchangably:

FileListIntern:            char[16]
filelist_uuid_from_id():   uint32_t[4]
ID:                        unsigned int
filelist_uuid_from_uint(): uint32_t[4]   but only the first element

This also relates to D7007: Add a session-wise uuid integer to IDs. introducing something that's not a UUID but still calling it one. I've raised an issue about this in the #core-module channel on Blender Chat, as this goes beyond the scope of this patch.

3494

const

source/blender/editors/space_file/filelist.h
72

File LGTM

source/blender/editors/space_file/filesel.c
1281–1283

This looks like it should be a separate function filelist_file_find(sfile->files, params) (see "G14: Feature Envy" in Clean Code).

source/blender/editors/space_node/node_intern.h
45 ↗(On Diff #38323)

These two structs don't seem to be required (no other changes to this file), so I'll remove them.

source/blender/editors/undo/ed_undo.c
53

File LGTM

source/blender/editors/util/ed_util.c
41

File LGTM

source/blender/editors/util/ed_util_ops.cc
23

File LGTM except where noted.

31

Some of the added includes are no longer necessary, I'll remove them.

source/blender/makesdna/DNA_asset_defaults.h
27

LGTM

source/blender/makesdna/DNA_asset_types.h
15

File LGTM except where noted.

source/blender/makesdna/DNA_screen_types.h
38

File LGTM

source/blender/makesdna/DNA_space_types.h
879

This is nasty, as it'll actually be stored in a uint32_t. Either use 4294967295L or ((uint32_t)-1).

source/blender/makesdna/DNA_workspace_types.h
18

File LGTM

source/blender/makesdna/intern/dna_defaults.c
147

File LGTM

source/blender/makesrna/RNA_access.h
66

File LGTM

471

File LGTM

source/blender/makesrna/RNA_enum_types.h
248–250

"this version" is a bit ambiguous. Do you mean "this struct"?

source/blender/makesrna/intern/makesrna.c
4378

Document why it should go after spaces.

source/blender/makesrna/intern/rna_ID.c
121

File LGTM except where noted

130

Add a comma after not EnumPropertyItem, otherwise strictly speaking the sentence means pretty much the opposite of what you intend it to mean.

source/blender/makesrna/intern/rna_asset.c
20

File LGTM except where noted

142

Remove the comment, or add another comment that explains why it should be kept ;-)

379

Remove it, see what breaks?

source/blender/makesrna/intern/rna_context.c
300–301

Can't we return an instance and let the Python GC handle its destruction? Honest question, I don't know if this is possible with RNA.

source/blender/makesrna/intern/rna_internal.h
25

File LGTM

source/blender/makesrna/intern/rna_palette.c
132

Is this still necessary?

source/blender/makesrna/intern/rna_pose_api.c
112

File LGTM

source/blender/makesrna/intern/rna_space.c
2564

Same question as before as to valid library types & having a separate function for these kind of comparisons.

2573

Same as before: delegate to different function; same for similar code below.

source/blender/makesrna/intern/rna_ui.c
1564–1565

Why is this? Why not just pass the full list ID to Python?

source/blender/makesrna/intern/rna_ui_api.c
537–542

I suspect these can all become const

source/blender/makesrna/intern/rna_workspace.c
102

File LGTM

source/blender/windowmanager/WM_api.h
612

File LGTM

source/blender/windowmanager/WM_types.h
442–443

There are three values here, but only one description. Which value does it map to? And what do the others mean?

source/blender/windowmanager/intern/wm_event_system.c
56

File LGTM

source/blender/windowmanager/intern/wm_init_exit.c
104

File LGTM

source/blender/windowmanager/intern/wm_operators.c
599

File LGTM

source/blender/windowmanager/intern/wm_uilist_type.c
16

File LGTM

Sybren A. Stüvel (sybren) marked 7 inline comments as not done.Jun 15 2021, 5:05 PM

Because this is my patch, all my notes are auto-marked as "done" by Phabricator. I've un-marked those that still need doing (or not, but then explicitly marked as "done").

Sybren A. Stüvel (sybren) updated this revision to Diff 38346.Jun 15 2021, 5:18 PM
  • Cleanup: make struct PoseBackup an opaque struct
  • Cleanup: remove byte order mark
  • Cleanup: remove unused include statements
  • Merge remote-tracking branch 'origin/master' into asset-browser-poselib
Harbormaster completed remote builds in B15213: Diff 38346.Jun 15 2021, 5:18 PM
Sybren A. Stüvel (sybren) marked 21 inline comments as done and 2 inline comments as done.Jun 18 2021, 10:14 AM

Marked notes that don't indicate a "todo" as "done"

Sybren A. Stüvel (sybren) updated this revision to Diff 38896.Jun 29 2021, 3:33 PM
  • Merge remote-tracking branch 'origin/master' into asset-browser-poselib
Harbormaster completed remote builds in B15489: Diff 38896.Jun 29 2021, 3:33 PM
Sybren A. Stüvel (sybren) updated this revision to Diff 38898.Jun 29 2021, 4:08 PM
Sybren A. Stüvel (sybren) marked 5 inline comments as done.

I've gone over my own remarks, and handled the tivial ones (const here, move some stuff there, etc.)

  • Cleanup: CTX_wm_asset_handle, move assignment of return param
  • Cleanup: remove unused #include
  • Cleanup: reduce amount of conditional code
  • Cleanup: add const to local variables and function parameters
  • Cleanup: typo fix
Harbormaster completed remote builds in B15491: Diff 38898.Jun 29 2021, 4:08 PM
Sybren A. Stüvel (sybren) added inline comments.Jun 29 2021, 4:11 PM
source/blender/editors/include/UI_interface_icons.h
108–112

meh, out of scope for this patch, and can always happen in some clean-up.

Sybren A. Stüvel (sybren) updated this revision to Diff 38999.Jul 1 2021, 11:01 AM
  • Merge remote-tracking branch 'origin/master' into asset-browser-poselib
Harbormaster completed remote builds in B15535: Diff 38999.Jul 1 2021, 11:01 AM
Sybren A. Stüvel (sybren) added inline comments.Jul 1 2021, 12:28 PM
source/blender/editors/space_file/filesel.c
1281

const

Sybren A. Stüvel (sybren) added inline comments.Jul 1 2021, 12:35 PM
source/blender/editors/space_file/filelist.c
2120–2121

filelist_uuid_unset() only sets the first element of the UUID array, but memcmp() compares all elements. This is unacceptable, as it relies on undefined behaviour of the initialisation of arrays.

Revision Contents
Changeset List

PathSize
3 lines
build_files/
config/
3 lines
utils/
3 lines
release/
scripts/
modules/
bpy_extras/
11 lines
presets/
keyconfig/
keymap_data/
2 lines
startup/
bl_operators/
83 lines
bl_ui/
24 lines
27 lines
source/
blender/
blenkernel/
6 lines
15 lines
3 lines
6 lines
6 lines
intern/
119 lines
57 lines
5 lines
1 line
31 lines
20 lines
10 lines
blenloader/
intern/
11 lines
1 line
editors/
armature/
3 lines
4 lines
2 lines
139 lines
638 lines
asset/
5 lines
88 lines
637 lines
33 lines
113 lines
include/
16 lines
64 lines
11 lines
62 lines
5 lines
interface/
1 line
44 lines
17 lines
275 lines
2 lines
54 lines
92 lines
2 lines
113 lines
8 lines
278 lines
4 lines
991 lines
96 lines
73 lines
render/
55 lines
screen/
8 lines
22 lines
space_api/
2 lines
space_file/
18 lines
3 lines
25 lines
17 lines
117 lines
78 lines
47 lines
space_node/
16 lines
3 lines
undo/
3 lines
util/
3 lines
3 lines
makesdna/
7 lines
55 lines
24 lines
27 lines
5 lines
intern/
2 lines
makesrna/
4 lines
16 lines
intern/
2 lines
91 lines
205 lines
27 lines
11 lines
5 lines
2 lines
280 lines
35 lines
182 lines
20 lines
windowmanager/
7 lines
7 lines
intern/
2 lines
5 lines
6 lines
94 lines

Diff 38346

View Options

.gitmodules

Loading...
View Options

build_files/config/pipeline_config.json

Loading...
View Options

build_files/utils/make_update.py

Loading...
View Options

release/scripts/modules/bpy_extras/asset_utils.py

Loading...
View Options

release/scripts/presets/keyconfig/keymap_data/blender_default.py

Loading...
View Options

release/scripts/startup/bl_operators/assets.py

Loading...
View Options

release/scripts/startup/bl_ui/__init__.py

Loading...
View Options

release/scripts/startup/bl_ui/space_filebrowser.py

Loading...
View Options

source/blender/blenkernel/BKE_animsys.h

Loading...
View Options

source/blender/blenkernel/BKE_armature.h

Loading...
View Options

source/blender/blenkernel/BKE_asset.h

Loading...
View Options

source/blender/blenkernel/BKE_context.h

Loading...
View Options

source/blender/blenkernel/BKE_screen.h

Loading...
View Options

source/blender/blenkernel/intern/anim_sys.c

Loading...
View Options

source/blender/blenkernel/intern/armature_pose.cc

Loading...
View Options

source/blender/blenkernel/intern/asset.cc

Loading...
View Options

source/blender/blenkernel/intern/blendfile.c

Loading...
View Options

source/blender/blenkernel/intern/context.c

Loading...
View Options

source/blender/blenkernel/intern/screen.c

Loading...
View Options

source/blender/blenkernel/intern/workspace.c

Loading...
View Options

source/blender/blenloader/intern/versioning_300.c

Loading...
View Options

source/blender/blenloader/intern/versioning_userdef.c

Loading...
View Options

source/blender/editors/armature/CMakeLists.txt

Loading...
View Options

source/blender/editors/armature/armature_intern.h

Loading...
View Options

source/blender/editors/armature/armature_ops.c

Loading...
View Options

source/blender/editors/armature/pose_backup.c

Loading...
View Options

source/blender/editors/armature/pose_lib_2.c

Loading...
View Options

source/blender/editors/asset/CMakeLists.txt

Loading...
View Options

source/blender/editors/asset/asset_edit.cc

Loading...
View Options

source/blender/editors/asset/asset_list.cc

Loading...
View Options

source/blender/editors/asset/asset_ops.cc

Loading...
View Options

source/blender/editors/asset/asset_temp_id_consumer.cc

Loading...
View Options

source/blender/editors/include/ED_armature.h

Loading...
View Options

source/blender/editors/include/ED_asset.h

Loading...
View Options

source/blender/editors/include/ED_fileselect.h

Loading...
View Options

source/blender/editors/include/UI_interface.h

Loading...
View Options

source/blender/editors/include/UI_interface_icons.h

Loading...
View Options

source/blender/editors/interface/CMakeLists.txt

Loading...
View Options

source/blender/editors/interface/interface.c

Loading...
View Options

source/blender/editors/interface/interface_context_menu.c

Loading...
View Options

source/blender/editors/interface/interface_handlers.c

Loading...
View Options

source/blender/editors/interface/interface_icons.c

Loading...
View Options

source/blender/editors/interface/interface_intern.h

Loading...
View Options

source/blender/editors/interface/interface_ops.c

Loading...
View Options

source/blender/editors/interface/interface_panel.c

Loading...
View Options

source/blender/editors/interface/interface_query.c

Loading...
View Options

source/blender/editors/interface/interface_region_search.c

Loading...
View Options

source/blender/editors/interface/interface_template_asset_view.cc

Loading...
View Options

source/blender/editors/interface/interface_template_search_menu.c

Loading...
View Options

source/blender/editors/interface/interface_templates.c

Loading...
View Options

source/blender/editors/interface/interface_utils.c

Loading...
View Options

source/blender/editors/interface/interface_widgets.c

Loading...
View Options

source/blender/editors/render/render_preview.c

Loading...
View Options

source/blender/editors/screen/area.c

Loading...
View Options

source/blender/editors/screen/screen_context.c

Loading...
View Options

source/blender/editors/space_api/spacetypes.c

Loading...
View Options

source/blender/editors/space_file/file_draw.c

Loading...
View Options

source/blender/editors/space_file/file_intern.h

Loading...
View Options

source/blender/editors/space_file/file_ops.c

Loading...
View Options

source/blender/editors/space_file/filelist.h

Loading...
View Options

source/blender/editors/space_file/filelist.c

Loading...
View Options

source/blender/editors/space_file/filesel.c

Loading...
View Options

source/blender/editors/space_file/space_file.c

Loading...
View Options

source/blender/editors/space_node/drawnode.cc

Loading...
View Options

source/blender/editors/space_node/node_buttons.c

Loading...
View Options

source/blender/editors/undo/ed_undo.c

Loading...
View Options

source/blender/editors/util/ed_util.c

Loading...
View Options

source/blender/editors/util/ed_util_ops.cc

Loading...
View Options

source/blender/makesdna/DNA_asset_defaults.h

Loading...
View Options

source/blender/makesdna/DNA_asset_types.h

Loading...
View Options

source/blender/makesdna/DNA_screen_types.h

Loading...
View Options

source/blender/makesdna/DNA_space_types.h

Loading...
View Options

source/blender/makesdna/DNA_workspace_types.h

Loading...
View Options

source/blender/makesdna/intern/dna_defaults.c

Loading...
View Options

source/blender/makesrna/RNA_access.h

Loading...
View Options

source/blender/makesrna/RNA_enum_types.h

Loading...
View Options

source/blender/makesrna/intern/makesrna.c

Loading...
View Options

source/blender/makesrna/intern/rna_ID.c

Loading...
View Options

source/blender/makesrna/intern/rna_asset.c

Loading...
View Options

source/blender/makesrna/intern/rna_context.c

Loading...
View Options

source/blender/makesrna/intern/rna_internal.h

Loading...
View Options

source/blender/makesrna/intern/rna_palette.c

Loading...
View Options

source/blender/makesrna/intern/rna_pose_api.c

Loading...
View Options

source/blender/makesrna/intern/rna_space.c

Loading...
View Options

source/blender/makesrna/intern/rna_ui.c

Loading...
View Options

source/blender/makesrna/intern/rna_ui_api.c

Loading...
View Options

source/blender/makesrna/intern/rna_workspace.c

Loading...
View Options

source/blender/windowmanager/WM_api.h

Loading...
View Options

source/blender/windowmanager/WM_types.h

Loading...
View Options

source/blender/windowmanager/intern/wm_event_system.c

Loading...
View Options

source/blender/windowmanager/intern/wm_init_exit.c

Loading...
View Options

source/blender/windowmanager/intern/wm_operators.c

Loading...
View Options

source/blender/windowmanager/intern/wm_uilist_type.c

Loading...