Motion transfer setup #1
No reviewers
Labels
No Label
No Milestone
No project
No Assignees
2 Participants
Notifications
Due Date
No due date set.
Dependencies
No dependencies set.
Reference: dr.sybren/rignodes#1
Loading…
Reference in New Issue
Block a user
No description provided.
Delete Branch "cgtinker/powership:motion_transfer"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
I've been testing out powership and tried to transfer raw tracking data to a rig with it. On the run I added various math and utility nodes, basically, making it work. While doing so, the math nodes (which I removed) and muting change has been introduced which may not work with all introduced nodes properly (b.e. you currently cannot split a vector, do math operations and combine it again without unexpected results).
Besides there are no drastic changes and to support keyframed controls I added a "on_frame_changed" handler.
This is great!
I haven’t been able to do a full review, just had time for a small peek and a few notes.
@ -50,7 +50,6 @@ def execute_tree(
# The running of this tree will trigger another depsgraph update, which
# should not trigger yet another execution.
global _skip_next_autorun
Please don’t include formatting changes like this. Formatting changes shouldn’t be in the same PR as functional changes.
@ -3,2 +3,3 @@
is_first_load = "nodes" not in locals()
import bpy
from . import nodes
is_first_load = "nodes" not in locals()
Please keep this block as-is. These changes will break the reloading mechanism.
Please keep this block as-is. These changes will break the reloading mechanism.
That's why it hasn't been updating!
My lsp destroyed the formatting a bit, doing my best to fix it – pylance has been fighting against mypy and no-one has been the winner I guess. Pep8 came to the rescue and yea well, here we are :)
What kind of formatter are you using? (probably will add a few more nodes in the future if you don't mind)
@ -101,0 +101,4 @@
def _on_frame_changed_post(
scene: bpy.types.Scene, depsgraph: bpy.types.Depsgraph
) -> None:
global _skip_next_autorun
This seems like the same logic as the depsgraph update callback. Maybe move it into a separate function and call that from both places?
@ -579,0 +627,4 @@
class Distance(AbstractPowerShipNode):
""" Calculate normal from three points. """
Does this calculate a normal vector? Or its length?
The length.
I went through the comments and updated them (some were unrelated and happened due to closed eyes copy pasting...)
Glad you like the patch!
I considered to remove the distance node and to just add a regular length node.
On the one hand nodes like a "distance node" are really easy to understand and kinda useful as they save you a node in the tree, but on the other hand a length node is way more useful overall. Personally I even prefer length squared but...
What do you think about "easy to read for a non-math background" nodes?
Stopped using a formatter for now, guess we should match them first.
I think both would be useful; I would expect a "length" node to have a single vector input, whereas a "distance" node I would expect to have two vectors as input. Technically they'd be the same if one of the input vectors is the origin.
We could give the node two outputs and only compute connected ones? That way you can choose between the length of the squared length whenever you use the node.
That might be a good choice, indeed. Which I think would lead us to having two nodes, "distance" and "length", as that would make them easier to find in the "Add node" menu.
I'm using black.
Thanks for the updates! I found a bunch of things to adjust, but I think they're all minor and easily done.
Please check the code with
mypy .
, there's a bunch of errors. I've also solved a few of those in themain
branch, so be sure to merge that before looking at this. If you need a hand with that, let me know.@ -1,3 +1,6 @@
/.venv
/.vscode
__pycache__
*.DS_Store
Please don't add platform/IDE-specific files here. These can go into
.git/info/exclude
(which does the same but isn't tracked) or in your global gitignore file (if you want to have those available to all Git projects you work on).@ -75,0 +81,4 @@
) -> None:
global _skip_next_autorun
if not _skip_next_autorun:
_skip_next_autorun = True
Since
_skip_next_autorun
is a boolean, theif
condition can be removed.@ -6,2 +6,2 @@
from math import degrees, radians
from typing import TypeVar, Callable, Optional, Iterable
from math import degrees, radians, sqrt
from typing import TypeVar, Callable, Optional, Iterable, Any
Both
sqrt
andAny
are unused.@ -272,3 +272,3 @@
def _first_input_to_output(self) -> None:
"""Copy the first input's default value to the output, if the sockets are compatible."""
Keep formatting changes out of the patch. You can use
git gui
or some other tool to cherry-pick which lines you do (not) want to include in a commit. That way you can exclude such changes, commit the rest, then revert the unwanted formatting changes to get rid of them.@ -579,0 +582,4 @@
bl_icon = "EMPTY_ARROWS"
def init(self, context):
self.add_optional_input_socket("NodeSocketFloat", "X")
Here I think a regular input socket would be better. The "optional" ones (and I should document this better) are intended for sockets that will be ignored when they are unconnected. In this case I feel regular sockets would be better, as those allow you to enter their values manually when the socket is unconnected.
@ -579,0 +598,4 @@
if y is not None:
v.y = y
if z is not None:
v.z = z
This can be simplified by using shortcutting (
a or b
evaluates tob
whena
is falsey).@ -579,0 +609,4 @@
bl_icon = "EMPTY_ARROWS"
def init(self, context):
self.add_optional_input_socket("NodeSocketVector", "Vector")
Same as above, this could be a regular vector input socket.
@ -579,0 +647,4 @@
bl_icon = "EMPTY_ARROWS"
def init(self, context):
self.add_optional_input_socket("NodeSocketVector", "U")
These should again be regular input sockets, as it makes no sense to do the computation without a full set of three points.
@ -579,0 +658,4 @@
w = self._get_optional_input_value("W", Vector)
a = v - u
b = w - u
normal = a.cross(b)
This only outputs a normal vector when
a
andb
are perpendicular. Better to usea.cross(b).normalized()
@ -579,0 +680,4 @@
class RotateTowards(AbstractPowerShipNode):
""" Calculate the rotation from a vector with a track and up axis
based on the given origin, destination. """
Follow PEP 257 (in this case, the initial line should be shorter, and the leading/trailing spaces should be removed). Same for other docstrings.
Wellll.... try to follow PEP 257 but also keep in mind that Blender uses those docstrings for the tooltips, and thus they shouldn't end in a period. Stupid, I know...
@ -579,0 +695,4 @@
up: bpy.props.EnumProperty( # type: ignore
name="Up",
items=_enum_up_axis_items,
default='Y'
Why is
Y
the default up-vector, and notZ
?I thought it was like that in the python interface (and I was wrong).
Just checked in mathutils_Vector.c, there the up axis is Y and the track axis is Z. Should we follow that convention or go for Z as up axis and X as track axis?
@ -579,0 +707,4 @@
def init(self, context):
self.add_optional_input_socket("NodeSocketVector", "Origin")
self.add_optional_input_socket("NodeSocketVector", "Destination")
I think these names could be improved for clarity. How about "Vector" and "Rotate To"?
@ -579,0 +724,4 @@
bl_idname = "OffsetRotation"
bl_label = "Offset Rotation"
bl_icon = "EMPTY_ARROWS"
default_value = Quaternion()
default_value
doesn't seem used.@ -579,0 +772,4 @@
angle_type: bpy.props.EnumProperty( # type: ignore
name="Type",
items=[
("DEFAULT", "Unsigned", ""),
Why name it
DEFAULT
instead ofUNSIGNED
? And why isUNSIGNED
the default value? Not saying that it should be the other one, just wondering about your thought process.Usually an angle is not signed, you got to define something to sign it. That's why I thought unsigned angles should be the default.
I rechecked and to be honest, I neither knew that the "signed" vector method is only for 2D vectors nor how it actually signed an angle. So I guess I removed it and kept only the "unsigned" angle for now - my bad sorry. Kinda liked the idea of an easy to use signed angle.
So far, when I needed a signed angle, I ended up using a plane. Basically I've used the signed distance (based on the normal) from the plane to the destination of the vector and used the sign for the angle. This is possible with the current system without changes so I guess it's fine.
@ -579,0 +794,4 @@
signed = self.angle_type == 'SIGNED'
angle = 0
if not (u.length == 0 or v.length == 0):
Don't compute
length
when you can uselength_squared
as well.@ -579,0 +845,4 @@
case 'SUBSTRACT':
res = u-v
case 'DIVIDE':
res = Vector([x/y if y != 0.0 else float("nan")
This could be an interesting design discussion.
Not something to do here -- this code is fine.But in general, we have to decide how Powership is going to handle erroneous values. For example,. theVector.normalized()
function I suggested above will simply return(0, 0, 0)
for zero vectors. I feel that maybe here it could also make sense to definex / 0 → 0
and return as many usable values as possible.I actually had a little design discussion with @nathanvegdahl about this, and until we have a 'debug mode' that can visualise where NaNs are produced, it's better to stick to regular floats. So in this case
x/y if y != 0.0 else 0.0
And another minor thing: the list comprehension can be replaced by a generator expression for a little bit of added performance and readability:
@ -579,0 +850,4 @@
case 'CROSS':
res = u.cross(v)
case _:
print("Vector math operation not found:", self.operation)
Either log/print, or raise an exception. Don't do both, as one issue will be reported twice.
@ -579,0 +851,4 @@
res = u.cross(v)
case _:
print("Vector math operation not found:", self.operation)
raise ValueError
Don't use a class as exception, always instantiate it.
@ -579,0 +897,4 @@
case 'SUBSTRACT':
res = u-v
case 'DIVIDE':
res = u/v if v != 0 else float("nan")
nan → 0.0
@ -579,0 +900,4 @@
res = u/v if v != 0 else float("nan")
case _:
print("Math operation not found:", self.operation)
raise ValueError
Same as above.
@ -658,0 +997,4 @@
match self.space:
case "WORLD":
bone_mat_world = Matrix.LocRotScale(
loc, rot.normalized(), scale)
in which case would
rot
not be normalized? I'd expectbone_mat_world.decompose()
to return a unit quaternion. If you want to normalize the socket value, do that above withrot = control_rotation.normalized()
.I think that can be removed. I was confused because the quaternion affected the scale which indicates that it's not a unit quaternion. This happened on longer bone chains - rounding the scale seemed to fix the issue so I guess it has been some floating point error.
@ -1023,3 +1321,3 @@
nodeitems_utils.NodeItem("ToEulerNode"),
nodeitems_utils.NodeItem("FromEulerNode"),
],
]
Always end in a comma, it keeps diffs clean when you want to add another parameter later.
Thanks for the hints and information!
Good to know that generators are more efficient than list comprehensions. I always thought it's the other way around. Merged the changes from your main branch and fixed the issues.
I'm not sure how to go about mypy. Probably the stubs I use are not sufficient. I tried "blender stubs" and "bpy-fake-module-3.4" but as I get about 600 mypy errors something seems to go wrong.
List comprehensions are basically generators that collect the result into a list, so it needs more memory allocations.
I don't use those stubs, so mypy is arguably less useful in my setup, but I don't get any errors, at least not on the
main
branch.mypy.ini
should be set up so that it ignores unknown modules. Be sure to run mypy from the powership directory itself, and not some higher-up directory, otherwise it may not find themypy.ini
file.Update: also this PR produces no mypy errors any more on my system.
Alright, thanks! Sorry for the commits after asking for the review, I cleaned up some typing related stuff. Tried to get Matan Gover's mypy extension to work properly with stubs in vsc... But guess that's to much.
Running mypy globally like you described works for me swell.
I'm getting errors when opening
powership_transfer.blend
and changing a frame to force a forward solve:I'm working on a more generic solution for things like this ;-)
Thanks again for the PR! I'll accept & merge it into the
main
branch, then do a few tweaks to improve upon things further. Your changes illuminated some shortcomings in my code, and it's easier for me to merge this PR first, then address those shortcomings directly on the main branch.