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

Add an op for creating geometry #14

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

mishurov
Copy link

Hi!

I've created an additional op based on SourceGeo for models that create geometry from images. I moved the code related to the client-server interactions into a mixin. Unfortunately I hadn't found an elegant way avoiding using templates yet I'd separated template implementation into another file so it doesn't increase compilation time.

As an example of usage I've added a TensorFlow Lite model. The file sizes of the models I use are small, so they can easily fit into the repository without needing to download from external resources. The only caveat is that the model was extracted from Google's ARCore .apk file, so I don't know if it's permissible to use it in open source projects.

Here https://ai.googleblog.com/2019/03/real-time-ar-self-expression-with.html

In the article they also mention some "windowed smoothing", the term is quite broad. I didn't implement anything thus the animation can be noisy. A Kalman filter or so probably may be implemented in the server side code.

I also a bit confused about "get_geometry_hash()" it's quite possible that I'm using it wrongly in the code.

There're were also issues with using protobuf simultaneously in two shared libraries.

Some pictures to make the pull request more fancy.

Screenshot_2019-06-19_22-48-38

Screenshot_2019-06-19_22-41-12

@ringdk
Copy link
Contributor

ringdk commented Jun 24, 2019

Hi @mishurov!

First off, this is absolutely amazing. Thank you for taking the time to put this together. We're going to review it over the week and pull it in.

Secondly, I think you're right about using data extracted from the .apk. We're aiming to be fully license compliant, for both ourselves and especially anyone who wants to use this project and so we won't include any extracted data where the usage license doesn't permit it or is a bit grey. However there are usually solutions to this and we can start investigating those.

Thanks again, I'm looking forward to playing with this!

@mishurov
Copy link
Author

@ringdk thank you, that's inspiring. The code is within limits of my understanding of the API and so forth, so I wouldn't even pretend that it is an ideal solution, more like a draft. The neural network I've included is tremendously convenient for debugging due to its modest requirements yet it doesn't actually matter if there would be any other network which takes an image and returns geometry.

@mateuszwojt
Copy link

Hi @mishurov I have couple of issues related to your code.

MLClient/MLClientGeo.cpp:89:19: error: cannot convert ‘DD::Image::GeoOp*’ to ‘DD::Image::Iop*’ in return
     return input1()->iop();

I believe you should use dynamic_cast, since GeoOp doesn't have a iop() method:

Similar issue one line below that:

MLClient/MLClientGeo.cpp:90:51: warning: invalid conversion from ‘DD::Image::Op*’ to ‘DD::Image::Iop*’ [-fpermissive]
   return MLClientMixin<SourceGeo>::default_input(0);

Apart from that, I'm having an error while accessing elements from protobuf::RepeatedField

MLClient/MLClientGeo.cpp:228:33: error: no match for ‘operator[]’ (operand types are ‘const google::protobuf::RepeatedField<float>’ and ‘int’)
           item[j] = (*attr.data)[i * dim + j];

Can you tell me which version of protobuf lib are you using to compile the plugin?

@mishurov
Copy link
Author

mishurov commented Aug 28, 2019

@GreenEminence the parent class Op has the method

virtual Iop * | iop ()
Cast to an Iop. This is much cheaper and safer than using dynamic_cast.

https://learn.foundry.com/nuke/developers/113/ndkreference/Plugins/classDD_1_1Image_1_1Op.html#a24e0d3db1f380929ed86e4a0a5913c1d

I was compiling the code against Nuke 11.3v1, may be something had changed in the API.

I was building Protobuf from source as the README recommends.

Protobuf (tested with 2.5.0 and 3.5.1)

Unfortunately I can't tell now which version of those two I was using because I've moved to a new Debian since then and I lost my intermediate files. Protobuf has another issue as well, since the code generates two different shared libraries which use the same generated C++ file, there were problems with loading the binaries simultaneously, I had to add option optimize_for = LITE_RUNTIME; into message.proto, it is mentioned in the comments in the file, it was the only workaround I'd found.

P.S. There was also sort of annoying thing with gcc which was hard to figure out. If you're compiling on Linux, you may try to set the -D_GLIBCXX_USE_CXX11_ABI flag to 0 or 1 in CMake so as to match the ABI of Nuke's binaries. E.g.

set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -D_GLIBCXX_USE_CXX11_ABI=0")

@mateuszwojt
Copy link

Hi @mishurov

Thanks for replying! I never managed to make it work using iop() call, so I decided to update the code with dynamic_cast for now. Thanks for the hint about the Nuke version, we're running an automated build process which compiles plugins for all major Nuke releases and I didn't notice any difference between Nuke 11.2 and 11.3.

Protobuf is another story - I'm not convinced it actually works with 2.5.0, I tried with 2.6.1 and couldn't make it work. I'm currently building 3.6.1 to see if it's going to help (matching the version that Houdini's currently using). I'll get back to you with the results.

Cheers!

@mishurov
Copy link
Author

Cheers @GreenEminence!

The initial motivation for the pull request was to point out the issue of returning geometry data from the server and to propose a draft solution. I was developing it on a single Linux machine and I frankly don't know how it would scale to industrial levels on different platforms and versions due to my limited personal resources. I hope Foundry will come up with some elegant solution after all.

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