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
Docs: Edit/rename "Getting Started" tutorial → "Tutorial: Assembling the Gaffer Bot" #2629
Docs: Edit/rename "Getting Started" tutorial → "Tutorial: Assembling the Gaffer Bot" #2629
Conversation
b5517ea
to
42b0cfc
Compare
ed9cd2a
to
d55372a
Compare
Rebased and removed dependency PR. |
7d8dc38
to
ffc2e7d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @medubelko ! I like the new tutorial a lot. I've made quite a few comments, but most are showing as out-of-date because the later commit renames the file... In the future, it'd be better to do the git mv
prior to making copy edits if possible.
Also, when you address the feedback, do it by pushing new commits (rather than squashing into the existing ones). Otherwise we'll lose all the comments and have to start the review process from scratch. When we're all done with the review we can decide whether its worth squashing down the results.
> here can be substituted with direct equivalents for Arnold or 3delight, but we do | ||
> recommend that you complete the tutorial using Appleseed before flying solo with your | ||
> renderer of choice. | ||
By the end of this tutorial you will have built a basic scene with Gaffer's robot mascot, Gaffy, and render an output image. You will learn the following lessons, not necessarily in this order: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is Gaffy the official name of the robot? If not, here are some alternate (equally goofy) proposals:
- Best Boy
- CLT (Chief Lighting Technician)
- Hamfast (look it up)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried to make up something endearing that was self-explanatory... and OK, Hamfast is a brilliant reference, but I don't think general audiences will get it. :(
- Importing textures | ||
- Building a simple shader | ||
- Applying a shader to geometry | ||
- Creating an environment light |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe just "Creating lights" ?
|
||
![Default layout](images/defaultLayout.png) | ||
After [installing Gaffer](../InstallingGaffer/index.md), launch Gaffer [from its directory](../LaunchingGafferFirstTime/index.md), or using the ["gaffer" command](../SettingUpGafferCommand/index.md). Gaffer will start, and you will be presented with an empty graph in the default UI layout. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Dangling sentence here... should it be a comma?
|
||
![Empty SceneReader](images/emptySceneReader.png) | ||
As Gaffer is a tool primarily designed for LookDev, lighting, and VFX process automation, we expect that your shot's modelling and animation will be created in an external tool like Maya, and then imported into Gaffer as a geometry/animation cache. Gaffer supports the standard Alembic (.abc) and USD (.usd) file formats, as well as its own native SceneCache (.scc) file format. Most scenes begin by importing geometry or images via one of the two types of Reader nodes: [SceneReader](../../Reference/NodeReference/GafferScene/SceneReader.md) or [ImageReader](../../Reference/NodeReference/GafferScene/ImageReader.md). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we're capitalizing LookDev, we should capitalize Lighting as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Technically, we support variants of .usd (.usdc and .usda)
- Locate the NodeEditor in the top right pane. | ||
- Enter `${GAFFER_ROOT}/resources/gafferBot/caches/gafferBot.scc` into the **File Name** field. | ||
- Move the mouse into the [Viewer][4] in the top left pane and press 'f' to frame the full scene. | ||
1. In the _Graph Editor_ in the bottom-left panel, right-click. The node creation menu will appear. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we need a formal name for the "node creation menu"
script["SceneReader"] = GafferScene.SceneReader() | ||
script.selection().add( script["SceneReader"] ) | ||
GafferUI.WidgetAlgo.grab( widget = scriptWindow, imagePath = "images/emptySceneReader.png" ) | ||
readerNode = script["SceneReader"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the downside of storing this in a variable is you can affect the lifetime of the node. if you load a new file onto the same script node, this SceneReader should die, but it won't unless you also explicitly del readerNode
. Generally, its better practice to refer to the node by indexing into the script unless you have a good reason not to.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, OK. I'd like to make the screenshot scripts as clear and readable as possible. By "indexing into the script", do you mean using script["SceneReader"]
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yep
GafferSceneUI.ContextAlgo.setSelectedPaths( script.context(), IECore.PathMatcher( [ "" ] ) ) | ||
GafferUI.WidgetAlgo.grab( widget = scriptWindow, imagePath = "images/fullyExpanded.png" ) | ||
#GafferUI.WidgetAlgo.grab( widget = scriptWindow, imagePath = "images/mainSceneFullyExpanded.png" ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why is this one commented out?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Last minute removal; not sure if I'll reuse it later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in general, we would remove the code completely. you always have it in the history of the file if you need to recover it.
script["Camera"] = GafferScene.Camera() | ||
cameraNode = script["Camera"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same comment regarding node lifetime
script["Group"] = GafferScene.Group() | ||
script["Group"]["in"][0].setInput( script["SceneReader"]["out"] ) | ||
script["Group"]["in"][1].setInput( script["Camera"]["out"] ) | ||
groupNode = script["Group"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same comment regarding node lifetime
@@ -123,12 +123,7 @@ __children["hosek_environment_edf"]["parameters"].addChild( Gaffer.FloatPlug( "h | |||
__children["hosek_environment_edf"]["parameters"].addChild( Gaffer.FloatPlug( "ground_albedo", defaultValue = 0.30000001192092896, minValue = 0.0, maxValue = 1.0, flags = Gaffer.Plug.Flags.Default | Gaffer.Plug.Flags.Dynamic, ) ) | |||
__children["hosek_environment_edf"]["__model"].setValue( 'hosek_environment_edf' ) | |||
__children["hosek_environment_edf"].addChild( Gaffer.V2fPlug( "__uiPosition", defaultValue = IECore.V2f( 0, 0 ), flags = Gaffer.Plug.Flags.Default | Gaffer.Plug.Flags.Dynamic, ) ) | |||
__children["hosek_environment_edf"]["__uiPosition"].setValue( IECore.V2f( 24.1878395, -11.1626062 ) ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Out of curiosity, did you use gaffer to manually open, edit, and resave each of these gfr files? Or did you edit the text directly?
Would you have found making those edits easier if we didn't store the gfr at all and instead built the graph from scratch in the screengrab.py file? I didn't do that during my revamp, mainly because it seemed tedious to script the graph layout.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For these I just copy-pasted between Gaffer and my IDE. Building everything line-by-line in a .py is useful when you have a handful of nodes and you don't want to source another script, but yeah, beyond a handful of nodes, it's far easier to work it out in Gaffer proper.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
k, good to know
Thanks for the advice on updating the PR. Wasn't sure about that part of the process. |
Implemented suggested changes, and found a bunch of other little bits to fix here and there. The local IE build folder has also been updated for preview. |
|
||
|
||
## Nodes, Scenes, and Plugs ## | ||
## Node Data Flow ## |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I expanded this section based on an updated explanation of how the nodes are processed.
I couldn't find a reasonable way of signposting this section, but this is probably because this section should really be moved to its own article, and only briefly touched on here. It's certainly valuable info, but I don't think it belongs in this tutorial with this level of depth.
@@ -283,7 +316,7 @@ Congratulations! You have successfully rendered your first image. Gaffy is curre | |||
|
|||
## Pinning an Editor to a Node ## |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Continuing my earlier comment about too much depth, this section could also use shortening at a later point. Again, it's useful, but it stops the tutorial flow in its tracks. A simple prescriptive instruction telling the user to do this would suffice.
cameraNode["transform"]["translate"].setValue( imath.V3f( 19, 13, 31 ) ) | ||
viewer.view().viewportGadget().frame( groupNode["out"].bound( "/group" ) ) | ||
GafferSceneUI.ContextAlgo.setSelectedPaths( script.context(), IECore.PathMatcher( [ "/group/camera" ] ) ) | ||
for i in viewer._Viewer__toolChooser.tools(): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a very hacky method. If it can be improved, please advise.
del nodeEditorWindow | ||
del readerNode |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For preventing node lifetime issues, will this suffice?
Needs a rebase. |
- .gfr scripts : Remove Parent node from screenshots - screengrab.py : - Cleaned up for readability, comments added - Add adjusted environment light screenshots - generate.sh : Copy over more UI icons
- Add waitForCompletion() and delays where needed - Fix mislabelled comment
8947f35
to
97dadf9
Compare
Rebased, updated screengrab.py to use new |
- Update copy to reflect 3D scene model. - Shortened and consolidated some wording. - Improved some of the formatting.
@andrewkaufman, I've given this a quick once over - you happy to merge? |
Isn't the link on the main page of the docs pointing to the wrong tutorial now? |
Good catch. Updated. |
Edit and rename of the Gaffer bot tutorial.
Please review article edits here: 7d56008
Preview:
/home/michaeldu/dev/gafferDocsBuilds/docsBeginnerTutorial
Changes