search

david-cattermole / mayaMatchMoveSolver

A Bundle Adjustment solver for MatchMove related tasks.
https://david-cattermole.github.io/mayaMatchMoveSolver/
Other
101 stars 29 forks source link

Adding "Remove Solver Nodes" tool #181

Closed ktonegawa closed 3 years ago

ktonegawa commented 3 years ago

adding Remove Solver Nodes tool in an attempt to tackle this issue: https://github.com/david-cattermole/mayaMatchMoveSolver/issues/76

This is a PR created to start the conversation of how to proceed next as this is far from finished.

This tool still requires:

david-cattermole commented 3 years ago

Proper Doc-strings on all functions

Your functions are not super complex, so I'm not worried about this. The most important doc-strings are the ones the user will call, or the ones that will appear in the Sphinx documentation (https://david-cattermole.github.io/mayaMatchMoveSolver/mmSolver.tools.html).

Possibly its own section in the tools.html page

There are two sides to that; 1) Code documentation (generated automatically from the doc-strings). 2) User documentation (manually written).

Here is an example of automatic documentation: https://github.com/david-cattermole/mayaMatchMoveSolver/blob/master/docs/source/mmSolver.tools.channelsen.rst

This is an example of manually written documentation. https://github.com/david-cattermole/mayaMatchMoveSolver/blob/master/docs/source/tools_generaltools.rst

Which is online here: https://david-cattermole.github.io/mayaMatchMoveSolver/tools_generaltools.html

Adding images is a bit of a pain, but it can be done.

A test file written (but I am not sure how to write that)

I don't think we need to worry about unit tests for this tool.

david-cattermole commented 3 years ago

There is another part to this, where will the tool exist on the shelves/menus? I would suggest your tool goes under the "General Tools" sub-menu?

The mmSolver menus and shelves are auto-generated using some .json configuration files.

I have added this README file to explain some of the details: https://github.com/david-cattermole/mayaMatchMoveSolver/blob/master/config/README.md

If you don't feel comfortable doing this, I can add it in?

ktonegawa commented 3 years ago

Hey David, thank you so much for your feedback!

I submitted a new commit with some changes to address things you have suggested. Please let me know what you think. I am still not sure what to do on the documentation part...

https://github.com/ktonegawa/mayaMatchMoveSolver/commit/4c7a7d07d21f911a7eac6c225c40c55e87e45071 https://github.com/ktonegawa/mayaMatchMoveSolver/commit/9a378b209673d3a7784efbd2910191d702265e75

david-cattermole commented 3 years ago

Thanks @ktonegawa! These changes look fantastic.

There is only one last small thing.

Can you move these functions into a new "lib.py" file?

Functions needing to be moved:

The functions should all move to a new module mmSolver.tools.removesolvernodes.lib, for example mmSolver.tools.removesolvernodes.lib.filter_nodes()

My reasoning for this change is so that (in the future) we can easily find and delete the nodes without the need for a GUI call. The lib submodules in mmSolver are all meant to be independent of the Maya GUI, so that if we want to write unit tests, we can test everything in the lib sub-module without opening the Maya GUI.

Other than that change I think this can be approved and we can merge.

For documentation I am using Sphinx, which uses "ReStructuredText" markup language (.rst file extension). It's the same format used in doc-strings. https://docutils.sourceforge.io/docs/user/rst/quickref.html

The doc-strings from the Python module can be converted into HTML automatically. I can add this in fairly easily after we merge, and I can send you the commit so you can see the change I made.

However for end-user documentation, it is written (in ReStructuredText), like this: (GitHub automatically "renders" this link) https://github.com/david-cattermole/mayaMatchMoveSolver/blob/master/docs/source/tools_generaltools.rst

The raw text can be found here: https://raw.githubusercontent.com/david-cattermole/mayaMatchMoveSolver/master/docs/source/tools_generaltools.rst

Edit the .rst file and it will automatically generate the website. To test locally, you should be able to run Sphinx manually on your local computer by 'cd'ing into the "mayaMatchMoveSolver/docs/" directory and running "make" (Linux or Windows). This will use your currently installed Python install, then read the .rst files and build the documentation. Load the generated documentation by opening mayaMatchMoveSolver/docs/build/html/index.html in a web browser.

david-cattermole commented 3 years ago

Actually.... I noticed I didn't make that comment about the lib.py file before, so don't worry about it.

I'll merge this request in in a few hours.

Thanks again!

ktonegawa commented 3 years ago

Hey David, I made another PR specifically for the documentation portion: https://github.com/david-cattermole/mayaMatchMoveSolver/pull/189