Center 2D Offset #209
Center 2D Offset #209
Conversation
python/mmSolver/utils/reproject.py
Outdated
| offset_plus_minus_node = maya.cmds.createNode('plusMinusAverage', name='offset_plusMinusAverage1') | ||
| maya.cmds.connectAttr(cam_shp + '.filmOffset', offset_plus_minus_node + '.input2D[0]') | ||
| maya.cmds.setAttr(offset_plus_minus_node + '.input2D[1]', 0.0, 0.0, type='float2') | ||
| maya.cmds.connectAttr(offset_plus_minus_node + '.output2D', node + '.filmOffset') |
There was a problem hiding this comment.
Rather than using "filmOffset" on the camera, I think it's better to use the "pan" attribute, so the connection becomes:
mmReprojection.outPan -> offset_pluseMinusAverage1.input2D[1]
offset_pluseMinusAverage1.output2D -> cameraShape.pan
The reason for this is because users may want to solve the filmOffset values on the camera, or it may already contain some values that need to be maintained.
python/mmSolver/utils/reproject.py
Outdated
| zoom_mult_node = maya.cmds.createNode('multiplyDivide', name='zoom_multiplyDivide1') | ||
| maya.cmds.setAttr(zoom_mult_node + '.input1X', 1.0) | ||
| expression_string = '{zoom_mult_node}.input2X = 1/{zoom_mult_node}.input1X;'.format(zoom_mult_node=zoom_mult_node) | ||
| maya.cmds.expression(string=expression_string, object=zoom_mult_node) |
There was a problem hiding this comment.
I know it's tempting to use an expression and I know it will work, however I'd prefer to avoid expressions because they will slow down the Maya scene evaluation and stop parallel evaluation. Another reason is that expressions are only evaluated when the user changes frame, but node connections will evaluate immediately.
It should be possible to replace the 1/{zoom_mult_node}.input1X with yet another "multiplyDivide" node with a "divide" operation to provide the same output value.
…nd now starting to implement a UI
|
Hey @david-cattermole, so an update to this: I moved around some stuff and now starting to actually incorporate an UI in this commit: 770bd4c I basically just elected to add to the existing |
|
Hello @ktonegawa, Your latest commit looks good to me.
Yes, the .ui file will be converted to a If you copy the line for the smoothkeys UI and replicate it for your UI it should work. mayaMatchMoveSolver/python/CMakeLists.txt Line 66 in e765edc As long as you've added the entry into the CMakeLists.txt file it should be picked up. If not it makes me think something is not correct with the way you're calling CMake. As a side note, I will be deprecating the "Reset" and "Help" buttons in favor of an "Edit > Reset Settings" and "Help > Tool Help..." menu (#206), it's a trivial change, and I can make the change after your code is pulled. Let me know if I'm not making any sense or if you still have problems with the .ui / .py file generation. David |
Arrrrgg how did I miss that? I guess when I previously submitted the |
Yeah, I think I did now you mention it. It's not a big deal :) |
|
Hey @ktonegawa, Am I holding you up on this at all? David |
|
Thanks for following up. Sorry for the silence, I've been extremely occupied the last couple weeks. I am still working on this, mostly just working to get the UI to work. |
|
Hey @ktonegawa, There's no problem, no rush. David |
|
Hi @david-cattermole , this is now ready for an initial review. The way this is set up is sort of weird but I tried to keep the dependency relationships similar to that of the Smooth Curves tool. The basis of this offset window is that if there are no offset nodes available, this window pops up but it does nothing. But if it does exist it will load the window with the sliders set at the corresponding values as the offset/zoom nodes. What's remaining now is documentation for this (if required). |
…ll more that I have not adjusted yet)
|
Hello @ktonegawa, Thank you very much, sorry I've been really busy. The tool's logic you are explaining sounds good to me. David P.S. Your "Remove Solver Nodes" tool is amazing! It's a very welcome tool. Thank you so much for contributing. |
Thank you! And the "Remove Solver Nodes" I am still somewhat unsatisfied with since I wanted to add some other features like automatically baking any nodes parented under or constrained to the bundles. I do plan on adding these in but not sure when I should do that. |
The zoom offset was removed. Adding the zoom offset stopped the Pan/Zoom tool from being able to drag in the viewport to zoom in/out. Zoom slider now directly changes the camera zoom. The slider to value remapping has been simplified. Moved some "tool.py" functions to "lib.py" because they were not intended for users to run, and they should be "hidden away". Changed the default slider range from "1-100" to "0-100", so that "50" is the middle value. Removed unneeded use of "kwargs", because it can be replaced with keyword arguments on the function. (When the keyword arguments are in the function definition users do not need to read the function source code to know what values can be used with the function.)
|
Hello @ktonegawa, I have tested out your branch and found a few issues.
I hope you don't mind, I started looking at these issues and I eventually attempted to address them in my commit c356ad3. To address 1), I removed the "zoom offset" and connected the zoom slider directly to the Camera shape node's To address 2) I found that Maya does not update properly when connecting to the parent attribute plug ("pan"), but it works perfectly well if you connect to "horizontalPan" and "verticalPan" individually. To address 3) I have changed the UI to use slider range 0 to 100, so that "50" and default values are zero for the pan offsets, but for zoom it's slightly wrong 1.005. To address 4) I reduced the range of the values. This isn't a perfect solution however because I realized that the "pan" values are in inches (like the camera "film offset" values), not -1.0 to +1.0. For now I decreased the range to -0.5 to +0.5. I think an ideal solution would be use the camera horizontal and vertical film aperture attributes to normalize the values. For 5) and 6) I made some changes to the code. Also, rather than the complicated "convert_range()" function, I introduced a What do you think about my changes? Feel free to revert my commit if you prefer. The only major existing problem I can see is 4) because 4) might affect the "offset nodes" that need to be created, and I'd prefer not to change that in a future version of mmSolver because it will be complicated to detect and fix the node network afterwards. Other than 4) I am very happy to release this, it's fantastic work and a great tool. |
|
Hey @david-cattermole, apologies for making you do so much refactoring, far from it being "perfect". I don't think I ever noticed that behaviour with pan not updating until the frame is changed, so it's great you caught that. Some questions from what you raised: I) How were you able to "simplify" the remapping functions? What is the difference between what I had and these "lerp" methods you've introduced...? II) Should I have the assumption now that if a method is to be "hidden" from the user, it should be contained into a lib.py as much as possible? III) How would your range changes for the pan affect the created offset nodes...? Not sure if I fully understood what you were referring to. Also I would be cautious as what other attributes to affect of a camera shape node since certain workflows may be using it to create actual 2d offsets like camera shake effects to be used for Comp... Otherwise I have no issues to the changes you made, this is all super informative for me. |
|
Hello @ktonegawa, Don't worry, I had some good fun looking at the tool and trying the new tool out.
Your
Yes, mostly. I generally try to only keep 1 or 2 functions in It's a very subjective thing. Maybe I'm going over-board with the "tool.py" vs "lib.py" - I'm not sure.
The slider in the UI goes from 0 to 100. Right now, the slider values are remapped from: However that is only 0.0 inches to 0.5 inches. That means your offset tool will have a different screen-space offset for smaller film back cameras compared to larger film back cameras. Therefore I think it's a good idea to normalize the offset values to be 0.0 to (0.5 * film back size). And finally, I'd prefer to add the extra David |
|
Oh I see, I guess re-looking at both And for the pan min/max issue I see now what you are saying. I am always happy to work on these additions, although I am currently not very sure why another node would be required to just seemingly set a new I would be able to add this in if that part is cleared up, but if you feel it would be faster on your end then by all means. Then after you pushed your commit I can see what it is you were describing. |
|
Hello @ktonegawa,
You are exactly correct, we only need to set a new PAN_MAX value! I don't know what I was thinking. When the user opens the UI, the horizontal and vertical film aperture should be queried, halved and used as the PAN_MAX value. If you want to be tricky, you can use a different PAN_MAX value for vertical and horizontal, or to make things easier, simply take the larger of both values. That sounds simple, and there's no need for an extra node (which makes things easier)! Are you ok to make this change? David |
…e relative to the horizontal and vertical aperture as per conversation with David
|
@david-cattermole made a few changes to address what you just said. |
|
It looks good to me. I think we can consider this completed. Thanks @ktonegawa :) |
This addresses issue #135
Creating this PR as a means to start a discussion on implementation methods of this feature.
Based on the conversation I had with @david-cattermole, this solution just adds an additional
plusMinusAveragenode and controls thecameraShape.filmOffsetattribute to handle the X and Y offsets. I have also added additional nodes to allow for zoom controls using thecameraShape.zoomattribute, just to keep it in line with the Center 2D tool also using the Camera Pan feature.At this time of writing, several things needs to be worked out:
centertwodeetool to keep thereproject.pyutility relatively clean