implement MDagPath

[Muream]

This is not ready to be merged yet. I'm making the PR for visibility, and because I had a few questions.

My questions:

1. Are we aiming to be more consistent with the C++ API or OpenMaya2?
   DagPath.extendToShape() returns a reference to self in OM2 but not in the C++ API.
   I feel like not returning anything would make more sense since self is already modified by the method
2. Do we have a standardized format for the error messages?
   I've just copied OM2's messages so far.
3. I was expecting exclusiveMatrix and exclusiveMatrixInverse to fail when called from an invalid DagPath but they return an identity matrix instead.
   This seems consistent with OM2 but I'm not sure why they're not raising a RuntimeError where something like childCount does with a similar implementation

[mottosso]

Are we aiming to be more consistent with the C++ API or OpenMaya2?

Good question. There are subtle and not-so-subtle differences between the two, some of which I don't understand.

# C++
MTransformationMatrix::addTranslation()
MTransformationMatrix::rotation() -> MQuaternion

# Python
MTransformationMatrix.translateBy()
MTransformationMatrix.rotation() -> MEulerRotation

# But whyyyy?

They've made a few creative choices here, presumably to make it more user-friendly but it's so subjective..

I think we should stick to the C++ documentation as closely as possible and leave creative choices to the user. When that is not possible, such as..

// C++
MDagPath dagPath;
MFnDagNode(mobj).getPath(dagPath);

..where it's modifying a variable from a function rather than return a value (classic C++), we'll need to make some less-creative choices that I expect to become a predictable pattern anywhere this happens in C++.

# Python
dagPath = MFnDagNode(mobj).getPath(dagPath)

In this case, the reason they're doing it the way that they are in C++ is because they choose not to throw exceptions. Instead, they return a MStatus value expecting the user to handle it.

MDagPath dagPath;
MStatus status = MFnDagNode(mobj).getPath(dagPath);
if (status == MS::kSuccess) {
  // It's ok to use dagPath
}

With Python, I expect we'll be able to completely replace MStatus with exceptions.

Do we have a standardized format for the error messages? I've just copied OM2's messages so far.

Let's start there. I think it's hard to know until we have enough of a foundation to actually start using cmdc for actual code and experience what messages make the most sense given the context. At this point, some message is better than no message.

I was expecting exclusiveMatrix and exclusiveMatrixInverse to fail when called from an invalid DagPath but they return an identity matrix instead.

Yes, the API loves this. It's perfectly happy letting you continue working with bad memory and chooses to randomly crash on you whenever it feels like it instead. This is one of the things I'd like cmdc to get rid of. It's a bad habit.

In this case, if the API isn't returning a bad MStatus, but we know for certain the value is bad, we should throw an exception ourselves to prevent the user from experiencing a crash.

[Muream]

Nice, that all sounds good!

It might be worth creating a CONTRIBUTING.md to keep track of these design decisions

[mottosso]

It might be worth creating a CONTRIBUTING.md to keep track of these design decisions

Good idea!

[Muream]

I've gotten rid of any MStatus checks here, It looks more consistent between methods that return a relevant status and those that don't.
However, this opens the problem where we might not think of checking all the ways that might make a method fail.
Maybe switching on the status after the manual checks to throw anyway on any unhandled errors would be a good way to go?

Since I'm not using MStatus anymore, I've changed the error message and reworded them to something I felt was more informative.

[mottosso]

I think we should still test it, just in case there is some edge case whereby it does actually fail. We could test both, would be no harm in that.

[mottosso]

I've gotten rid of any MStatus checks here

Bah, looks like I commented on an old commit. But anyway, the MStatuses may not always return whether it fails, but it might sometimes. So we should probably still check it, alongside our extra checks. What do you think?

[Muream]

Yeah, that sounds good to me

[Muream]

Not quite done yet but I've implemented most methods now.
For the error handling, I added checks to the returned status for almost everything except apiType because it returns kInvalid when it fails.

A few questions:

1. The method getDisplayStatus is implemented in OpenMaya 2 but not in the C++ API, I'm guessing it's fine to ignore it?
2. I couldn't figure out what pathCount is supposed to do, seems to always return 1. Any clue?

[mottosso]

The method getDisplayStatus is implemented in OpenMaya 2 but not in the C++ API, I'm guessing it's fine to ignore it?

There's lots of things missing in OM 2. I personally think we should include as much as humanly possible. I run into things missing in OM 2 far too often, many do, and it's one of the motivations behind writing a new binding.

We don't have to implement everything at once though, so feel free to leave it for now if it slows you down.

I couldn't figure out what pathCount is supposed to do, seems to always return 1. Any clue?

I think this has to do with instances. Try making an instance of a transform, and it should return 2.

[Muream]

There's lots of things missing in OM 2. I personally think we should include as much as humanly possible. I run into things missing in OM 2 far too often, many do, and it's one of the motivations behind writing a new binding.

The function is actually there in OM2 but not it the C++ API haha

I think this has to do with instances. Try making an instance of a transform, and it should return 2.

That would make sense, I'll test that

[mottosso]

Oh haha, I misread. :D I could have sworn this was in the API because I'm using it all the time, but turns out it's actually called MGeometryUtilities::displayStatus(dagPath).

Maybe they moved that there, because it isn't exposed from it's original place? I haven't used it from Python before.

That's an interesting point actually. I'm hoping we're able to avoid that, and actually expose it where it is in the C++ API rather than move stuff around. E.g. cmdc.GeometryUtilities.displayStatus(dagPath). If not, we might be forced into making some of these creative choices ourselves, and that would be unfortunate. :(

[Muream]

Not 100% sure I like that but how about "implementing" the method but just raising an error saying cmdc.GeometryUtilities.displayStatus(dagPath) should be used instead?

Main pro is that it helps people transition from OM2 easier
But the con is that the existence of the method is misleading and people will be drawn to use it until they realize it just throws an error

Again, not really convinced by this, just throwing the idea out there

[mottosso]

I think we should stick to the C++ docs, that will be the beacon of light. Users with experience with the C++ API will be familiar with cmdc, and users of cmdc will become familiar with the C++ API.

[mottosso]

As soon as you're happy @Muream I'm happy to merge this and carry on!

[Muream]

I've implemented s few more things during the week but the rest seems pretty low priority so I'm happy to leave that out yeah
I'll push all that when I'm on the computer 👍

[Muream]

A few notes on what I've done and omitted:

• I still couldn't figure out pathCount, even instances so I left it unimplemented rather than implemented but untested.
• I've added all the missing C++ methods but left them unimplemented
• I have left outclassName on purpose because it would have returned MDagPath instead of DagPath and python already has obj.__class__.__name__ anyway

Wasn't sure if I should have squashed all the commits or rebased the branch against master so I haven't done any of that. Let me know if you need me to do more work before merging this :)

[mottosso]

Good call.

I have left outclassName on purpose because it would have returned MDagPath instead of DagPath and python already has obj.class.name anyway

Good point!

Wasn't sure if I should have squashed all the commits or rebased the branch against master so I haven't done any of that. Let me know if you need me to do more work before merging this :)

I think this looks good, great work. :)

[mottosso]

I've made it so that new tags generate a Maya module for all platforms and all Maya's.

Can give it a try here:

• https://github.com/mottosso/cmdc/releases

[Muream]

Nice, I only have 2022 installed but that loaded nicely there
The only thing is the zip file doesn't seem to be named properly. The one for 0.1.1 says 0.1.0

[mottosso]

Yeah, me and GitHub Actions had an argument last night, and it won. Next time. :)