mottosso / cmdx

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

Plugs not taking and returning UI units #21

Closed monkeez closed 4 years ago

monkeez commented 4 years ago

Hey, whilst implementing the AnimCurve updates (almost done), I noticed this bug with the way plugs are read and written.

Under the Units section of the README it mentions that cmdx takes and returns values in units used by the UI, but this doesn't seem to be the case. It looks like it's currently using Maya's internal units by default. You can confirm this by running this (with Maya UI set to degrees):

node = cmdx.createNode(cmdx.tTransform)
node["rx"] = 5
node["rx", cmdx.Degrees]
# Result: 286.478897565 #

I believe I know how to fix this and am happy to do it, but it will be a bit of work so I'm just checking that using the UI units is the intended functionality before I start?

It seems like using the UI units would make the most sense.

mottosso commented 4 years ago

Eeeek, yes. You are right. They differ, and that's bad. I've attempted a fix in exactly the way you propose, dealing with UI units only since that's both expected (and I think also what you get with cmds?).

However, there's a subtlety!

Picture having written this code.

node["rx"] = 5

Which cmdx interprets as degrees because that's what the UI has been set to. Your code depends on a 5 degree angle between node and otherNode because maybe you're doing physics simulation or IK solving or what not. Then the user changes UI units to radians.

Suddenly, your code has changed. Your simulation and IK has changed. IK would flip, as it's past 180 degrees and physics would encounter a massive change in angle over that one frame or whatever.

So, I think what we want is for both reading and writing of plugs to happen in a consistent unit. Which unit? The API, both Python and Maya, uses radians for angles hence cmdx does too. It's helpful for when you pass values between API and cmdx. For distances, it's centimeters, which conveniently happens to be the default UI unit as well.

At least that's my rationale. It's not immediately obvious, which I agree is bad. And it's bit me a few times. But I haven't figured out an alternative, how to better balance the principle-of-least-astonishment for both first-use and long-term reliability.

The "solution" at the moment is..

node["rx"] = cmdx.radians(5)

Although if there's one thing we could fix it would be writing to plugs with that same unit qualifier which is something I would like to see.

node["rx", cmdx.Degrees] = 5

Under the Units section of the README it mentions that cmdx takes and returns values in units used by the UI

That's a bummer, it must have been written during my initial implementation of this mechanism, before I realised why it cannot be. That should also be updated.

Thanks for highlighting this, haven't really spoken it out loud before but it still bothers me on a weekly basis so if you think of a cure, this would be a good time to get it in.

monkeez commented 4 years ago

I feel like both UI units and internal units have their merits.

cmds does use UI units so people used to that may be confused initially. Whereas the issue about the user changing the UI units and getting wildly different results is alarming (though I've never experienced it myself using cmds)

I think it's one of those things where there isn't a right solution, just need to pick one and stick to it. Using Maya's internal units would probably require less code so would hopefully be easier to maintain (plus it already works this way). It's really up to you, I'm happy with either way :).

Also, I tried running this and it seemed to work as expected so I'm not sure if I'm missing something?

node["rx", cmdx.Degrees] = 5

Plug.write() doesn't accept a unit though.

mottosso commented 4 years ago

I tried running this and it seemed to work

Oh, yes you're right. Brainfart.

I think it's one of those things where there isn't a right solution, just need to pick one and stick to it.

Yes, in that case, let's stay close to the Maya API. The only one I can spot that isn't doing this right now is time.

cmds.currentTime(1)
t = cmdx.encode("time1")
t["outTime"].read() == 1  # Ui Unit
t["outTime"] = 1  # Seconds
t["outTime"].read() == 24

Which I'll fix now since it's low-hanging fruit.

monkeez commented 4 years ago

As mentioned in #23 I discovered that Maya uses UI units for setting and getting MTime in MFnAnimCurve. I'm not sure if it changes how I feel about this issue but I thought it worth mentioning.

I'm wondering if there's any other areas where Maya is inconsistent with units.