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

Docs : Add Arnold trace sets example and re-classify blockers #3337

Merged
merged 2 commits into from Oct 22, 2019

Conversation

themissingcow
Copy link

  • Adds an example to cover Arnold trace set setup after user queries
  • Re-homes the blockers example under 'lighting' (@medubelko I couldn't see any other doc refs to it).

@johnhaddon
Copy link
Member

@medubelko, could you give this one a quick review?

@themissingcow
Copy link
Author

themissingcow commented Sep 20, 2019

@medubelko, could you give this one a quick review?

@medubelko Note, these were written pre style-guide...

@johnhaddon
Copy link
Member

@medubelko, are you able to take a look? If not I'll give a quick once over and merge myself...

@themissingcow
Copy link
Author

@medubelko, are you able to take a look? If not I'll give a quick once over and merge myself...

@johnhaddon (and @medubelko) I'll go back to this tomorrow and update to follow the examples style guide now we have one. This didn't exist at the time of writing, so it will be flagrantly out of line I'm sure.

@medubelko
Copy link
Contributor

@themissingcow Thanks. I'll put some time aside tomorrow to review your update.

@themissingcow
Copy link
Author

@medubelko Sorry for the delay here, I've updated the example attempting to follow the examples style guide. The only notable exception is that I couldn't do the 'bigger backdrop behind the nodes' thing, as the new backdrops are always created on top! Do you have a trick for that?

@themissingcow themissingcow removed the request for review from mattigruener October 8, 2019 16:39
Copy link
Contributor

@medubelko medubelko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@themissingcow Please see comments. I've tried a new format, where I put in editing markup directly in GitHub, so you can see the precise changes. Hopefully this is readable and helpful.

The only notable exception is that I couldn't do the 'bigger backdrop behind the nodes' thing, as the new backdrops are always created on top! Do you have a trick for that?

Create the background Backdrop, copy the serialized graph to a text editor, rename all of the references to the foreground Backdrop to a higher index, copy-paste back into Gaffer. Tedious, I know. Maybe I can ask you to make a Backdrop layer ordering feature? :)

doc/examples/rendering/traceSetsArnold.gfr Outdated Show resolved Hide resolved
doc/examples/rendering/traceSetsArnold.gfr Outdated Show resolved Hide resolved
doc/examples/rendering/traceSetsArnold.gfr Outdated Show resolved Hide resolved
doc/examples/rendering/traceSetsArnold.gfr Outdated Show resolved Hide resolved
doc/examples/rendering/traceSetsArnold.gfr Outdated Show resolved Hide resolved
doc/examples/rendering/traceSetsArnold.gfr Outdated Show resolved Hide resolved
doc/examples/rendering/traceSetsArnold.gfr Outdated Show resolved Hide resolved
doc/examples/rendering/traceSetsArnold.gfr Outdated Show resolved Hide resolved
doc/examples/rendering/traceSetsArnold.gfr Outdated Show resolved Hide resolved
doc/examples/rendering/traceSetsArnold.gfr Outdated Show resolved Hide resolved
@themissingcow
Copy link
Author

@medubelko Thanks for all the tweaks - all should be updated in the latest push, aside from a few things (I've replied to in-line) where the specific meaning/terminology needed to be preserved.

Copy link
Contributor

@medubelko medubelko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good stuff. There are a few more things I'd like to tweak.

doc/examples/rendering/traceSetsArnold.gfr Outdated Show resolved Hide resolved
doc/examples/rendering/traceSetsArnold.gfr Outdated Show resolved Hide resolved
doc/examples/rendering/traceSetsArnold.gfr Outdated Show resolved Hide resolved
doc/examples/rendering/traceSetsArnold.gfr Outdated Show resolved Hide resolved
doc/examples/rendering/traceSetsArnold.gfr Outdated Show resolved Hide resolved
doc/examples/rendering/traceSetsArnold.gfr Outdated Show resolved Hide resolved
doc/examples/rendering/traceSetsArnold.gfr Outdated Show resolved Hide resolved
doc/examples/rendering/traceSetsArnold.gfr Outdated Show resolved Hide resolved
Copy link
Contributor

@medubelko medubelko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi Tom, thanks for going over my nitpicks. Just one more, in addition to the missing "of" in the above comment: double-quotation marks for set names. I should have picked up on them last week, my mistake.

doc/examples/rendering/traceSetsArnold.gfr Outdated Show resolved Hide resolved
doc/examples/rendering/traceSetsArnold.gfr Outdated Show resolved Hide resolved
@themissingcow
Copy link
Author

Hi Tom, thanks for going over my nitpicks. Just one more, in addition to the missing "of" in the above comment: double-quotation marks for set names. I should have picked up on them last week, my mistake.

No probs at all, hopefully all are updated in 9a114cf.

Thanks for the eagle eye - sorry for the missing words!

Copy link
Contributor

@medubelko medubelko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gah! Just one more.

(I think I should come up with a better review markup, so it's harder to miss all the typographic suggestions...)

doc/examples/rendering/traceSetsArnold.gfr Outdated Show resolved Hide resolved
@themissingcow
Copy link
Author

themissingcow commented Oct 21, 2019

k I should come up with a better review markup, so it's harder to miss all the typographic suggestions...)

If it helps, I can make revisions in separate commits then we can squish them before merging, then you can just see the deltas between each revision?

I'd been force-pushing so it was merge-able, but it's no problem to sign off then squish if it makes it any easier.

I think the markup itself is great though and works well this end, at least. Was just an oversight, apologies.

@medubelko
Copy link
Contributor

Thanks. Good to go.

If it helps, I can make revisions in separate commits then we can squish them before merging, then you can just see the deltas between each revision?

Oh, no, everything was fine on my end, I just want to make it easier for you to keep track of requested edits (à la GDocs).

I'd been force-pushing so it was merge-able, but it's no problem to sign off then squish if it makes it any easier.

I'm for this, since in my experience it makes it easier from the committer's side. But for this particular PR, it wasn't a challenge to keep track of your changes. :)

@johnhaddon johnhaddon merged commit 761932f into GafferHQ:master Oct 22, 2019
@themissingcow themissingcow deleted the traceSetsExample branch October 24, 2019 09:42
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.

None yet

4 participants