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

ImageTransform : Implement concatenation #2842

Merged
merged 6 commits into from Feb 4, 2020

Conversation

johnhaddon
Copy link
Member

This is a slight rejig of @lucienfostier's #2751.

  • Removed the scoped_connection member, because it's not necessary.
  • Used ContextMonitor to simplify the context leakage test. This revealed some additional bugs.
  • Refactored the context logic into the ChainingScope utility class. This makes the changes to the various hash/compute quite a bit simpler, and also fixes the context leakage bug. Another reason I did this is because I'm interested in using the concatenation pattern for other nodes, and this seemed like a standardisable approach.
  • Fixed a bug whereby the private __inTransform plug had its connection serialised into the script.
  • Fixed a nasty hashing bug that took me ages to track down (rotate must now be taken into account for the resampleMatrixPlug).
  • Fixed another hashing bug (must call base class first, not last).
  • Made the no-op chaining operations perfect pass-throughs (identical hashes) so that they share the same cache entries, rather than populate the cache with duplicate tiles.
  • Added some additional tests.

Lucien, despite these changes, your original concatenation scheme is still very much intact; it's just the details of the implementation that have changed. I'm pleased to say that it is an impressive improvement both in performance and filtering, so thanks very much for taking the initiative with it!

Question for the gallery : what do you think to the representation of the concatenation in the GraphEditor?

concat

I've found it really handy to have a visualisation for it, but it's a little odd that we're showing auxiliary connections for private plugs. I believe we're doing that because our primary usage of auxiliary connections is the Expression node, and the connections we want to see there are to private plugs (for now, they'll become public in the planned refactor). So, should we hide the concatentation connections, keep 'em as is, or add a different visualisation? Bear in mind I don't really have any time available for spending on the latter...

@johnhaddon
Copy link
Member Author

johnhaddon commented Oct 12, 2018

One other thing I forgot to mention. Because we now rely on extractSHRT to decompose the concatenated transform, it's possible for tiny numeric errors to cause us to do more work than necessary. For instance, with inputs consisting of pure rotation, we can end up with tiny scale values creeping in, causing us to use the internal Resample when it's not actually necessary. I'm inclined to use some thresholding to get us back to the same performance as before for these simple cases - any objection? If not I'll push another commit to take care of that...

@andrewkaufman
Copy link
Contributor

I like the extra connections, otherwise you don't know its concatenated.

Copy link
Collaborator

@lucienfostier lucienfostier left a comment

Choose a reason for hiding this comment

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

Hey John, thanks for taking the time to improve this.
This implementation is quite a bit cleaner.

I’ve left a few inline comment mostly for my own curiosity.

Cheers

{
// We're at the top of a chain. Remove the context
// variable so it doesn't leak out to unrelated nodes.
m_scope.emplace( context );
Copy link
Collaborator

Choose a reason for hiding this comment

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

why not place the line above before if/else? seems like it would have the same end result but the code would be cleaner.

}
else if( output == outTransformPlug() )
{
inTransformPlug()->hash( h );
Copy link
Collaborator

Choose a reason for hiding this comment

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

why don't you test if the inTransformPlug is connected to anything before adding its hash?

Copy link
Member Author

Choose a reason for hiding this comment

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

Because it's not necessary, and because the general rule in Gaffer is that nodes should be blissfully ignorant about their connection to the outside world. We have to bend the rules to manage the inTransform connection, but there's no need to do so here.

M33f result = inTransformPlug()->getValue();
if( enabledPlug()->getValue() )
{
result *= transformPlug()->matrix();
Copy link
Collaborator

Choose a reason for hiding this comment

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

same here, do we just don't care if it's connected or not?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, we don't care - if it's not connected, the matrix will be identity anyway.


ImageProcessor::compute( output, context );
}

void ImageTransform::hashDataWindow( const GafferImage::ImagePlug *parent, const Gaffer::Context *context, IECore::MurmurHash &h ) const
{
ChainingScope chainingScope( context, this );
if( chainingScope.chained() )
Copy link
Collaborator

Choose a reason for hiding this comment

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

I am confused why the dataWindow hash is different than the outTransformPlug hash? ( in relation to using the ChainingScope here or not above )

Copy link
Member Author

Choose a reason for hiding this comment

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

We don't need to use the ChainingScope for the outTransform because if the outTransform is used at all, we know it's because it has been chained. We never need a pass-through for the outTransform.

@danieldresser
Copy link
Collaborator

Code looks reasonable to me.

Current visualization is better than nothing - it would be nice if the connection was special somehow ( at minimum a different color ) instead of looking a normal expression, but that could be added later.

What happens if you chain a transform inside a box with one outside? Do you see a connection from the box?

The main thing that I think maybe should still be addressed before rolling this out is that it feels like there ought to be a quick way to turn this off? Currently, it appears that you're not checking the filter type of the different transforms in the chain, so if you have two transforms, one using a box, and one using gaussian, whichever one comes last is the only one that actually affects anything.

This is probably fine by default ( It certainly can be confusing, but Nuke has the same approach of assuming it should automagically do confusing concatenations to speed things up for you if don't tell it not to ), but it seems like it would be pretty nice if there was a toggle on the node to turn it off?

By the same token, have you thought about allowing more transform concatenation? For example, in the "transform -> grade -> transform" case, concatenating the transforms would give different results, but it's not really any different than ignoring the filtering settings the way you currently are.

@lucienfostier
Copy link
Collaborator

@danieldresser I tested the scenario with the transform inside a box and the connection simply is not shown at all. I discussed that with @mattigruener and it's a current limitation of the auxiliary connection as far as I remember.

@johnhaddon
Copy link
Member Author

@danieldresser I tested the scenario with the transform inside a box and the connection simply is not shown at all. I discussed that with @mattigruener and it's a current limitation of the auxiliary connection as far as I remember.

Neither do I think the auxiliary connection should ever show that connection. First, it's a private plug, and second, it's not even from the box (it bypassing any idea of promotion at all). It seems we should instead introduce a new means of displaying concatenation status - perhaps a little symbol on the edge of the node?

The main thing that I think maybe should still be addressed before rolling this out is that it feels like there ought to be a quick way to turn this off? Currently, it appears that you're not checking the filter type of the different transforms in the chain, so if you have two transforms, one using a box, and one using gaussian, whichever one comes last is the only one that actually affects anything.

I'll defer to @lucienfostier on this, but in the other packages I'm aware of having this feature, it's always been expected to work this way, and the trick for breaking concatenation has been to insert a non-concatenating node.

By the same token, have you thought about allowing more transform concatenation? For example, in the "transform -> grade -> transform" case, concatenating the transforms would give different results, but it's not really any different than ignoring the filtering settings the way you currently are.

Lucien and I talked about this already, and the current design was made with this in mind. It's pretty trivial to implement, but the plan was to get a first version out without it. The main downside is that I can't see any way of doing it without "leaking" the context variable to the Grade node.

And since nobody seems to have picked up on it, here's one of my original comments that I think needs consideration before any merge...

One other thing I forgot to mention. Because we now rely on extractSHRT to decompose the concatenated transform, it's possible for tiny numeric errors to cause us to do more work than necessary. For instance, with inputs consisting of pure rotation, we can end up with tiny scale values creeping in, causing us to use the internal Resample when it's not actually necessary. I'm inclined to use some thresholding to get us back to the same performance as before for these simple cases - any objection? If not I'll push another commit to take care of that...

@danieldresser-ie
Copy link
Contributor

it's always been expected to work this way, and the trick for breaking concatenation has been to insert a non-concatenating node.

This feels like too much of a trick to me - and it's non-obvious what a non-concatenating node is, especially when we might make more nodes concatenating ( e.g. in the future we might make Grade nodes concatenating, breaking graphs that use a Grade to force non-concatenation ). I guess my uneasiness is mostly philosophical though - I would say any time an optimization changes the results you are getting, there should be an obvious way to turn it off and compare to the non-optimized result. But after talking to a few other people, no one else seems particularly bothered by this.

the main downside is that I can't see any way of doing it without "leaking" the context variable to the Grade node.

In the simple cases, the Grade node's output will just be connected to one place, so it would still just be evaluated in one context, right? Having something extra in the context isn't much cost, as long as it's just one context. And of course, it's not really "extra" - if the Grade is used in both a concatenated and non-concatenated context, we need to compute different results for those two cases. I guess the only wasteful case is if the Grade is used in two different concatenated contexts, so it performs the Grade at the original resolution twice. I guess that is pretty wasteful - maybe not that common, but fair enough that it's worth thinking about before supporting concatenation of nodes that actually do some work other than changing the transform.

I suppose the other confusing thing is that if we did support concatenating with Grades, this "optimization" could actually make things perform much worse in some cases.

For example:
Read 4K -> Downsample to 16x16 -> Grade -> Downsample to 8x8

If we concatenated this, we would perform one downsample from 4K to 8x8 - which is fairly efficient, and good for sampling quality. But we would perform the Grade on the original 4K, which could be much slower.

Which brings me back to thinking that there should be some explicit option for turning this off when you don't want it.

I'm inclined to use some thresholding ...

Oh, sorry, forgot to reply to this. Yeah, sounds very reasonable. I'm a bit curious about just how high the threshold will need to be in order to reliably detect zeros ... in theory, as the chain of input transforms gets longer and longer the possible error could grow arbitrarily large? In practice, you will probably be able to find a compromise value that handles a reasonably large input chain without being noticeable even when rotating a very large image and checking pixel values far from origin.

@johnhaddon
Copy link
Member Author

In the simple cases, the Grade node's output will just be connected to one place, so it would still just be evaluated in one context, right? Having something extra in the context isn't much cost, as long as it's just one context.

Yeah, I think it's unlikely to be a practical concern - for me it was more a philosophical one about allowing the private variables to become visible someplace. It would be fairly trivial to prevent expressions and substitutions from using them though...

I guess the only wasteful case is if the Grade is used in two different concatenated contexts, so it performs the Grade at the original resolution twice.

There is only one concatenation context, in that the only new state introduced by concatenation is the "__imageTransform:chained" variable, which only ever appears with a value of true. So if two concatenated contexts are different, it's because the original contexts would have been different anyway...

If we concatenated this, we would perform one downsample from 4K to 8x8 - which is fairly efficient, and good for sampling quality. But we would perform the Grade on the original 4K, which could be much slower. Which brings me back to thinking that there should be some explicit option for turning this off when you don't want it.

OK, I'm sold. I'll add a concatenatePlug(), defaulted true.

I'm inclined to use some thresholding ...

Yeah, sounds very reasonable. I'm a bit curious about just how high the threshold will need to be in order to reliably detect zeros ... in theory, as the chain of input transforms gets longer and longer the possible error could grow arbitrarily large?

Yeah, I'll need to do some experimenting. My main concern was just that the single-node case didn't get any slower - in the multiple node case I suspect the concatenation more than compnsates. So I don't think we'll need too big a threshold to still have a win all around...

@andrewkaufman andrewkaufman added the pr-revision PRs that should not be merged because a Reviewer has requested changes. label Oct 17, 2018
@johnhaddon
Copy link
Member Author

I've pushed 05f2c19 to hide the concatenation connections, and 4501720 to add a plug to control concatenation. I'm not too happy about the mix of terminology : concatenation externally and chaining internally. I'd used "chaining" originally to avoid any potential for thinking that turning it off meant that upstream transforms weren't inherited, but maybe that's an unfounded worry. I'd like to use one term consistently, so let me know if you have a preference...

Looking into the precision issues a little more, there's another issue I think. Many pivot/rotate interactions decompose into a non-zero translation, so now we go through the resampled input (which applies translation) where we wouldn't have previously. I don't really know why we're going through that to apply translation though - we could use the rotatation sampling path for that anyway. Tests on master suggest that pure rotation (direct sampling of input) is as quick or faster than pure translation (internal resample), so I'm wondering if I should switch things up so we only use the internal resampler when we have a scaling component. Do you see any reason not to?

This was meant to be a quick push to get Lucien's PR over the line, but since it's becoming more involved, I'm going to put it aside for a bit in an attempt to make some progress on my actual assigned tasks...

@lucienfostier
Copy link
Collaborator

Would switching to a 2dTransformPlug makes things easier than using MatrixPlug in regards to the precision issue with matrix decomposition?

@johnhaddon
Copy link
Member Author

Would switching to a 2dTransformPlug makes things easier than using MatrixPlug in regards to the precision issue with matrix decomposition?

I'm not sure, but it has the big downside of needing multiple computes (one for every x,y,z of every translate,rotate,scale and pivot), with an explosion of cross-dependencies when they're chained. I don't think it's the way to go. The other thing about a 4x4 matrix is I think it can be repurposed for corner pins in the future, and still be concatenated with ImageTransforms...

@lucienfostier
Copy link
Collaborator

Would switching to a 2dTransformPlug makes things easier than using MatrixPlug in regards to the precision issue with matrix decomposition?

I'm not sure, but it has the big downside of needing multiple computes (one for every x,y,z of every translate,rotate,scale and pivot), with an explosion of cross-dependencies when they're chained. I don't think it's the way to go. The other thing about a 4x4 matrix is I think it can be repurposed for corner pins in the future, and still be concatenated with ImageTransforms...

ah yes! both very good points...

@andrewkaufman andrewkaufman removed pr-revision PRs that should not be merged because a Reviewer has requested changes. labels Oct 23, 2018
@johnhaddon johnhaddon added the pr-revision PRs that should not be merged because a Reviewer has requested changes. label Oct 24, 2018
@andrewkaufman
Copy link
Contributor

Closing for now due to lack of progress (eg lack of priority).

@johnhaddon
Copy link
Member Author

Reopening - this one is close enough and important enough that I think we should prioritise it.

@andrewkaufman
Copy link
Contributor

6 months later and its still not prioritized... How about we discuss it in the Thursday meeting, and either get it on the WIP board or close it again.

@johnhaddon
Copy link
Member Author

Sounds good. I'm in favour of getting it on the board - 0.54 has a bit of a performance focus and it fits in nicely with that. At least I should spend a day to see if it's more work than that (I suspect it's just a day).

@danieldresser-ie
Copy link
Contributor

I was chatting to Lucien about this, and thinking about even fairly experienced people can get tripped up by Nuke's implementation of this if they aren't already familiar with exactly how it works. I had an idea for what would actually make it feel pretty natural to me, this might be hard to implement right now, but I thought I should mention how I think this would be presented in my ideal world:

There would be a filter mode named "Automatic", which just means, "I don't care what the filter is, Gaffer should do something high quality". We would need to make sure we pick a good value for what this filter actually does ( maybe it would be different for downscaling and upscaling? ). But if we have this Automatic mode, then the rules for concatenation are easy: you can only concatenate when the earlier filters in the chain are set to "Automatic". This seems like a really clean way to make sure that by default, you get the performance benefit of concatenation, but that someone who has specifically picked a filter doesn't get surprised by it being changed underneath them. It feels far less offensive to have the software do weird things with your filtering if the filter mode is set to "Automatic" - it makes sense that if you leave it at automatic, it will be doing some special things in the background to go faster and give higher quality.

I'm not sure if this makes sense right now, but it seems like a nice idea that would make me feel a lot better about this whole thing - the way Nuke does it has always felt confusing to me.

@johnhaddon
Copy link
Member Author

What if I want a specific filter (non-automatic) and I know I do want concatenation?

@danieldresser-ie
Copy link
Contributor

You can specific a specific filter on the last node of the chain, and the earlier nodes set to Automatic will be concatenated with it.

@johnhaddon
Copy link
Member Author

Sounds good.

Next question. We have something similar to the "Automatic" filter option for the Resize node already - the "Default" filter chooses something appropriate depending on whether or not you're upsizing or downsizing. We deliberately omitted this option from the ImageTransform though, because for the animated case, it is jarring to switch filter from one frame to the next. Are we concerned about this if we adopt your suggestion, or would we simply say "if you're animating and it's jarring, pick an explicit filter on the node at the bottom of the chain"? But if we do that, aren't we simply trading one gotcha for another? If so, are we better sticking with the same gotcha people might be familiar with from other packages?

If we went with the new filter semantics, would we simply drop the "concatenate" plug entirely, or would we still need that for more explicit control?

@danieldresser-ie
Copy link
Contributor

We deliberately omitted this option from the ImageTransform though, because for the animated case, it is jarring to switch filter from one frame to the next.

Unfortunately, you make an excellent point. I can't really think of a solution that is really quick, elegant, and good, and maybe that's a fatal flaw in this idea.

However, for a basic user, it's already a real shame that we don't automatically pick good filters. And even on Resize, it is kinda unfortunate that resizing 1 pixel larger produces a result that is visibly much blurrier than resizing 1 pixel smaller. Brainstorming a bit, the natural solution would seem to be a filter that smoothly switches from a good downscaling filter to a good upscaling filter as the scale changes. A simple linear lerp of the kernel weights should work fine - everything is linear, so this is mathematically equivalent to blending the resulting images from the two filters, but it fits within our architecture as a basic filter.

My main hesitation here is that I can find any literature about other people doing this, but I can't think of any reason why it wouldn't perform well with a reasonable transition point chosen. In a quick empirical test with intuitively reasonable values, a blend from lanczos at a scaling factor of 1, to blackman-harris at a scaling factor of 1.5, seems to give quite nice results.

Maybe this conversation is getting a bit out of scope, but this doesn't seem hard to do, and would give us nicer behaviour in Resize as well. The only real cost would be a somewhat more expensive filter kernel while in the transition zone.

If we went with the new filter semantics, would we simply drop the "concatenate" plug entirely, or would we still need that for more explicit control?

Hmm, good question. My first instinct is always for more explicit control, but if we were to do it with automatic mode, I can't really think of a particular use case where we would need it?

@johnhaddon
Copy link
Member Author

Maybe this conversation is getting a bit out of scope

I think so. Your proposals are interesting, but I don't think we should tie them into getting this over the line. I'm going to argue for "better the devil you might be familiar with from a certain other compositing package", and suggest that we keep the explicit "concatenate" plug for now, and do any exploration of fancy automatic filter selection as followup work.

@danieldresser-ie
Copy link
Contributor

Hmmm, that sounds reasonable, but trying to get from a world where we automatically concatenate everything by default, to one where we only concatenate auto filters, sounds like a backwards compatibility nightmare?

Suppose at some point in the future, we do devise a magical "auto" filter ( or do testing that reveals some simple hack that is good enough ), and we want to only concatenate chains that begin with auto filters - what would the backwards compatibility path look like?

Hmm. If there is a chance that you would be OK with the auto filter approach, I'm a bit tempted to hack around a bit at home on the kernel weight lerp thing.

@johnhaddon
Copy link
Member Author

Suppose at some point in the future, we do devise a magical "auto" filter ( or do testing that reveals some simple hack that is good enough ), and we want to only concatenate chains that begin with auto filters - what would the backwards compatibility path look like?

If we wanted to keep old networks rendering exactly as before, I think this would work :

  • Make the "concatenate" plug be a three-state thing : "on", "off" and "auto". The first two states correspond to the bool serialised in the old scripts, and keep them rendering as before. We use a "userDefault" metadata to make all newly created nodes take the "auto" setting. Possibly we hide the "concatenate" plug for newly created nodes if we truly believe auto is foolproof.
  • We use a "userDefault" metadata to set the filter to "auto" for newly created nodes. Old nodes keep their old filter settings.

@danieldresser-ie
Copy link
Contributor

OK, yeah, that works.

@johnhaddon johnhaddon added this to Up Next in Work in Progress old Oct 24, 2019
@johnhaddon
Copy link
Member Author

This has lingered for too long - moving to Up Next on the project board. 0.56 will be a pretty big release as far as GafferImage goes (deep!), so it makes sense to include this with that.

@themissingcow themissingcow removed this from Up Next in Work in Progress old Dec 5, 2019
@johnhaddon johnhaddon added this to the Gaffer 0.56 milestone Jan 28, 2020
johnhaddon and others added 6 commits January 31, 2020 12:21
Unless they're Expressions, in which case show them for now.
…e==0`

The ImageTransform node separates out the translation and scaling components of the transform from the rotation component, and applies them using an internal Resample node. This provides improved performance for changes in scale by taking advantage of separable filtering. The ImageTransform also deliberately performs this resampling _even for identity transforms_, to avoid sudden jumps in situations such as an animated translation passing through the origin on one frame, or scaling passing through a value of 1.

This consistency of filtering was being broken in the case of non-zero rotation without translation. In this case the resampling was bypassed entirely, leading to jumps in filtering when a rotation was initiated. To fix this we now _always_ use the resampled input for all cases. This does lead to slower operation for pure-rotation operations, but that is consistent with the hit we've already accepted for the identity case.

Fixes
-----

- ImageTransform : Fixed inconsistent filtering of transforms containing rotation but no translation.
This improves speed and filtering quality for chains of adjacent ImageTransforms.
@johnhaddon
Copy link
Member Author

I've pushed an update which fixes this PR up for the latest GafferImage changes, and attempts to address the outstanding issues with precision/filtering.

it's possible for tiny numeric errors to cause us to do more work than necessary. For instance, with inputs consisting of pure rotation, we can end up with tiny scale values creeping in, causing us to use the internal Resample when it's not actually necessary.

Many pivot/rotate interactions decompose into a non-zero translation, so now we go through the resampled input (which applies translation) where we wouldn't have previously.

On further examination of the above, I determined that trying to avoid resampling for pure rotation was completely inconsistent with another behaviour of the ImageTransform. It deliberately doesn't avoid resampling for identity transforms, with the intention being that there should be no sudden jumps in filtering when animating a transform through identity. So trying to avoid resampling for pure rotation led to jumps when animating through zero translation. I fixed this in 7236d71, which made the
issue of numeric error in concatenation moot.

I feel like there must be a better overall approach to decomposing the transform/filtering here, but I don't know what it is. One obvious consideration is that we should support motion blur for ImageTransforms, and that probably means switching to a monte-carlo sampling approach. That would be analogous to a 3d render, where we would mip-map our textures first and then sample them at hit points. So maybe it wouldn't be so different, with our internal Resample taking the place of mip-mapping.

In any case, what I've tried to do for now is to keep the filtering approach consistent with what it was before, and deal only with the concatenation.

@danieldresser-ie, it would be great if you could take a look at this today.

@lucienfostier
Copy link
Collaborator

I feel like there must be a better overall approach to decomposing the transform/filtering here, but I don't know what it is. One obvious consideration is that we should support motion blur for ImageTransforms, and that probably means switching to a monte-carlo sampling approach. That would be analogous to a 3d render, where we would mip-map our textures first and then sample them at hit points. So maybe it wouldn't be so different, with our internal Resample taking the place of mip-mapping.

That makes a lot of sense as this is what happens in Nuke.
Only node that supports motion blur via stochastic sampling supports concatenation ( or I guess you can view it the other way around ).
Just out of curiosity, is mip-mapping a requirement for this approach?

@johnhaddon
Copy link
Member Author

Just out of curiosity, is mip-mapping a requirement for this approach?

Not necessarily - I just found it helpful to draw the analogy with 3d rendering. Although I do think it'd be a very interesting project to make GafferImage mip-mapped at the core, so any image can be generated at any mip level (so basically in addition to tileOrigin, the context would have mip level).

@lucienfostier
Copy link
Collaborator

make GafferImage mip-mapped at the core, so any image can be generated at any mip level (so basically in addition to tileOrigin, the context would have mip level).

Would you use that for say a big radius blur, use a small resolution input, blur then upscale?

@johnhaddon
Copy link
Member Author

Would you use that for say a big radius blur, use a small resolution input, blur then upscale?

Yeah, I don't think you'd need to upscale though. You'd directly generate the output image at whatever scale you wanted, but be able to choose the most appropriate mip level to sample from the input (the bigger the blur, the smaller the level).

It would also be useful in the Viewer - if you zoomed out we'd just compute a lower mip-level. Sort of like proxies but on steroids. I should emphasise that this is all just idle speculation :)

@danieldresser-ie
Copy link
Contributor

I guess this is reasonable. The thing that's really bothering me now is seeing that we're doing the extra sampling even when the internal resample can handle everything. Shouldn't be too hard to add the extra softening into the filter on the Resample when the second sampling isn't needed, instead of sampling twice to get the extra softening. Actually, that should be a better solution to the problem you just solved as well - if we just padded the kernel, we could bypass the resample node when possible ... this would be a less obvious win though, since the resample can be performed separably, maybe performing two samplings could sometimes be faster than one sampling with a wider filter ... basically, I don't have any real problem with this approach for rotations, since rotations without scaling is fairly special case anyway. I think we should do better for the common, fast case of translate/scale without rotation though.

@johnhaddon
Copy link
Member Author

The thing that's really bothering me now is seeing that we're doing the extra sampling even when the internal resample can handle everything.

I don't follow. When the internal resample can handle everything, we use the output from the internal resample directly :

if( !(op & Rotate) )

Shouldn't be too hard to add the extra softening into the filter on the Resample when the second sampling isn't needed, instead of sampling twice to get the extra softening.

Again, I don't follow. The rotation sampling is just using bilinear interpolation (via Sampler), and therefore has no softening for an identity rotation. I'm sure there's a case for better sampling of rotations too, but that's not something I'm trying to tackle in this PR.

I think we should do better for the common, fast case of translate/scale without rotation though.

I believe that is what we're already doing.

@danieldresser-ie
Copy link
Contributor

Apologies - I should have been looking at the code closer. I got confused thinking about the comment "It deliberately doesn't avoid resampling for identity transforms", but had forgotten that the sampler just does bilinear and so this isn't really an issue there. Should have stared at the existing code more. Sorry for the confusion, I guess this is fine.

@johnhaddon johnhaddon merged commit d328fd4 into GafferHQ:master Feb 4, 2020
@lucienfostier
Copy link
Collaborator

lucienfostier commented Feb 4, 2020

woot woot

@johnhaddon johnhaddon deleted the concatTransformFixes branch February 5, 2020 10:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr-revision PRs that should not be merged because a Reviewer has requested changes.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants