Combined setValue() serialisation for CompoundNumericPlugs #765
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
I like that it's more human readable (and results in smaller gfr files). I didn't realize we lose precision in the V3f repr... that sounds like something we ought to fix anyways. Shall I make a Cortex issue for that? Do you think we should hold off merging this until that is fixed? Even if the gains are minimal, they didn't hurt anything, and the files are nicer. |
|
I'd like to approach this next release cautiously, given the possible implications of breaking something in the file format, so let's make a Cortex issue and get it merged before merging this, then get a few folks testing. |
|
ImageEngine/cortex#262 has been merged now, but this needs a rebase first. |
This will allow more flexible serialisations to be made.
This can be reimplemented by derived classes to provide more control over the serialisation of values.
If none of the child plugs have a connection, then we can simply serialise the value with a single setValue() call on the parent. Fixes GafferHQ#761.
|
Rebased - should merge fine now. |
andrewkaufman
added a commit
that referenced
this pull request
Apr 30, 2014
Combined setValue() serialisation for CompoundNumericPlugs
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This implements #761. It hasn't had a huge impact on file size or load times though - approaching 4% and 2% savings respectively for a certain monster production file. It also specifies values in less precision than before as a result of using the repr for V3f etc rather than the repr for the python built in double type. If that's something we're worried about (I think it should be) then I think we should probably reimplement repr for Vec and Color types in the IECore bindings to have improved precision - a bit or reading around seems to imply that boost::lexical_cast does a bunch of shenanigans to ensure reasonable round tripping between float and decimal representation, and might not be as horrifically slow as it has been in the past…
It's possible that we might see bigger improvements in other sorts of file - the file content in my test case is dominated by addChild() calls when adding dynamic plugs for shaders, so files with fewer dynamic plugs and more non-default values might see a bigger improvement.
It's also arguable that the new format is more human readable than the previous, by putting colour values in one place rather than split over three lines.
What do you reckon? It's a bit underwhelming...