mottosso / cmdx

Fast and persistent subset of maya.cmds
https://mottosso.com/cmdx
BSD 2-Clause "Simplified" License
193 stars 36 forks source link

Usability improvements #24

Closed mottosso closed 4 years ago

mottosso commented 4 years ago

Lots of good stuff!

mottosso commented 4 years ago

Also don't forget to have a look at the broken test. Looks like there's just one, related to how paths are returned.

wougzy commented 4 years ago

ok! i'll have a look this evening. that's the part where i'm not familiar with all of this (:

mottosso commented 4 years ago

No problem, I'll guide you through it. :)

Step 1 Step 2
image image
Step 3 Step 4
image image

Yeah, the UI/UX of that is terrible. xD

You can also run the test locally, which I would recommend. You'll just need a few things.

mayapy
nose
flaky

And then call..

cd cmdx/
mayapy run_tests.py
wougzy commented 4 years ago

i'm a bit confused about the unit tests. it looks like they failed over already existing part of cmdx i didn't touched and that are quiet recent (the doctests failed at the docstrings of Plug.append and it tried to compare traceback log from failed on purpose commands)

otherwise i commited some cleanup and i removed my personal stuff that i moved to my wrapper

mottosso commented 4 years ago

I had a look at the failing tests, this one:

======================================================================
FAIL: Doctest: cmdx.Plug.append
----------------------------------------------------------------------
Traceback (most recent call last):
  File "C:\program files\autodesk\maya2020\bin\python27.zip\doctest.py", line 2226, in runTest
    raise self.failureException(self.format_failure(new.getvalue()))
AssertionError: Failed doctest test for cmdx.Plug.append
  File "C:\github\mottosso\cmdx\cmdx.py", line 2350, in append

----------------------------------------------------------------------
File "C:\github\mottosso\cmdx\cmdx.py", line 2366, in cmdx.Plug.append
Failed example:
    node["notArray"].append(2.0)
Expected:
    Traceback (most recent call last):
    ...
    TypeError: "|appendTest.notArray" was not an array attribute
Got:
    Traceback (most recent call last):
      File "C:\program files\autodesk\maya2020\bin\python27.zip\doctest.py", line 1315, in __run
        compileflags, 1) in test.globs
      File "<doctest cmdx.Plug.append[5]>", line 1, in <module>
        node["notArray"].append(2.0)
      File "C:\github\mottosso\cmdx\cmdx.py", line 2390, in append
        raise TypeError("\"%s\" was not an array attribute" % self.path())
    TypeError: "appendTest.notArray" was not an array attribute

And it's subtle, but the problem is:

# Expected
    TypeError: "|appendTest.notArray" was not an array attribute

# Got
    TypeError: "appendTest.notArray" was not an array attribute

Notice the missing |? It has to do with how attribute paths have changed from full to shortest. We should definitely keep showing the full paths, but I also noticed that..

>>> persp = encode("persp")
>>> persp["translate"].path()
persp.translate
>>> persp["translateX"].path()
persp.translate.translateX

Where the shortestPath didn't address the issue of overly verbose attribute names, unless we pass full=False.

>>> persp["translateX"].path(full=False)
persp.translateX

I think False should be the default, I can't see why one would ever want that extra level for attribute paths, it especially doesn't add to readability. Unless I'm overlooking something?

mottosso commented 4 years ago

I made some changes but wasn't able to push to your repo, here's a patch it would be great if you could integrate.

0001-Reduce-default-verbosity-of-attribute-names.patch ```bash From 025004bd2a86edded4ac9e244c06ec4055aa128c Mon Sep 17 00:00:00 2001 From: Marcus Ottosson Date: Wed, 7 Oct 2020 07:09:06 +0100 Subject: [PATCH] Reduce default verbosity of attribute names E.g. from .translate.translateX -> .translateX --- cmdx.py | 46 ++++++++++++++++++++++++++++++++++++++-------- 1 file changed, 38 insertions(+), 8 deletions(-) diff --git a/cmdx.py b/cmdx.py index 1f1006a..ef6c91b 100644 --- a/cmdx.py +++ b/cmdx.py @@ -2096,7 +2096,7 @@ class Plug(object): if isinstance(other, str): try: # E.g. node["t"] + "x" - return self._node[self.name() + other] + return self._node[self.name(long=False) + other] except ExistError: # E.g. node["translate"] + "X" return self._node[self.name(long=True) + other] @@ -2662,20 +2662,50 @@ class Plug(object): return self._mplug.attribute().apiTypeStr - def path(self, full=True): + def path(self, full=False): + """Return path to attribute, including node path + + Examples: + >>> persp = encode("persp") + >>> persp["translate"].path() + '|persp.translate' + >>> persp["translateX"].path() + '|persp.translateX' + + """ + return "{}.{}".format( - self._node.shortestPath(), self._mplug.partialName( + self._node.path(), self._mplug.partialName( includeNodeName=False, useLongNames=True, useFullAttributePath=full ) ) - def name(self, long=False, full=True): - return self._mplug.partialName( - includeNodeName=False, - useLongNames=long, - useFullAttributePath=full + def name(self, long=True, full=False): + """Return name part of an attribute + + Examples: + >>> persp = encode("persp") + >>> persp["translateX"].name() + 'translateX' + >>> persp["tx"].name() + 'translateX' + >>> persp["tx"].name(long=False) + 'tx' + >>> persp["tx"].name(full=True) + 'translate.translateX' + >>> persp["tx"].name(long=False, full=True) + 't.tx' + + """ + + return "{}".format( + self._mplug.partialName( + includeNodeName=False, + useLongNames=long, + useFullAttributePath=full + ) ) def read(self, unit=None, time=None): -- 2.28.0.windows.1 ```

Save that file, and then:

$ cd cmdx
$ git apply 0001-Reduce-default-verbosity-of-attribute-names.patch
wougzy commented 4 years ago

omg i completely missed that "pipe" character in the logs. i forgot i modified the node path part of this function and i thought it was safe. a brilliant sample of why unit test is a good practice..

mottosso commented 4 years ago

2020.3 was released yesterday with an interesting addition MDGModifier.deleteNode(includeParents=True), I wonder if this could address the issues you were seeing? :O

Not much additional info about why it was added.

wougzy commented 4 years ago

haha thanks autodesk... it looks like this is precisely the problem i had with the modifier

when i run this sample on maya 2018. everything is deleted...

parent = mx.create_node(mx.tTransform)
child = mx.create_node(mx.tTransform, parent=parent)

with mx.DagModifier() as md:
  md.delete(child)

it also explains why it is so difficult to delete shapes

wougzy commented 4 years ago

while we're at it, what do you think about updating DagNode default string representations?

    def __str__(self):
        return self.shortestPath()

    def __repr__(self):
        return self.shortestPath()

the point is to stick to how maya print nodes by default. i don't know if it has a performance impact. this is something i modified in my wrapper anyway and which is a concern for me as i use this behaviour a lot to verbose data. I have a very biased vision on the subject because we often work with very deep hierarchies in rig and you see what i mean this is hard to read (:

i'd also suggest to change repr of Plug to be more pythonic

    def __repr__(self):
        return self.path(full=False)
mottosso commented 4 years ago

while we're at it, what do you think about updating DagNode default string representations?

Hm, yes I can see what you mean. I would leave __repr__ with the full path, but use the short path for __str__. Would that work for you?

wougzy commented 4 years ago

yes that's it! i believe that's what most tds would expect as it's the default Maya behaviour. I'll see that tomorrow quietly, it looks like there is a lot unit test that depend on it

speaking about that i may have a last small question about default stuffs. I noticed the default keyable state of new created attributes was True. It is False by default in mel and cmds. I couldn't find what was the default setting of the api. So was it done on purpose?

mottosso commented 4 years ago

I noticed the default keyable state of new created attributes was True

You're right, that sounds inconsistent with the API as well.

Happy to switch that default to False.

mottosso commented 4 years ago

Let me know when you're happy @wougzy and we'll get this merged. If something isn't quite perfect yet, let's save it for another PR where we can focus in on just that, before this PR gets too big.

wougzy commented 4 years ago

haha right. i think i'm done with theses ones yeah. i'll leave the str changes. i don't want to interfere with the unit test and i can deal with that in wrapper anyway

mottosso commented 4 years ago

Whop! Before this PR gets too big, let's make it happen!

mottosso commented 4 years ago

🥳