Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implment MDagModifier bindings #22

Merged
merged 15 commits into from
Jun 13, 2021

Conversation

yantor3d
Copy link
Contributor

@yantor3d yantor3d commented Jun 6, 2021

Added

  • Implement MDagModifier bindings

Fixes

  • Fix typo in build_win32.ps1 so MFn.Types.inl is cleaned up after each build

@yantor3d
Copy link
Contributor Author

yantor3d commented Jun 6, 2021

I thought implementing MDagModifier was going to be a piece of cake, but I immediately ran into a problem.

MDagModifier is a subclass of MDGModifier. However, I can't figure out how to subclass an existing binding. The official docs don't seem to say, and this issue suggests a way that feels kind of hacky...

If you Google "pybind11 subclass" you will find a couple of threads
on how to solve the problem, none of which worked in this scenario.

The issue on the pybind github about subclasses (below) cites a way
to subclass an existing binding that feels as robust as creating
a class on the fly using type("MyClass", bases, attrs)
 -> pybind/pybind11#1193

Another issue recommends using py:base, which has been deprecated.
 -> pybind/pybind11#17

...also it didn't work.

It seems to me that because we are defining each class as an .inl rather
than a .hpp or .cpp file, they are not aware of each other. To remedy
this, I've included the MDGModifier binding in the MDagModifier file,
with an pre-processor to prevent duplicate entries. Otherwise, the base
class would be omitted in the main.cpp.

I think this solution will work well when we get to other classes with
many subclasses, like the whole MFn* tree.
@yantor3d
Copy link
Contributor Author

yantor3d commented Jun 7, 2021

After a lot of googling I have concluding that none of the examples out there work when using .inl files.

I did eventually figure out a solution that works - wrapping the base class in an ifndef/define/endif preprocessor and including it before the subclass definition. However, I but I don't know if this solution will work for classes with multiple subclasses. We'll burn that bridge when we get to it.

@yantor3d yantor3d marked this pull request as ready for review June 7, 2021 03:02
@Muream
Copy link
Contributor

Muream commented Jun 11, 2021

@yantor3d quick heads up, I've changed the format of the docstrings in my last PR that's now been merged, you may get conflicts there

We now also have automatically generated stubs, if that step fails during CI, make sure that:

  1. The docstrings in your .inl files don't contain any signatures
  2. that your classes have been declared in ForwardDeclaration.inl before they're used in any signatures. Those classes don't necessarily need any implementation, they just need to be declared for pybind to be able to map the C++ type to the Python type.

A few other things might trip you so if you run into any problem, give me a ping.

I'll add those notes in the contributing.md as well during the weekend

@mottosso
Copy link
Owner

Great work @Muream. Would it be possible to add a note to the README as well, along with a short gif demonstrating the final result in any of the editors you are using it in?

@Muream
Copy link
Contributor

Muream commented Jun 11, 2021

Yep I can absolutely do that

@yantor3d
Copy link
Contributor Author

Thanks for the heads up! I'll take care of the merge after work.

@yantor3d
Copy link
Contributor Author

Stabalized and ready for merge

@mottosso mottosso merged commit b0a4cdb into mottosso:master Jun 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants