-
Notifications
You must be signed in to change notification settings - Fork 205
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
Documentation : Add Example files to Gaffer distribution and expose in the UI #3108
Conversation
I have some plans to improve the documentation within the Backdrops in the examples, but in the mean time, please C+C on wording/etc.. |
4844427
to
b1c5f50
Compare
After discussion today, will move the Gaffer files from |
One nitpick: let's name the directory |
We've called the API |
I think "Demo" makes more sense in the context of the articles, so maybe they'll just have to be inconsistent? |
The articles will refer to the menu item though won't they? Isn't a demonstration something performed by somebody, while an example is something sitting there to be looked at? |
b1c5f50
to
e1a16b2
Compare
I've added in the files for the Spherical/Anamorphic Camera setups from the Documentation. For now, they're in with the rest of them. Be good to work out if this is the right place, and how to then best link them from the docs, etc... I had to add them in somewhere so they'd be available in the distribution so they could be immediately opened in Gaffer from the menu. I couldn't find them anywhere in the current release. Feel free to suggest any renaming of menu items/etc... or copy changes in the Backdrop nodes. Would appreciate a proof-read if anyone has any time :) |
Just FYI, I merged #2943 which adds some more demo/example files. I presume you'll want an update here to account for those as well. |
Perhaps it's video game argot peeking through, but I associate a "demo" that lacks an active teacher with something like "a contrived derivative of a product, output, process, or idea, meant for illustration or practical explanation." From what we have already, an "example" can range between a nugget of an idea and a full implementation, and the word comes out too readily in sentences ("for example, let's look at", "take, for example", "keeping in mind the above examples"), so I don't think its meaning will be precise enough in the docs themselves. Anyway, I don't want to stall this. Like I said, feel free to use "example" in the API. We don't have to match perfectly: the docs lack the extra context that the application provides, and I already use over-specific or redundant phrases ("the StandardAttributes node", "the File Name plug", "the |
That's my fault, due to them being omitted from the most recent release. There's no issue in duplicating them, since when we move them to the Resources project they'll need to be duplicated anyway. |
For visibility: I've migrated the backdrop text to Google Drive, where it'll be easier for Tom and I to collaborate on changes. |
We don't need to move them to the resources project - that's for big binary blobs that we don't want in the main repo, and don't really need to be versioned in the same way. These are nice little lumps of text that we might want to update/version alongside the docs, so they're best kept in the main repo. The issue in duplicating them is that we won't know which one to edit and they'll get out of sync, so we should just have the one version. It doesn't matter that they weren't released in 0.53 - they're in master where this PR branches from, and Tom has found them now.
I think the association with video games is one of the things that makes it sound a bit odd to me.
I don't really buy it. To be clear, all I'm suggesting is that we rename the "Demos" heading to "Examples", and then replace the download links that are a pain to use with pointers to the "/Help/Examples/TheExample" menu items. We are rather quibbling over esoterica here, but I want to repeat a point I've made before. Our users cover a broad spectrum of technical ability/interest, but they're not static on that spectrum. Artists pick up a bit of scripting here and there and one day end up writing C++, and coders have even been known to make the odd picture here and there. Keeping consistent terminology through the API and documentation facilitates those transitions... |
Ah, that's good. I wasn't sure of the criterion. This frees me up to put sample reference files in the main repo.
I concede, but can the heading be "Example Graphs"? I admit that such specificity is just as applicable to the existing "Demos" headings.
Agreed. |
It's a deal! |
Tom, I've provided a more complete edit of the remaining examples on Drive. I think it's best if I let you review the suggestions I've made, and then we can reconvene for a second round of edits and cleanup. |
b35aa67
to
db80e52
Compare
@medubelko Thanks for the most excellent feedback on the copy in the examples. I've updated all the examples to reflect the comments in the Drive doc. I've also remove the Light Linking example as yours is much better! |
db80e52
to
2662e72
Compare
2662e72
to
f7c1a8f
Compare
I haven't built this branch yet to see it in full context, but it should be similar to how I've pointed to menu entries before:
You won't need to include "on the menu bar" each time, but I wanted to provide it in use in a sentence. |
f7c1a8f
to
a9c8567
Compare
@medubelko Thanks for the out-of-band feedback, I've integrated all those changes, and menu items are referenced as per your prior art. I tried to build the Docs, but on OSX it seemed to look a little off (a lot of the screenshots aren't marked up in the right places, and the page formatting is a little off). I wondered if it was a Sphinx version issue or similar. Are you able to build this PR to checks working as expected still? I should have access to a Linux machine in the coming weeks so can hopefully resolve this then. |
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.
Hey Tom,
Just made a couple of last minute comments. Could you take care of those at the same time as rebasing on the latest master (the rebase should fix the build errors on Travis).
Cheers...
John
import Gaffer | ||
import GafferScene | ||
import GafferUI | ||
import GafferArnold |
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 will break for any non-arnold-supporting builds. See other startup files for the except ImportError
approach we take to that.
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.
So does GafferArnoldUI always load even if Arnold isn't supported in the build? Wondered if
Line 101 in 80d6eea
if moduleSearchPath.find( "arnold" ) : |
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 Arnold isn't supported in the build, then GafferArnoldUI won't exist. The approach we take is this :
- Everywhere except
startup/gui/menus.py
, we catchImportError
and silently fail. - In
startup/gui/menus.py
we report errors rather than suppress them, so we only report the error in one place. We assume that if the user has gone to the trouble of puttingarnold
on PYTHONPATH, then they are expected to be able to use it, and any problem in loading GafferArnold should be reported.
This is slightly academic perhaps, because all public builds ship with Arnold support. But I want to maintain the possibility for a studio to maintain an internal build without Arnold support, instead using some other renderer.
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.
Sure thing, will add in, thanks - just thought that the GafferArnoldUI
startup directory wouldn't be loaded unless GafferArnoldUI
was initialising...
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.
Updated and pushed
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.
Sorry, wait, I'm talking nonsense again. You are right, GafferArnoldUI won't even exist in this case. I only recently fixed that, which I think is why I was getting confused.
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.
Un-updated and pushed :)
a9c8567
to
a9709ff
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.
@themissingcow I did a pseudo-code review.
Big picture thing to think about: I built and tested the branch, and one thing jumped out at me – the examples are not read-only by default. In local Gaffer installs, where a user has not necessarily made the files read-only, they might play around with the example and accidentally save. Further, just clicking around the graph can cause backups to generate, leading to the "Load backup?" dialog on subsequent load, which seems inappropriate. In retrospect, this is an issue with all of the files in /resources
and /doc
, but we don't (to my knowledge) encourage users to put themselves in circumstances where they could write to them. It seems quite likely to me that they might accidentally overwrite an example.
doc/examples/boxBasics.gfr
Outdated
Gaffer.Metadata.registerValue( __children["ScaleScene"]["custom_scale"], 'layout:index', 4 ) | ||
__children["PathFilter"]["paths"].setValue( IECore.StringVectorData( [ '/cube' ] ) ) | ||
__children["PathFilter"]["__uiPosition"].setValue( imath.V2f( 16.6021118, -0.451083541 ) ) | ||
__children["Backdrop"]["title"].setValue( 'Demo: Box Node Basics' ) |
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.
'Example: Box node basics'
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.
Actually, it looks like this one snuck back in.
I suspect this has to do with font sizing. Can you create an issue and upload sample images of where it looks wrong?
Everything seemed to work fine on my end. |
Yeah, its a really good point - John and I were discussing this. I think we meant to work out a good place to do the 'read-only' thing and then, erm, didn't :). Let us have a think and try and work out something that makes sense. Good point re: the backups too. |
Sure thing, I'll take this out-of-band and we can always make a new issue if its a general OSX thing. It might be as I have 1 retina screen and one not. I think it might affect the resolution of screengrabs even on the non-retina screen.
👍 great, appreciate you checking. |
Adds the Examples API to the GafferUI module to allow Example Gaffer files to be registered at runtime via Python. Examples are registered with a unique key. The key is short textual title. If it contains ‘/‘ characters, they will be interpreted as defining parent groups of the example. Gaffer remembers the order Examples are registered. Eg: `Rendering/Cameras/Setting up Anamorphic rendering` would define an example titled `Setting up …` within a `Cameras` group under `Rendering`. Examples can also be registered with an optional Description and/or a list of Gaffer Node classes that are considered relevant to what is covered by that particular file. This is to allow Users to more easily find examples for specific Nodes. As many Modules may need to test for the existence of their example files, we add a convenience method to the GafferUITest.TestCase class to verify that all registered examples exist. This will result in duplicate checks in many common cases, but seems the cleanest compromoise.
Any registered Example Gaffer Files are now listed in the Help menu in the main menu bar. Any examples registered with a list of Notable Nodes will also be listed directly in the Tool menu within the Node Editor when a node of that type is being edited.
1ad2e02
to
4a24255
Compare
@medubelko We believe we have addressed this in 9618c38, we now open any examples from the Help menu as an 'untitled copy'. As such, there won't be any @johnhaddon Please may you sanity-check the implementation? |
LGTM. Looks like we have a build error on Travis though :
I think this might be because we don't (yet) build the docs on Travis, so Sphinx doesn't exist. I don't think we need to use |
Adds basic examples covering: - Per-location Shader Variation (Arnold) - Shader Wedge Tests - Contact Sheet Generation - Anamorphic/Spherical Cameras (Arnold)
4a24255
to
d03f8e4
Compare
Oh rubbish, sorry. Sorted. |
d03f8e4
to
9618c38
Compare
9618c38
to
8a14b12
Compare
LGTM. Thanks Tom. |
doc/examples/boxBasics.gfr
Outdated
Gaffer.Metadata.registerValue( __children["ScaleScene"]["custom_scale"], 'layout:index', 4 ) | ||
__children["PathFilter"]["paths"].setValue( IECore.StringVectorData( [ '/cube' ] ) ) | ||
__children["PathFilter"]["__uiPosition"].setValue( imath.V2f( 16.6021118, -0.451083541 ) ) | ||
__children["Backdrop"]["title"].setValue( 'Demo: Box Node Basics' ) |
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.
Actually, it looks like this one snuck back in.
This required the removal of the Gaffer file (and corresponding image generation) for the CreatingConfigurationFiles3 tutorial as it relied on loading a Reference node. We currrently have no way to make the path to the file portable. See issue GafferHQ#3124 and GafferHQ#3125.
In order to prevent people accidentally editing Example files, they will be opened as an 'untitled copy'. Thus requring the user to save to a new path. This was chosen instead of simply locking the example files, as Gaffer's UI prevents Nodes in locked files from being edited - which would make for a somewhat restrictive playground. API Changes : - Adds the `asNew` kwarg to GafferUI.FileMenu.addScript method. When true the script is opened as an untitled copy.
8a14b12
to
0134b91
Compare
@medubelko Sorry - thought I had them all. Should be good now! Thanks. |
Gaffer examples can now be registered with Gaffer at startup using the
GafferUI.Examples.registerExample
function. Registered Examplesare available in the Help menu.
Examples can optionally be associated with certain node types. The Cog
menu in the Node Editor will then list any applicable Examples for the Node
type being edited.
The examples themselves are work-in-progress. This initial PR contains two
Arnold examples for review.
User Changes:
Help > Examples
will now list any registered ExamplesNode Editor -> <cog> -> Examples
API Additions:
GafferUI.Examples
GafferUITest.TestCase.assertExampleFilesExist