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

Viewer Snapshot to Catalogue #5533

Merged
merged 2 commits into from
Nov 16, 2023

Conversation

ericmehl
Copy link
Collaborator

@ericmehl ericmehl commented Nov 9, 2023

This does pretty much what the title suggests. In addition to sending the pixel data, a display window indicating the camera's resolution gate will be added if the snapshot is taken when viewing through a camera. I'm currently including the frame and the scene plug from which the snapshot was taken in metadata.

Identifying the catalogues in a script is one area that might deserve some more attention. I'm using GafferImage.Catalogue.RecursiveRange() to find all Catalogue nodes from the script root. Some recent production scripts have around 600k - 1m nodes. Remote machines didn't seem to have any noticeable lag in getting all of the catalogues using list( catalogues ) in the Python terminal.

I do notice a pretty good lag on my workstation with an artificially generated network of a similar number of nodes - 3-4 seconds on a release build.

Not sure if that's a matter of the topology of the node graphs or simply CPU speed / cores (I have 48, not sure what the remote machines have).

I put the Catalogue search into the "Send to Catalogue" submenu so it can be deferred until the user expands that menu, so that provides a bit of a guard against unwanted lags.

Checklist

  • I have read the contribution guidelines.
  • I have updated the documentation, if applicable.
  • I have tested my change(s) in the test suite, and added new test cases where necessary.
  • My code follows the Gaffer project's prevailing coding style and conventions.

Copy link
Member

@johnhaddon johnhaddon left a comment

Choose a reason for hiding this comment

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

Thanks Eric, this is a nice feature! I've requested some changes inline, but hopefully nothing too drastic...
Cheers...
John

include/GafferSceneUI/Private/OutputBuffer.h Outdated Show resolved Hide resolved
Changes.md Outdated Show resolved Hide resolved
include/GafferSceneUI/SceneGadget.h Outdated Show resolved Hide resolved
src/GafferSceneUI/SceneGadget.cpp Outdated Show resolved Hide resolved
}
}

std::unique_ptr<OIIO::ImageOutput> output = OIIO::ImageOutput::create( fileName.c_str() );
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if we should be writing half-float images when the format is EXR? That's what we write when an ImageNode is dragged to the Catalogue. Any opinion @murraystevenson?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That sounds good to me, I suppose full float data for the viewport is a little excessive. I'll defer to @murraystevenson for a final go or no go.

python/GafferSceneUI/SceneViewUI.py Outdated Show resolved Hide resolved
python/GafferSceneUI/SceneViewUI.py Outdated Show resolved Hide resolved
python/GafferSceneUI/SceneViewUI.py Show resolved Hide resolved
python/GafferSceneUI/SceneViewUI.py Outdated Show resolved Hide resolved
python/GafferSceneUI/SceneViewUI.py Show resolved Hide resolved
@ericmehl
Copy link
Collaborator Author

I've addressed all the comments save the float-to-half conversion. I also added a bit more information to the tooltip in the menu for catalogs that are disabled because their plug is not editable in ae00e62. Hopefully that can save users a little bit of hunting for the source of the menu item being inactive.

@johnhaddon
Copy link
Member

Thanks Eric, LGTM. Could you get it all squashed ready to merge and tack a final half-floatifying commit on the end in anticipation please? Bear in mind that we've made a release (or two?) since the PR was opened, to the Changes.md bits all need shuffling up into the latest section.

@ericmehl
Copy link
Collaborator Author

Changed the output type to half-float ✔️ , squashed ✔️ and moved the changes.md entries up to the top ✔️ . Should be ready to go.

@johnhaddon
Copy link
Member

Thanks Eric - I've rebased just one more time to fix the inevitable Changes.md conflict. Merging...

@johnhaddon johnhaddon merged commit 585941c into GafferHQ:1.3_maintenance Nov 16, 2023
4 checks passed
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.

2 participants