Skip to content

Commit

Permalink
Refactor RectI & RectD methods to simplify code and improve readability.
Browse files Browse the repository at this point in the history 
- Changed RectI::toCanonical(), RectI::toCanonical_noClip()
  and RectD::toPixelEnclosing() to return their value
  instead if using an out pointer param. This simplifies
  a lot of callers, avoids default construction in a lot of cases,
  and allows many variables to be marked const which makes it easier
  to ensure values don't change unexpectedly in large functions.

- Added RectI::toNewMipMapLevel() helper function to reduce
  duplicate code and make the code a little more readable.

- Added RectI::intersect() and RectD::intersect() methods that
  return the intersection or a null rect. Updated a few places
  where this form simplifies or makes the code more clear.

- Added RectI::clipIfOverlaps() and RectD::clipIfOverlaps() helper
  functions to replace a common usage of the existing intersect()
  method. The intent is to make it a little more clear that the
  intersection only happens if there is an overlap and it avoids
  having to specify a variable name twice.

- Renamed the original intersect() to intersectInternal() and made
  it private. The intersect() and clipIfOverlaps() changes above
  made it so there were no callers outside of RectI and RectD that
  needed this.

- Removed some comment out code and a few variables/computations
  that weren't actually used for anything.

- Added documentation for a few methods to make their behavior
  a little more clear.

- Updated python wrapper type definitions to reflect the changes
  above. The intersect modifications could be removed because the
  new intersect() signature and semantics match the desired python
  signature and semantics.
  • Loading branch information
@acolwell
acolwell committed Sep 5, 2023
1 parent ae9f0ba commit b134344
Show file tree
Hide file tree
Showing 32 changed files with 291 additions and 468 deletions.
58 changes: 18 additions & 40 deletions Engine/EffectInstance.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -655,8 +655,7 @@ EffectInstance::retrieveGetImageDataUponFailure(const double time,
}

assert( !( (supportsRenderScaleMaybe() == eSupportsNo) && !(scale.x == 1. && scale.y == 1.) ) );
RectI pixelRod;
rod.toPixelEnclosing(scale, getAspectRatio(-1), &pixelRod);
const RectI pixelRod = rod.toPixelEnclosing(scale, getAspectRatio(-1));
try {
int identityInputNb;
double identityTime;
Expand Down Expand Up @@ -992,8 +991,7 @@ EffectInstance::getImage(int inputNb,
}


RectI pixelRoI;
roi.toPixelEnclosing(renderScaleOneUpstreamIfRenderScaleSupportDisabled ? 0 : mipMapLevel, par, &pixelRoI);
RectI pixelRoI = roi.toPixelEnclosing(renderScaleOneUpstreamIfRenderScaleSupportDisabled ? 0 : mipMapLevel, par);

ImagePtr inputImg;

Expand Down Expand Up @@ -1115,23 +1113,19 @@ EffectInstance::getImage(int inputNb,
assert(inputImgMipMapLevel != 0);
///Resize the image according to the requested scale
ImageBitDepthEnum bitdepth = inputImg->getBitDepth();
RectI bounds;
inputImg->getRoD().toPixelEnclosing(0, par, &bounds);
const RectI bounds = inputImg->getRoD().toPixelEnclosing(0, par);
ImagePtr rescaledImg = std::make_shared<Image>( inputImg->getComponents(), inputImg->getRoD(),
bounds, 0, par, bitdepth, inputImg->getPremultiplication(), inputImg->getFieldingOrder() );
inputImg->upscaleMipMap( inputImg->getBounds(), inputImgMipMapLevel, 0, rescaledImg.get() );
if (roiPixel) {
RectD canonicalPixelRoI;

if (!inputRoDSet) {
bool isProjectFormat;
StatusEnum st = inputEffect->getRegionOfDefinition_public(inputEffect->getRenderHash(), time, scale, view, &inputRoD, &isProjectFormat);
Q_UNUSED(st);
}

pixelRoI.toCanonical(inputImgMipMapLevel, par, inputRoD, &canonicalPixelRoI);
canonicalPixelRoI.toPixelEnclosing(0, par, roiPixel);
pixelRoI = *roiPixel;
pixelRoI = pixelRoI.toNewMipMapLevel(inputImgMipMapLevel, 0, par, inputRoD);
*roiPixel = pixelRoI;
}

inputImg = rescaledImg;
Expand Down Expand Up @@ -1185,7 +1179,7 @@ EffectInstance::calcDefaultRegionOfDefinition(U64 /*hash*/,
unsigned int mipMapLevel = Image::getLevelFromScale(scale.x);
RectI format = getOutputFormat();
double par = getAspectRatio(-1);
format.toCanonical_noClipping(mipMapLevel, par, rod);
*rod = format.toCanonical_noClipping(mipMapLevel, par);
}

StatusEnum
Expand Down Expand Up @@ -1288,7 +1282,7 @@ EffectInstance::ifInfiniteApplyHeuristic(U64 hash,
assert(!format.isNull());
double par = getAspectRatio(-1);
unsigned int mipMapLevel = Image::getLevelFromScale(scale.x);
format.toCanonical_noClipping(mipMapLevel, par, &canonicalFormat);
canonicalFormat = format.toCanonical_noClipping(mipMapLevel, par);
}

// BE CAREFUL:
Expand Down Expand Up @@ -1680,7 +1674,7 @@ EffectInstance::getImageFromCacheAndConvertIfNeeded(bool /*useCache*/,
////just discard this entry
Format projectFormat;
getApp()->getProject()->getProjectDefaultFormat(&projectFormat);
RectD canonicalProject = projectFormat.toCanonicalFormat();
const RectD canonicalProject = projectFormat.toCanonicalFormat();
if ( canonicalProject != (*it)->getRoD() ) {
appPTR->removeFromNodeCache(*it);
continue;
Expand Down Expand Up @@ -1740,22 +1734,12 @@ EffectInstance::getImageFromCacheAndConvertIfNeeded(bool /*useCache*/,
//The rodParam might be different of oldParams->getRoD() simply because the RoD is dependent on the mipmap level
const RectD & rod = rodParam ? *rodParam : oldParams->getRoD();


//RectD imgToConvertCanonical;
//imgToConvertBounds.toCanonical(imageToConvert->getMipMapLevel(), imageToConvert->getPixelAspectRatio(), rod, &imgToConvertCanonical);
RectI downscaledBounds;
rod.toPixelEnclosing(mipMapLevel, imageToConvert->getPixelAspectRatio(), &downscaledBounds);
//imgToConvertCanonical.toPixelEnclosing(imageToConvert->getMipMapLevel(), imageToConvert->getPixelAspectRatio(), &imgToConvertBounds);
//imgToConvertCanonical.toPixelEnclosing(mipMapLevel, imageToConvert->getPixelAspectRatio(), &downscaledBounds);
RectI downscaledBounds = rod.toPixelEnclosing(mipMapLevel, imageToConvert->getPixelAspectRatio());

if (boundsParam) {
downscaledBounds.merge(*boundsParam);
}

//RectI pixelRoD;
//rod.toPixelEnclosing(mipMapLevel, oldParams->getPixelAspectRatio(), &pixelRoD);
//downscaledBounds.intersect(pixelRoD, &downscaledBounds);

ImageParamsPtr imageParams = Image::makeParams(rod,
downscaledBounds,
oldParams->getPixelAspectRatio(),
Expand Down Expand Up @@ -1786,9 +1770,9 @@ EffectInstance::getImageFromCacheAndConvertIfNeeded(bool /*useCache*/,
*/
int downscaleLevels = img->getMipMapLevel() - imageToConvert->getMipMapLevel();
RectI dstRoi = imgToConvertBounds.downscalePowerOfTwoSmallestEnclosing(downscaleLevels);
dstRoi.intersect(downscaledBounds, &dstRoi);
dstRoi.clipIfOverlaps(downscaledBounds);
dstRoi = dstRoi.upscalePowerOfTwo(downscaleLevels);
dstRoi.intersect(imgToConvertBounds, &dstRoi);
dstRoi.clipIfOverlaps(imgToConvertBounds);

if (imgToConvertBounds.area() > 1) {
imageToConvert->downscaleMipMap( rod,
Expand Down Expand Up @@ -2229,11 +2213,9 @@ EffectInstance::Implementation::tiledRenderingFunctor(const RectToRender & rectT


///Upscale the RoI to a region in the full scale image so it is in canonical coordinates
RectD canonicalRectToRender;
renderMappedRectToRender.toCanonical(renderMappedMipMapLevel, par, rod, &canonicalRectToRender);
if (renderFullScaleThenDownscale) {
assert(mipMapLevel > 0 && renderMappedMipMapLevel != mipMapLevel);
canonicalRectToRender.toPixelEnclosing(mipMapLevel, par, &downscaledRectToRender);
downscaledRectToRender = renderMappedRectToRender.toNewMipMapLevel(renderMappedMipMapLevel, mipMapLevel, par, rod);
}

// at this point, it may be unnecessary to call render because it was done a long time ago => check the bitmap here!
Expand Down Expand Up @@ -2296,10 +2278,9 @@ EffectInstance::Implementation::tiledRenderingFunctor(const RectToRender & rectT
RenderScale scale( Image::getScaleFromMipMapLevel(mipMapLevel) );
// check the dimensions of all input and output images
const RectD & dstRodCanonical = firstPlaneToRender.renderMappedImage->getRoD();
RectI dstBounds;
dstRodCanonical.toPixelEnclosing(firstPlaneToRender.renderMappedImage->getMipMapLevel(), par, &dstBounds); // compute dstRod at level 0
RectI dstRealBounds = firstPlaneToRender.renderMappedImage->getBounds();
const RectI dstBounds = dstRodCanonical.toPixelEnclosing(firstPlaneToRender.renderMappedImage->getMipMapLevel(), par); // compute dstRod at level 0
if (!frameArgs->tilesSupported) {
const RectI dstRealBounds = firstPlaneToRender.renderMappedImage->getBounds();
assert(dstRealBounds.x1 == dstBounds.x1);
assert(dstRealBounds.x2 == dstBounds.x2);
assert(dstRealBounds.y1 == dstBounds.y1);
Expand All @@ -2311,8 +2292,7 @@ EffectInstance::Implementation::tiledRenderingFunctor(const RectToRender & rectT
++it) {
for (ImageList::const_iterator it2 = it->second.begin(); it2 != it->second.end(); ++it2) {
const RectD & srcRodCanonical = (*it2)->getRoD();
RectI srcBounds;
srcRodCanonical.toPixelEnclosing( (*it2)->getMipMapLevel(), (*it2)->getPixelAspectRatio(), &srcBounds ); // compute srcRod at level 0
const RectI srcBounds = srcRodCanonical.toPixelEnclosing( (*it2)->getMipMapLevel(), (*it2)->getPixelAspectRatio() ); // compute srcRod at level 0

if (!frameArgs->tilesSupported) {
// http://openfx.sourceforge.net/Documentation/1.3/ofxProgrammingReference.html#kOfxImageEffectPropSupportsTiles
Expand All @@ -2325,7 +2305,7 @@ EffectInstance::Implementation::tiledRenderingFunctor(const RectToRender & rectT
* Blur will actually retrieve the image from the cache and downscale it rather than recompute it.
* Since the Writer does not support tiles, the Blur image is the full image and not a tile, which can be veryfied by
*
* blurCachedImage->getRod().toPixelEnclosing(blurCachedImage->getMipMapLevel(), blurCachedImage->getPixelAspectRatio(), &bounds)
* bounds = blurCachedImage->getRod().toPixelEnclosing(blurCachedImage->getMipMapLevel(), blurCachedImage->getPixelAspectRatio())
*
* Since the Blur RoD changed (the RoD at mmlevel 0 is different than the ROD at mmlevel 1),
* the resulting bounds of the downscaled image are not necessarily exactly result of the new downscaled RoD to the enclosing pixel
Expand Down Expand Up @@ -2553,8 +2533,7 @@ EffectInstance::Implementation::renderHandler(const EffectTLSDataPtr& tls,

///then upscale
const RectD & rod = sourceImage->getRoD();
RectI bounds;
rod.toPixelEnclosing(it->second.renderMappedImage->getMipMapLevel(), it->second.renderMappedImage->getPixelAspectRatio(), &bounds);
const RectI bounds = rod.toPixelEnclosing(it->second.renderMappedImage->getMipMapLevel(), it->second.renderMappedImage->getPixelAspectRatio());
ImagePtr inputPlane = std::make_shared<Image>(it->first,
rod,
bounds,
Expand All @@ -2576,8 +2555,7 @@ EffectInstance::Implementation::renderHandler(const EffectTLSDataPtr& tls,
if ( ( it->second.downscaleImage->getComponents() != idIt->second->getComponents() ) || ( it->second.downscaleImage->getBitDepth() != idIt->second->getBitDepth() ) ) {
ViewerColorSpaceEnum colorspace = _publicInterface->getApp()->getDefaultColorSpaceForBitDepth( idIt->second->getBitDepth() );
ViewerColorSpaceEnum dstColorspace = _publicInterface->getApp()->getDefaultColorSpaceForBitDepth( it->second.fullscaleImage->getBitDepth() );
RectI convertWindow;
idIt->second->getBounds().intersect(downscaledRectToRender, &convertWindow);
const RectI convertWindow = idIt->second->getBounds().intersect(downscaledRectToRender);
idIt->second->convertToFormat( convertWindow, colorspace, dstColorspace, 3, false, false, it->second.downscaleImage.get() );
} else {
it->second.downscaleImage->pasteFrom(*(idIt->second), downscaledRectToRender, false, glContext);
Expand Down

0 comments on commit b134344

Please sign in to comment.