Hi @Damien Picard (pioverfour),

Great, this is exactly what I hoped would happen, someone would fix my errors and help me improve haha.

I'm new to this Blender dev thing, so I don't know how to grab this diff, but I will figure it out.
I'll take a look at the code in more detail when I get a chance but here are some quick thoughts.

Fixes
Fix widgets’ names: they were hardcoded and didn’t follow the preferences, leading to crashes.
The UI was put back into the Item category, instead of Create, because it is not related to object creation.
Fix some strange topology in two widget shapes.

Cheers!

Improvements (in my opinion — to be discussed!)
UI
UI and operators use a new poll method, so that they work when either the rig or the camera is selected.
The composition guides UI was converted to a panel, so that they may be drag-selected.

Nice.

Focal Length display depends on the camera’s Lens Unit.

Are you talking about the 'focal_length' here, or just the units it is using? (ie meters of Blender Units etc)

Operators
Marker binding and DOF object operators were converted to the bpy.data API, making them simpler.
Rigging
Bones were moved around so that they are more similar between rigs.
They were scaled down to be 1 unit long, a simpler length — for instance, widgets are the same size as modeled. Widgets were scaled up to compensate.
The camera and aim bones were placed at 1.7 unit high, to be approximately at a standing human’s eyes’ height if the scene is in meters.

I originally had the crane rig length and height set to 1 for simplicity, plus in old versions of Blender the float sliders where super sensitive but that might not be an issue anymore post 2.80.

Properties and drivers for focal length and Aim Lock constraint influence were removed, as they seemed redundant. The properties are now directly exposed in the UI.

Oh these properties are for the ease of animation. When you animate the rig and want to control the focal length, you had to animate the rig AND the camera, creating 2 animation data blocks.
If it is driven by a property it contains the focal length (and f-stop etc) into the one action. Otherwise it's a pain in the butt to animate.

With the aim lock, this was there in case someone wants to disable it (essentially making it a normal unrigged camera). Image a shot where the camera follows a character, but at the end of the shot, the camera falls to the floor and bounces around (or something similar)
If you wanted to animate this with it locked, it would be pretty difficult to do.

Much of the rig generation was refactored to deduplicate code between the two rig types.

That was going to be my next task :)

General
Automatic renaming to .000 was removed, since Blender already handles duplicate names.
Widget prefix and collection were renamed to WGT- and Widgets respectively. This is to be closer to another much-used rigging add-on, Rigify, hopefully unifying them.

ewww, That is one of the things I dislike about the Rigify naming convention haha. But I can change my preferences that is fine.

Arm and height bones were renamed using PascalCase, to be more in line with Blender’s own convention (see eg. NurbsCurve).
The GPL license header was added to every file.
Some cleanup was done to better respect Python’s PEP 8.

Didn't know that about the GPL block. Cheers.

In D6543#153709, @Wayne Dixon (waylow) wrote:

Great, this is exactly what I hoped would happen, someone would fix my errors and help me improve haha.

I like free software :)

I'm new to this Blender dev thing, so I don't know how to grab this diff, but I will figure it out.

You can download the diff file and apply it to a local Blender source repository, using git apply D6543.diff in the release/scripts/addons directory. You can then replace the addons directory in Blender’s install location by a soft or symbolic link to the add-ons repo in the sources. I know there is also the Phabricator command line tool, Arcanist, explained here, but I haven’t used it.

When you’re ready for review, there are a few ways we could go about this:

• You can simply leave comments on this diff page directly in the code diff, I’ll make new versions of the diff as the review proceeds, and commit once you accept the changes;
• I see this add-on is also hosted on a GitHub repository. If it’s more convenient for you, I can make a pull request there, and commit to Blender’s add-on repo once merged;
• if you have push access to the addons repository, you can also apply the diff in git, edit the files there, commit and push yourself. You’d then close this diff;

Focal Length display depends on the camera’s Lens Unit.

Are you talking about the 'focal_length' here, or just the units it is using? (ie meters of Blender Units etc)

The camera’s focal length (lens property) in mm is exposed in your interface. But the user may choose to use a field of view in degrees instead (the angle property), and then the mm value is useless, so my change displays the property currently selected.
EDIT: This was true if directly exposing the camera’s properties, but it is useless with a custom prop and driver, so I removed this part.

Bones were moved around so that they are more similar between rigs.
They were scaled down to be 1 unit long, a simpler length — for instance, widgets are the same size as modeled. Widgets were scaled up to compensate.
The camera and aim bones were placed at 1.7 unit high, to be approximately at a standing human’s eyes’ height if the scene is in meters.

I originally had the crane rig length and height set to 1 for simplicity, plus in old versions of Blender the float sliders where super sensitive but that might not be an issue anymore post 2.80.

I didn’t feel this was a problem when using shift + drag for precision.

Properties and drivers for focal length and Aim Lock constraint influence were removed, as they seemed redundant. The properties are now directly exposed in the UI.

Oh these properties are for the ease of animation. When you animate the rig and want to control the focal length, you had to animate the rig AND the camera, creating 2 animation data blocks.
If it is driven by a property it contains the focal length (and f-stop etc) into the one action. Otherwise it's a pain in the butt to animate.

Oh I get it. Makes sense! I’ll put the lens one back in.

With the aim lock, this was there in case someone wants to disable it (essentially making it a normal unrigged camera). Image a shot where the camera follows a character, but at the end of the shot, the camera falls to the floor and bounces around (or something similar)
If you wanted to animate this with it locked, it would be pretty difficult to do.

The property is still exposed, only directly from the constraint instead of through a custom prop and driver. Since it is in the armature object, it still belongs to the same action as the rest of the properties.

Widget prefix and collection were renamed to WGT- and Widgets respectively. This is to be closer to another much-used rigging add-on, Rigify, hopefully unifying them.

ewww, That is one of the things I dislike about the Rigify naming convention haha. But I can change my preferences that is fine.

This is a matter of personal preference: as far as I know there isn’t a strict convention. I just thought it would be more convenient, but it’s up to you!

Hey Damien,
I haven't had a proper look at your code yet (I'm hoping to get a chance this Sunday), but I've just noticed that the PascalCase is used for the naming convention of the Bones as well as the properties.

While I don't mind this for properties and variables etc (as the end user won't really see them), for the actual bone names there are much easier to read conventions.

Rigify uses snake_case with common prefixes - ie MCH and DEF .L and .R
(although it is very untidy with all the .L.001 and .L.002 etc all over the place!)

I also use the same snake_case, except with some modifications like suffixes instead of prefix and other small changes that make it easier to read, and hopefully others haha.
I'm usually pretty consistent across all my rigs, although it's an evolving system.

Here's my convention.
-All bones are snake_case (most people find this easier to read)
-control bones begin in the Capital letter (Root, Aim, Camera, Crane_height, Crane_arm)
-any bone that the animator will not touch does not start with a capital. (aim_MCH)
-mechanism bones have the _MCH suffix (capitals make it easier to read)
None of my other conventions are relevant to this rig.

I think either either this convention or rigify convention would make more sense (the only difference is it's all lower case and the MCH- is a prefix on the 'AimChild' for this example)

Does that make sense?
Thanks for you work on improving my code.

In D6543#154194, @Wayne Dixon (waylow) wrote:

I haven't had a proper look at your code yet (I'm hoping to get a chance this Sunday)

Hi, no problem, take your time!

but I've just noticed that the PascalCase is used for the naming convention of the Bones as well as the properties.

As I said I switched to PascalCase because that’s what Blender uses by default for objects, but it needs not apply to bones here.

I renamed the camera object from Dolly_camera and Crane_camera to just Camera, because the distinction didn’t seem useful and to simplify the code slightly. Do you agree with that?

For Crane_height and Crane_arm, I don’t mind their current name, I’ll change them back.

I thought a bit more about the aim mechanism bone. First, I like Rigify’s use of prefixes because it groups bones by category in the outliner, and using a hyphen makes a clear distinction with the underscores used as whitespace.
More importantly, its purpose is not immediately clear, so I think it should be renamed to something like MCH-Aim_shape_rotation.

In D6543#154202, @Damien Picard (pioverfour) wrote:

I renamed the camera object from Dolly_camera and Crane_camera to just Camera, because the distinction didn’t seem useful and to simplify the code slightly. Do you agree with that?

I meant to say that I’d like to do that.

Also, some more UI modifications I forgot to mention.

• For the crane rig, I moved the Tracking slider out of the Crane Arm box.
• I removed the redundant "Focal length:" label. Now, I wonder if it may not make more sense to move the slider to the very top of the UI, before Clipping.

Still struggling to find time to look properly.
I don't know how to easily download your diff and apply it at my end.
(I'm not really a developer)
But I think I need to install arc, is that right?

In the mean time - I have some answers for you.

I renamed the camera object from Dolly_camera and Crane_camera to just Camera, because the distinction didn’t seem useful and to simplify the code slightly. Do you agree with that?

It's handy to have the name differentiated from the normal run of the mill 'Camera' that is in the default start up file.
I've worked on projects where a temp camera have been placed in the scene but then the real camera (and rig) was added later. So it's handy to see the difference in the name.

I thought a bit more about the aim mechanism bone. First, I like Rigify’s use of prefixes because it groups bones by category in the outliner, and using a hyphen makes a clear distinction with the underscores used as whitespace.

That's another thing I personally don't like about rigify, which is purely just a preference thing. I prefer the suffix as it will group bones by name. That way I can see all "Arm" bones together, for example.
Also if you have to add a constraint or need to type in one of the input boxes, you can start typing and see all the bone names populate in the list.
But having said that, I'm not opposed to the Prefix in this instance. It's just one bone.

More importantly, its purpose is not immediately clear, so I think it should be renamed to something like MCH-Aim_shape_rotation.

Yeah I like that.

For the crane rig, I moved the Tracking slider out of the Crane Arm box.
I removed the redundant "Focal length:" label. Now, I wonder if it may not make more sense to move the slider to the very top of the UI, before Clipping.

Once I figure out how to load your code, I'll look at the UI edits....man I'm a noob.
I tried to group the UI logically, but I felt like the crane arm UI was a bit clunky.
In the latest version I made, I refactored it so the two used the same code block (it used to be 2 seperate classes), but there was still more improvements to make.

Also I was going to try and look into seeing if the dof_object could actually be set to a bone or figure out which developer to talk to to implement this.
That would get around the hack of having to create an empty and then parent the empty to the aim bone.
It would also be helpful to have access to bones rather than just objects in many other places too. (constraints for example)

In D6543#154392, @Wayne Dixon (waylow) wrote:

I don't know how to easily download your diff and apply it at my end.
But I think I need to install arc, is that right?

If you are struggling with applying the patch, I’d recommend against installing arc, as I find it too complex for simple patches. Git alone works just fine if you don’t handle many patches a day.

The patch applies to the addons repo, which is a submodule of Blender. If you do or intend to build Blender yourself, following the instructions on this page will get you the add-ons repo under blender/release/scripts/addons/.

Otherwise, if you haven’t yet cloned and compiled Blender yourself and just want the add-ons repo, the easiest is to clone it somewhere with:

git clone git://git.blender.org/blender-addons.git

Once you have the repo, to apply the patch, download it from the top of this diff page and in the addons repo, do:

git apply D6543.diff

This command updates git’s worktree with the changes from the patch. It’s then a matter of having Blender use this repository to load its add-ons, which you can do with a soft link or other methods. Feel free to ask if you need further assistance with that.

I renamed the camera object from Dolly_camera and Crane_camera to just Camera, because the distinction didn’t seem useful and to simplify the code slightly. Do you agree with that?

It's handy to have the name differentiated from the normal run of the mill 'Camera' that is in the default start up file.

All right!

I prefer the suffix as it will group bones by name.

That is reasonable as well.

Also I was going to try and look into seeing if the dof_object could actually be set to a bone or figure out which developer to talk to to implement this.

This would be a nice addition indeed, but I can’t help with that!

Hey Damien,

Ok I think I got the patch applied.
I use git so thanks for that tip ;)

I had to actually put the patch in the add_camera_rigs folder for it to work - does that seem correct?
Every other location gave me an error.

Then I rebuilt Blender and I think the patch is active.

However, I'm getting an error.

Error: Traceback (most recent call last):
  File "/Users/waylow/blender-build/build_darwin/bin/blender.app/Contents/Resources/2.83/scripts/addons/add_camera_rigs/build_rigs.py", line 254, in execute
    build_camera_rig(context, self.mode)
  File "/Users/waylow/blender-build/build_darwin/bin/blender.app/Contents/Resources/2.83/scripts/addons/add_camera_rigs/build_rigs.py", line 179, in build_camera_rig
    pose_bones["Aim"].custom_shape_transform = pose_bones["AimChild"]
KeyError: 'bpy_prop_collection[key]: key "AimChild" not found'

location: <unknown location>:-1

I think the issue is that AimChild now has a different name.

In D6543#154854, @Wayne Dixon (waylow) wrote:

Ok I think I got the patch applied.
I use git so thanks for that tip ;)

Great to hear!

I had to actually put the patch in the add_camera_rigs folder for it to work - does that seem correct?
Every other location gave me an error.

That is correct. In the patch I made, it could be applied in the addons repo’s root as well, but Phabricator seems to rewrite it so it can only be applied in the add_camera_rigs directory. I didn’t know that.

However, I'm getting an error.

I think the issue is that AimChild now has a different name.

Sorry about that, I didn’t properly test my previous revision… This should now be fixed.
I don’t know how familiar you are with git so just in case, this command discards all current changes to the worktree (use wisely!):

git reset --hard HEAD

You can then re-download and re-apply the patch.

HI Damien,

I got a chance to check everything out. Well done.

I have a couple things worth discussing...

-I've noticed that when you add a rig, the camera doesn't automatically become the active camera for the scene.
I think this is the best default action.

-The old version would add the rig at the cursor position, this version adds it at 0,0,0.
I'm not sure what the best default, but I am leaning towards the user adding it at the cursor.
What do you think?

The only other thought I had was about the naming convention for the armature objects (and data)
The camera object gets named "Dolly_Camera" but the armature is just called Dolly/Crane
I think this would make more sense if it was consistent. "Dolly_Rig" and "Crane_Rig" would make more sense.

The same for the "Camera_Root" widget, that looks like it is the only object using CamelCase.
Incidentally, I named it 'Camera_Root' in the code to differentiate between another 'Root' widget that might be for another character in the file.

But other than those tiny things I like all your work.
Oh but I think you forgot to add your name to the list of authors ;)

Since you are planning to add a 2d camera rig, I was thinking about if there could be any other useful camera rigs.
The only thought I had was a 'turntable camera rig'. Something you could quickly add to rotate around a model or something to show it off to a client or whatever.
Haven't thought it through fully as I don't know if you want the camera to rotate around the scene, or if you would want the model to rotate and the camera to stay where it is (because of the scene lights etc).
I'll let it percolate in my brain a little.

Anyway, once again - great work!

In D6543#156867, @Wayne Dixon (waylow) wrote:

-I've noticed that when you add a rig, the camera doesn't automatically become the active camera for the scene.

Although it’s not Blender’s default behaviour, it makes sense for a camera add-on aimed at creating primary shooting cameras. I’ll do that.

-The old version would add the rig at the cursor position, this version adds it at 0,0,0.
I'm not sure what the best default, but I am leaning towards the user adding it at the cursor.
What do you think?

Huh, I thought I did that. My mistake.

The camera object gets named "Dolly_Camera" but the armature is just called Dolly/Crane
I think this would make more sense if it was consistent. "Dolly_Rig" and "Crane_Rig" would make more sense.

That’s fine with me.

The same for the "Camera_Root" widget, that looks like it is the only object using CamelCase.
Incidentally, I named it 'Camera_Root' in the code to differentiate between another 'Root' widget that might be for another character in the file.

I kind of got lost over the whole naming convention here, but at least it’s now self-consistent again!

Oh but I think you forgot to add your name to the list of authors ;)

Thanks for reminding me :)

Since you are planning to add a 2d camera rig, I was thinking about if there could be any other useful camera rigs.
The only thought I had was a 'turntable camera rig'. Something you could quickly add to rotate around a model or something to show it off to a client or whatever.

There is already such an add-on by Antonio Vasquez. It animates an existing camera instead of creating a new one. There are things I don’t like about it, such as the fact that you can’t animate the rotation linearly for looped animations, but it otherwise looks fine. I think it would be better to contribute to that one than add a new rig here.

Anyway, once again - great work!

My pleasure! I’ll commit to master and close this diff if everything is all right with you. Thanks for the review!

Awesome.
I figured out you can use

git apply -R patch

to revert the changes of the patch.

I'm learning things!
I learnt a lot from your changes too. The simplest change but so effective was using the popover panel instead of the menu for the comp guides.
That is what I wanted it to do, but did not know how. (added that to my memory bank ;) )

Yeah this revision looks spot on. Well done.
So I guess I did my first code review!
I'm getting new business cards that says "Blender Developer" haha

Kidding, in case you couldn't tell. Keep up the good work.

In D6543#157089, @Wayne Dixon (waylow) wrote:

git apply -R patch

to revert the changes of the patch.
I'm learning things!

Well, so am I! It makes sense that it exists, but I wouldn’t have thought.

The simplest change but so effective was using the popover panel instead of the menu for the comp guides.
That is what I wanted it to do, but did not know how. (added that to my memory bank ;) )

It’s fairly recent, it was introduced with Blender 2.80 and I only learned about it recently, looking at how it’s made in the 3D View’s header. It is pretty neat.

So I guess I did my first code review!
I'm getting new business cards that says "Blender Developer" haha

Kidding, in case you couldn't tell.

Oh well I’m only pretending, myself!

Keep up the good work.

I’ll commit this and close the diff, then. Thanks again for the add-on and review, I’ll soon™ get to work on adding that new rig.

Hi @Damien Picard (pioverfour)
I was asked to add a camera offset control to the 3d camera rigs.
Which I have done on my Github (https://github.com/waylow/add_camera_rigs)

Can I ask that you check it out?
What's the best way for you to do that?
Should I make a patch? (I've never done that before)

The only part which affects the 2d portion of the code is the parenting. The 3d versions have the camera parented to this new offset bone. And the 2d version still is parented to Camera bone.
So I haven't broken any of your clever code (or at least I don't think I have)

I have noticed that there are several dependency cycle warnings in the terminal which relate to the 2d rig.
(I swear it wasn't anything I did haha)

Hello @Wayne Dixon (waylow),

In D6543#194298, @Wayne Dixon (waylow) wrote:

What's the best way for you to do that?
Should I make a patch? (I've never done that before)

I just cloned your GH repo and copied the files over to the Blender add-ons. Git picks up the changes and can show a diff. A patch would probably work as well, though.

The only part which affects the 2d portion of the code is the parenting. The 3d versions have the camera parented to this new offset bone. And the 2d version still is parented to Camera bone.
So I haven't broken any of your clever code (or at least I don't think I have)

I gave it a quick test, and couldn’t find anything broken. Looks good to me!

I have noticed that there are several dependency cycle warnings in the terminal which relate to the 2d rig.
(I swear it wasn't anything I did haha)

Don’t worry, those errors were there before :D I couldn’t find a way to circumvent them, but they don’t seem to be causing any delay in the rig, so they should be safe to ignore.

Thanks for checking it out @Damien Picard (pioverfour)
I'm glad I didn't break it.

Are you the one who commits this to master?

In D6543#194432, @Wayne Dixon (waylow) wrote:

Thanks for checking it out @Damien Picard (pioverfour)
I'm glad I didn't break it.

Are you the one who commits this to master?

I can do that if you don’t have the access rights, but since you’re (I believe) the maintainer for this add-on it would make sense that you ask for them too.
I squashed and committed under rBA0068e69d3. :)