Code quality: Use ID-type callback to get an ID's preview-image
Needs RevisionPublic
Actions

Authored by Julian Eisel (Severin) on Feb 5 2021, 6:13 PM.

Details

Reviewers

Bastien Montagne (mont29)
Sybren A. Stüvel (sybren)

Summary

Having switch-case like checks somewhere in the source code, to check which
ID-types support previews, is generally considered problematic.
Instead, the ID-type (or ID) itself should be able to say: "I support
previews". This localizes ID-type specific code, making it easier to maintain
and preventing issues like this:
https://developer.blender.org/rBfa96aa581192a14

By making this an ID-type callback taking the ID * as parameter, the ID can
do further checks to see if it should actually allow previews, e.g. objects can
check if the object-data type supports previews.

This patch also:

Improves const correctness. The ID_PRV_CASE() macro silently cast away const.
Fixes missing preview reading from external files for brushes & screens.

Diff Detail

Repository

rB Blender

Branch

temp-idtype-preview-getter (branched from master)

Build Status

Buildable 12664
Build 12664: arc lint + arc unit

Event Timeline

Julian Eisel (Severin) created this revision.Feb 5 2021, 6:13 PM

Harbormaster completed remote builds in B12664: Diff 33626.Feb 5 2021, 6:13 PM

Julian Eisel (Severin) requested review of this revision.Feb 5 2021, 6:13 PM

Julian Eisel (Severin) added a project: Pipeline, Assets & I/O.Feb 5 2021, 6:26 PM

Julian Eisel (Severin) added inline comments.

source/blender/blenkernel/BKE_icons.h
147	This const variant is annoying, but I prefer that over non-const-correct code. If we want the passed in `ID ` to be const, we either have to cast away const for the return value, or return a pointer to a `const PreviewImage `. Hence the `PreviewImage const `. (Not sure if that's obvious. Maybe I should add a comment?)
source/blender/blenloader/intern/readblenentry.c
238–246	Note that `ID_BR` and `ID_SCR` (well, should be `ID_SCRN` I guess) are missing here. With this patch they are handled.

Do we actually need the const version at all then? I'd rather not add too much overhead just to respect some strict const-ness, just pass non-const ID pointers around then, and get non-const ** of preview?

Bastien Montagne (mont29) edited projects, added Core; removed Pipeline, Assets & I/O.Feb 16 2021, 6:37 PM

I personally do think that const-correctness is important, especially if we want to port our code base to C++. If we don't have const-correctness in these lower level functions, we won't get it in the higher level ones. I think the overhead of just a little function is neglectable.

Merge branch 'master' into temp-idtype-preview-getter

Harbormaster completed remote builds in B13923: Diff 35947.Apr 7 2021, 5:58 PM

+1 on const-correctness.

LGTM. I think there is a place where some code could be cleaned up (see the note), no need to re-review after that, though.

source/blender/blenloader/intern/readblenentry.c
237–244	This could be encapsulated in its own function, like `bool BKE_previewimg_support(short id_code)`.

This revision is now accepted and ready to land.Apr 8 2021, 2:18 PM

Sybren A. Stüvel (sybren) added inline comments.Apr 8 2021, 2:41 PM

source/blender/blenkernel/intern/icons.cc
362–366	I think this is the right approach. `IDTypeInfo.preview_ptr_get()` should not be modifying the `ID ` it gets, so there it makes total sense to have it receive a `const ID `. As a result, it also has to return a const pointer. This function knows that it has received a non-const `ID *`, so to me this is the right place to cast away the constness of the returned pointer.

To be clear, what I call overhead here is:

Adding extra function to API only to deal with const/non-const cases (this remains C code for now, this is fairly annoying imho).
Adding hard-to-decipher things like * const *;

I also do not understand why you removed const ID pointer from BKE_previewimg_id_get???

In general, I find having struct PreviewImage *const *BKE_previewimg_id_get_p_const(const struct ID *id); in public BKE API extremely ugly and annoying, so I ask again: why do we need this one? Can't we simply do with both BKE_previewimg_id_get_p and BKE_previewimg_id_get, have the IDType callbacks be non-const, and do the const to non-const casting on the ID pointer when calling BKE_previewimg_id_get_p? I see really no reason why the IDType callback should take a const ID as parameter.

Note that you are not strictly const-correct anyway in this patch, since you cast it away in BKE_previewimg_id_get_p...

I also think that const-correctness fix should be split into another patch, since this is fairly different topic than adding a new callback to IDType.

I went away and created an alternative const-fix patch (based on current master, without the new IDType callback) in D10922

source/blender/blenkernel/intern/icons.cc
345	this can be re-written to only get `PreviewImage *` from `old_id`, getting the pointer to pointer on this one is only used to ensure the ID type does support previews...
source/blender/blenloader/intern/readblenentry.c
237–244	Not sure what would be the added value of such function here? it would simply means calling `BKE_idtype_get_info_from_idcode` twice instead of once... In fact having those callbacks in IDType is a much better way to check if previews are supported, compared to what we do in current master code. A `bool BKE_previewimg_support(short id_code)` could be useful in other places though, like in `ed_utils_ops.cc` below.

Bastien Montagne (mont29) requested changes to this revision.Apr 8 2021, 5:05 PM

This revision now requires changes to proceed.Apr 8 2021, 5:05 PM

Sybren A. Stüvel (sybren) added inline comments.Apr 8 2021, 5:26 PM

source/blender/blenloader/intern/readblenentry.c
237–244	The added value is that it places the query "does this ID's type support previews?" in its own function. This to me would make the code much clearer to read, as then such a single query is not spread over two calls (macro + function) and two comparisons. I don't see how this would turn one call to `BKE_idtype_get_info_from_idcode` into two, as `id_type` is only used to answer the question "does this support previews?"