Paths

Table of Contentst

Re-establish link to proxies when they are made local after appending.
ClosedPublic
Actions

Authored by Sybren A. Stüvel (sybren) on Oct 6 2016, 4:53 PM.

Details

Reviewers

Bastien Montagne (mont29)
Sergey Sharybin (sergey)

Maniphest Tasks

T49495: Broken armature proxy when appending scene

Summary

Fixes T49495, but since proxies are hard to get right, this really needs reviewing and thorough testing.

This patch requires that a scene is appended to the current blend file, and that the "Localize all" is not checked. The appended proxy object should also not be referenced from anything in a library (for example in a constraint). Referencing it from the appended data should be fine.

I've tested using:

unzip
linking.zip360 KBDownload
open Blender
File → Append → untick "Localize all" → select shot_file.blend/Scenes/test_scene
Switch to test_scene
Play around with the proxied armature, or play back the animation.

Diff Detail

Repository

rB Blender

Branch

sybren-relink-proxy

Build Status

Buildable 216
Build 216: arc lint + arc unit

Event Timeline

Sybren A. Stüvel (sybren) updated this revision to Diff 7554.Oct 6 2016, 4:53 PM

Sybren A. Stüvel (sybren) retitled this revision from to Re-establish link to proxies when they are made local after appending..

Sybren A. Stüvel (sybren) updated this object.

Sybren A. Stüvel (sybren) added reviewers: Bastien Montagne (mont29), Sergey Sharybin (sergey).

Sybren A. Stüvel (sybren) set the repository for this revision to rB Blender.

Sybren A. Stüvel (sybren) added a task: T49495: Broken armature proxy when appending scene.

Sybren A. Stüvel (sybren) updated this object.Oct 6 2016, 5:01 PM

Sybren A. Stüvel (sybren) updated this object.Oct 6 2016, 5:11 PM

To my knowledge this is fine.

General approach looks OK-ish (whole proxy thing is OK-ish at best anyway), though think there are some logical mistakes in code, noted in in-lined comments.

And limits mentioned in commentshere should be put in code's comments too ;)

source/blender/blenkernel/intern/library.c
1629	rather use 'remap_proxy_link' here, it’s common name for that kind of things these days in code ;) In fact, not convinced we need a new func here, this could be directly embedded in `BKE_library_make_local`’s code imho.
1636	This check should be done by caller code, would avoid useless func call altogether.
1638–1645	This whole block seems to be useless to me? `id->newid` being a copy, it will always be local (i.e. have NULL lib pointer). What you may want to check here is whether `ob->proxy->id.lib` is NULL - in this case indeed we cannot preserve proxy relation.
1647–1649	Think you rather want to check `id` usages here? again, `id->newid` is always local… But if `id` is still in use (either in lib or locally, btw), then you cannot transfer its proxy to its local copy.
1658–1659	Please, no single-lined if, else, etc. ;) To be removed anyway, if new local proxy was not used locally would be a nasty bug!
1754–1755	So remove this call here and…
1760	… add an else if clause her: else if (GS(id->name) == ID_OB && ((Object *)id)->proxy != NULL) { <amended code from reestablish_proxy_link()> }

This revision now requires changes to proceed.Oct 6 2016, 6:33 PM

Sybren A. Stüvel (sybren) marked 4 inline comments as done.Oct 7 2016, 9:40 AM

Sybren A. Stüvel (sybren) added inline comments.

source/blender/blenkernel/intern/library.c
1629	Will rename. Placing this code into its own function makes it more readable, and allows us to do check & quickly `return`. Otherwise the checks become more wieldy, the indentation level will be much greater, and the code will be less readable.
1647–1649	My approach was to check `id->newid` because it has already been remapped using `BKE_libblock_remap()`; it seems strange to check for usages of `id` after such a remap.

Bastien Montagne (mont29) added inline comments.Oct 7 2016, 9:47 AM

source/blender/blenkernel/intern/library.c
1629	I really think this should be inlined, it’s short (once you remove useless bits as mentionned in other comments), and also avoids calling `BKE_library_ID_test_usages(bmain, id, &is_local, &is_lib);` twice, since this info is also needed here. Not in favor of too much utility functions, especially when that does not help any factorization.
1647–1649	No it is not strange :P id to new_id remapping is done with `ID_REMAP_SKIP_INDIRECT_USAGE`, which means no indirect (lib) usage will be remapped, we don’t want other linked data to use local data ;)

Sybren A. Stüvel (sybren) marked 7 inline comments as done.Oct 7 2016, 9:56 AM

Reworked patch after feedback from mont29. Does require patch on BKE_library_ID_test_usages() (0f88a3546cbd020af3f5c47b049ae4ecc6bc54ba)

Code LGTM, can land in master! :)

source/blender/blenkernel/intern/library.c
1764	tend to use 'proxified' rather than 'proxied'… None of them seems to be valid english anyway, though :P

This revision is now accepted and ready to land.Oct 7 2016, 10:34 AM

ah, maybe add a comment above whole block about why we need this, the limitations, and that it’s nasty hack around weak feature ;)

Committed in 5c651554e2f98bff1cfdff0a6a6f029453dc3309 and 78817ae95c8834588bf2acf64591ed9216fb7b97

Revision Contents
Changeset List

Path

Size

source/

blender/

blenkernel/

intern/

library.c

39 lines

Diff 7554

View Options

source/blender/blenkernel/intern/library.c

Re-establish link to proxies when they are made local after appending.ClosedPublicActions