Upgrade object_edit_linked to blender2.8
ClosedPublic
Actions

Authored by Rainer Trummer (aliasguru) on Dec 12 2018, 2:40 PM.

Details

Reviewers

Brecht Van Lommel (brecht)
Jacques Lucke (JacquesLucke)

Commits

rBAaa2e8a2a0ab1: Port 'Edit Linked Library' addon to Blender 2.8

Summary

This diff updates the object_edit_linked Add-on to Blender 2.8. The main changes were:

upgrade the registration code
add a command to the Topbar menu (was a TODO in the old code base)
fix class naming
use logging instead of print statements

There are also some whitespace changes due to my Eclipse environment, I hope this is acceptable. Also, I have added myself as co-author. If that's a problem, I can undo this.

Diff Detail

Event Timeline

Rainer Trummer (aliasguru) created this revision.Dec 12 2018, 2:40 PM

Rainer Trummer (aliasguru) added reviewers: Brecht Van Lommel (brecht), Jacques Lucke (JacquesLucke).Dec 13 2018, 5:44 PM

I only made a very simple test and it seems to work (obviously not all code was reached, otherwise some bugs would have appeared).

While the functionality is ok, I think you should undo some style changes you/Eclipse did:

Things like # @UnresolvedImport etc should not be in this repo afaik.
Use pep8 function argument style again: e.g. name="Autosave" instead of name = "Autosave". While I'm not sure which one is better in general, consistency is more important here.
I don't really understand why Eclipse added more spaces around # symbols, I think those spaces should be removed again.
Style for function parameter annotations is context: bpy.context instead of context:bpy.context.

Maybe you can just most of these style issues by just configuring Eclipse a bit differently. Unfortunately I don't use Eclipse, so I can't tell you how to do it.

object_edit_linked.py
54	this should use the new `select_set` function.
56	is there any reason to use `context.blend_data` instead of `bpy.data`? Also `context.scene.objects.active` does not exist anymore. Use `context.view_layer.objects.active` instead?
254	Use `classes` instead of `__classes__`.

This revision now requires changes to proceed.Dec 13 2018, 7:13 PM

Updated the diff to meet requirements by Jacques Lucke, thanks for the feedback!

added a few comments

object_edit_linked.py
54	completely missed that, thanks
56	regarding context.blend_data: I started using that in my code when I realised it existed. At the same time, there were restrictions on what is available via context during UI `draw()` phase. So I thought there must be a reason why it is there, and maybe the reason is to prevent users to access things they should not. Is there a 'best practise' on when to use context.blend_data, or does it simply not matter? regarding `context.view_layer`: again, overlooked that, thanks
254	Done that change, maybe you could shed some light on why you requested it? When I learned Python, I came across the discussion that Python does not have strict visibility control of class members like C# (public, private, internal,...). So to tell users that read your code that a variable is supposed to be for internal use only, they recommended (in a book) to use the yourVariable notation. Is `classes` not supposed to be a "private" field?

A thing that might be worth aligning on is if in the target file the collection or object should be selected at all. I'm raising this because the target could open in Modelling Workspace, and then it automatically switches to Edit Mode. This might be a Performance issue on large scenes.

As an off-topic comment, was I doing the right thing when assigning reviewers? I never know if I should specify someone myself, or if that happens automatically. I did it myself this time, but it feels a bit like assigning work to people who should not be bothered. Is there a guideline on how to do this?

I'm not sure about the workspace opening thing. I'd just leave it as it is for now. Can be changed later on as well.

When I uploaded my first patches I also did not specify reviewers, but learned that I should have. So you did everything right.
If you don't specify a reviewer it is more likely that the patch gets lost over time..

I have not tested if the addon works now, I expect that you did it.

object_edit_linked.py
56	tbh, I don't really know why `context.blend_data` exists. I've never used it myself. Maybe @Brecht Van Lommel (brecht) can shed some light on this? For me it is best practice not to use `context` more than necessary. In general context depended functions should be avoided imo. Passing `context` into functions (and using `bpy.context`) always reminds me of "You wanted a banana but what you got was a gorilla holding the banana and the entire jungle." (Explanation) I'm not saying that I never do this, but that it should be avoided in general to make code more reusable.
254	It is right, in Python the concept of private variables does not really exist. Instead you can prefix your variable name with `_` or `__` to indicate that it should not be used from the outside. Also when you prefix it with `__` Python will mangle the variable name (at least in classes, not sure if it is the same at module level). Note: you should only use the prefix but not the suffix. Having something like `__classes__` could interfere with other things in Python. Also with the same reasoning you'd have to prefix almost all module level variables with `_` which does not increase the code quality imo. (Private Variables) Another way to indicate which things should be importable from a module is to use the `__all__` property at module level. This is used in multiple places in Blender. The main reason I suggested to use `classes` is that this is what is done in all the other addons and internal Python code afaik.

This revision is now accepted and ready to land.Dec 14 2018, 11:04 AM

@Jacques Lucke (JacquesLucke), I guess you can commit this?

object_edit_linked.py
56	It doesn't serve a purpose at this point. Maybe in the future if we supporting having multiple .blend files open, but even then I'm not sure. It's fine to use `bpy.data`.

@Jacques Lucke (JacquesLucke) Yes, I ran a few tests with production files we have here in the studio. I can't claim I have done every thinkable test though, just the common operations (Edit Linked, Go back, use_autosave, use_instance)

In D4070#92242, @Brecht Van Lommel (brecht) wrote:

@Jacques Lucke (JacquesLucke), I guess you can commit this?

ah yes, think so. Could you tell me how to do that right? Never committed code written by someone else (and keeping the original author intact).

You can change the author of a commit like this:

git commit --amend --author="Rainer Trummer <aliasguru>"

Then for the commit message just make sure you have the Differential Revision line so it links back here.

Closed by commit rBAaa2e8a2a0ab1: Port 'Edit Linked Library' addon to Blender 2.8 (authored by Rainer Trummer (aliasguru), committed by Jacques Lucke (JacquesLucke)). · Explain WhyDec 14 2018, 1:58 PM

This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

Path

Size

object_edit_linked.py

153 lines

Diff 12896

View Options

Upgrade object_edit_linked to blender2.8ClosedPublicActions