Page MenuHome

Refactor readfile's liblink code.
ClosedPublic

Authored by Bastien Montagne (mont29) on Jan 31 2020, 8:02 PM.

Details

Summary

Liblink specific ID type function was so far running a loop over all IDs
of relevant type, unlike almost any other 'ID-callback-like' functions
in Blender, which usually let the looping controll to calling code.

The latter approach is more convinient when one want to add generic
(i.e. type-agnostic) code, since it typically only has to change code in
one place (caller function) instead of tens of places (all the callback
functions).

This commit also changes/sanitizes a few things that had nothing to do
in main liblink code, like mesh conversion from tessfaces to polys
(which can be done in after-linking versionning code), or scenes' cycles
detection/check regarding background 'set' scenes.

Diff Detail

Repository
rB Blender

Event Timeline

Cleanup stupid clang-format failure.

Fix stupid mess with ID types order in liblink.

Some comments about non-trivial changes in this patch. Overall, think it should be fairly straight-forward though.

@Campbell Barton (campbellbarton) added you as reviewer mainly so that you can double-check the change to mesh (moving tessface -> polys conversion from liblinking to versioning code).

source/blender/blenloader/intern/readfile.c
3384–3386

This is simpler to just pass 'owner library' since that's what’s needed here, also make it in-line with Scene's master_collection handling.

4927–4951

Now that we have after-liblink doversion code, moved that whole part there.

9797–9798

Note that this being ran before FreestyleLineStyle IDs was therefore a bug, which is fixed now by using default standard iteration order of IDs (which does put linestyles before nodetrees).

source/blender/makesdna/DNA_scene_types.h
2158

Adding this to not abuse LIB_TAG_NEED_LINK for a very type-specific after-linking check in readflie liblinking code.

Brecht Van Lommel (brecht) requested changes to this revision.Jan 31 2020, 11:55 PM
Brecht Van Lommel (brecht) added inline comments.
source/blender/blenloader/intern/readfile.c
6503

This is now counting the number of newly linked scenes, instead of the total number of scenes as before.

I would just remove r_need_check_set and r_totscene from this function. And then instead count the total number of scene in lib_link_scenes_check_set, and don't do the need_check_set test. Both these things are really just unimportant optimizations, which were easy before but are too messy to be worth it.

9413

If the comment is correct, it should be ID_WM instead of ID_WS.

source/blender/blenloader/intern/versioning_280.c
1561–1563

I would leave XXX only for serious problems, if you think it's actually safe there is nothing expected to be fixed.

It can be a problem to be too conservative, if there is other code in between that relies on the conversioning having already happened, like BKE_mesh_calc_edges_loose. But since it was part of lib linking it was already happening late, so probably it won't cause any new problems.

This conversion was moved from direct link to lib link in rB90fa130a696a: =bmesh= various bugfixes. I'm pretty sure it's related to external customdata layers, where it's going to access id->lib to get the absolute filepath and then crash. And that problem would now happen again.

Given that we are talking about 9 year old files with a feature that wasn't used much, I'd be ok to disable the code that does this conversion, like P1236. And then we can get rid of that G_MAIN modification hack as well.

This revision now requires changes to proceed.Jan 31 2020, 11:55 PM
Bastien Montagne (mont29) marked 2 inline comments as done.Feb 1 2020, 4:24 PM
Bastien Montagne (mont29) added inline comments.
source/blender/blenloader/intern/versioning_280.c
1561–1563

Am not sure why you say it would crash again with that change? id->lib should be fully valid here, just as it is in liblink code? blo_split_main is not breaking anything here afaict, and id->lib, although pointing to some data that would not be in hacked G_MAIN, should still be valid?

Removing that support and getting rid of those ugly hacked G_MAIN sounds like a good plan anyway, but would rather do it in a separate commit.

Updates from review.

Brecht Van Lommel (brecht) added inline comments.
source/blender/blenloader/intern/versioning_280.c
1561–1563

You're right, I was thinking it was in do versions before linking.

Doing this kind of versioning so late is fragile, but any issues from that would not be new.

This revision is now accepted and ready to land.Feb 3 2020, 1:26 PM
This revision was automatically updated to reflect the committed changes.