Refactor clipIfOverlaps() so it does not have a return value. (#947)

This change removes the return value for clipIfOverlaps() and updates all callers. This was done to address a comment made in #930 (comment) - Removed return value from RectI::clipIfOverlaps() and RectD::clipIfOverlaps() - Introduced clip() that updates a RectI or RectD with the intersection of itself and another rectangle. - Reworked several clipIfOverlaps() callers to use clip() or intersect() instead of clipIfOverlaps().
NatronGitHub · Feb 20, 2024 · 95fd6d5 · 95fd6d5
1 parent 985103e
commit 95fd6d5
Show file tree

Hide file tree

Showing 7 changed files with 106 additions and 78 deletions.
diff --git a/Engine/EffectInstanceRenderRoI.cpp b/Engine/EffectInstanceRenderRoI.cpp
@@ -738,7 +738,8 @@ EffectInstance::renderRoI(const RenderRoIArgs & args,
         roi = args.roi.toNewMipmapLevel(args.mipmapLevel, 0, par, rod);
 
         if (frameArgs->tilesSupported) {
-            if (!roi.clipIfOverlaps(upscaledImageBoundsNc)) {
+            roi.clip(upscaledImageBoundsNc);
+            if (roi.isNull()) {
                 return eRenderRoIRetCodeOk;
             }
             assert(upscaledImageBoundsNc.contains(roi));
@@ -747,7 +748,8 @@ EffectInstance::renderRoI(const RenderRoIArgs & args,
         roi = args.roi;
 
         if (frameArgs->tilesSupported) {
-            if (!roi.clipIfOverlaps(downscaledImageBoundsNc)) {
+            roi.clip(downscaledImageBoundsNc);
+            if (roi.isNull()) {
                 return eRenderRoIRetCodeOk;
             }
             assert(downscaledImageBoundsNc.contains(roi));
@@ -844,8 +846,8 @@ EffectInstance::renderRoI(const RenderRoIArgs & args,
                 renderMappedMipmapLevel = args.mipmapLevel;
                 renderMappedScale = RenderScale::fromMipmapLevel(renderMappedMipmapLevel);
                 if (frameArgs->tilesSupported) {
-                    roi = args.roi;
-                    if ( !roi.clipIfOverlaps(downscaledImageBoundsNc) ) {
+                    roi = args.roi.intersect(downscaledImageBoundsNc);
+                    if ( roi.isNull() ) {
                         return eRenderRoIRetCodeOk;
                     }
                 } else {

diff --git a/Engine/Image.cpp b/Engine/Image.cpp
@@ -463,8 +463,11 @@ RectI
 Bitmap::minimalNonMarkedBbox(const RectI & roi) const
 {
     RectI realRoi = roi;
-    if (_dirtyZoneSet && !realRoi.clipIfOverlaps(_dirtyZone)) {
-        return RectI();
+    if (_dirtyZoneSet) {
+        realRoi.clip(_dirtyZone);
+        if (realRoi.isNull()) {
+            return RectI();
+        }
     }
 
     return minimalNonMarkedBbox_internal<0>(realRoi, _bounds, _map, NULL);
@@ -475,8 +478,11 @@ Bitmap::minimalNonMarkedRects(const RectI & roi,
                               std::list<RectI>& ret) const
 {
     RectI realRoi = roi;
-    if (_dirtyZoneSet && !realRoi.clipIfOverlaps(_dirtyZone)) {
-        return;
+    if (_dirtyZoneSet) {
+        realRoi.clip(_dirtyZone);
+        if (realRoi.isNull()) {
+            return;
+        }
     }
     minimalNonMarkedRects_internal<0>(realRoi, _bounds, _map, ret, NULL);
 }
@@ -487,9 +493,12 @@ Bitmap::minimalNonMarkedBbox_trimap(const RectI & roi,
                                     bool* isBeingRenderedElsewhere) const
 {
     RectI realRoi = roi;
-    if (_dirtyZoneSet && !realRoi.clipIfOverlaps(_dirtyZone)) {
-        *isBeingRenderedElsewhere = false;
-        return RectI();
+    if (_dirtyZoneSet) {
+        realRoi.clip(_dirtyZone);
+        if (realRoi.isNull()) {
+            *isBeingRenderedElsewhere = false;
+            return RectI();
+        }
     }
 
     return minimalNonMarkedBbox_internal<1>(realRoi, _bounds, _map, isBeingRenderedElsewhere);
@@ -501,9 +510,12 @@ Bitmap::minimalNonMarkedRects_trimap(const RectI & roi,
                                      bool* isBeingRenderedElsewhere) const
 {
     RectI realRoi = roi;
-    if (_dirtyZoneSet && !realRoi.clipIfOverlaps(_dirtyZone)) {
-        *isBeingRenderedElsewhere = false;
-        return;
+    if (_dirtyZoneSet) {
+        realRoi.clip(_dirtyZone);
+        if (realRoi.isNull()) {
+            *isBeingRenderedElsewhere = false;
+            return;
+        }
     }
     minimalNonMarkedRects_internal<1>(realRoi, _bounds, _map, ret, isBeingRenderedElsewhere);
 }
@@ -905,13 +917,10 @@ Image::pasteFromForDepth(const Image & srcImg,
     assert( !srcBounds.isNull() );
 
     // only copy the intersection of roi, bounds and otherBounds
-    RectI roi = srcRoi;
-    if (!roi.clipIfOverlaps(bounds)) {
-        // no intersection between roi and the bounds of this image
-        return;
-    }
-    if (!roi.clipIfOverlaps(srcBounds)) {
-        // no intersection between roi and the bounds of the other image
+    const RectI roi = srcRoi.intersect(bounds).intersect(srcBounds);
+    if (roi.isNull()) {
+        // no intersection between srcRoi and the bounds of this image or
+        // the bounds of the other image.
         return;
     }
 
@@ -1229,17 +1238,14 @@ Image::pasteFrom(const Image & src,
         glCheckError();
     } else if ( (thisStorage == eStorageModeGLTex) && (otherStorage != eStorageModeGLTex) ) {
         // RAM image to OpenGL texture
-        RectI dstBounds = getBounds();
-        RectI srcBounds = src.getBounds();
+        const RectI& dstBounds = getBounds();
+        const RectI& srcBounds = src.getBounds();
 
         // only copy the intersection of roi, bounds and otherBounds
-        RectI roi = srcRoi;
-        if (!roi.clipIfOverlaps(dstBounds)) {
-            // no intersection between roi and the bounds of this image
-            return;
-        }
-        if (!roi.clipIfOverlaps(srcBounds)) {
-            // no intersection between roi and the bounds of the other image
+        const RectI roi = srcRoi.intersect(dstBounds).intersect(srcBounds);
+        if (roi.isNull()) {
+            // no intersection between srcRoi and the bounds of this image or
+            // the bounds of the other image
             return;
         }
         GLuint pboID = glContext->getPBOId();
@@ -1301,17 +1307,14 @@ Image::pasteFrom(const Image & src,
     } else if ( (thisStorage != eStorageModeGLTex) && (otherStorage == eStorageModeGLTex) ) {
         // OpenGL texture to RAM image
 
-        RectI dstBounds = getBounds();
-        RectI srcBounds = src.getBounds();
+        const RectI& dstBounds = getBounds();
+        const RectI& srcBounds = src.getBounds();
 
         // only copy the intersection of roi, bounds and otherBounds
-        RectI roi = srcRoi;
-        if (!roi.clipIfOverlaps(dstBounds)) {
-            // no intersection between roi and the bounds of this image
-            return;
-        }
-        if (!roi.clipIfOverlaps(srcBounds)) {
-            // no intersection between roi and the bounds of the other image
+        const RectI roi = srcRoi.intersect(dstBounds).intersect(srcBounds);
+        if (roi.isNull()) {
+            // no intersection between srcRoi and the bounds of this image or
+            // the bounds of the other image
             return;
         }
 
@@ -1385,8 +1388,8 @@ Image::fillForDepthForComponents(const RectI & roi_,
 {
     assert( (getBitDepth() == eImageBitDepthByte && sizeof(PIX) == 1) || (getBitDepth() == eImageBitDepthShort && sizeof(PIX) == 2) || (getBitDepth() == eImageBitDepthFloat && sizeof(PIX) == 4) );
 
-    RectI roi = roi_;
-    if (!roi.clipIfOverlaps(_bounds)) {
+    const RectI roi = roi_.intersect(_bounds);
+    if (roi.isNull()) {
         // no intersection between roi and the bounds of the image
         return;
     }
@@ -1454,8 +1457,8 @@ Image::fill(const RectI & roi,
     QWriteLocker k(&_entryLock);
 
     if (getStorageMode() == eStorageModeGLTex) {
-        RectI realRoI = roi;
-        if (!realRoI.clipIfOverlaps(_bounds)) {
+        const RectI realRoI = roi.intersect(_bounds);
+        if (realRoI.isNull()) {
             // no intersection between roi and the bounds of the image
             return;
         }
@@ -1788,6 +1791,7 @@ Image::halveRoIForDepth(const RectI & roi,
                         bool copyBitMap,
                         Image* output) const
 {
+    assert( _bounds.contains(roi) );
     assert( (getBitDepth() == eImageBitDepthByte && sizeof(PIX) == 1) ||
             (getBitDepth() == eImageBitDepthShort && sizeof(PIX) == 2) ||
             (getBitDepth() == eImageBitDepthFloat && sizeof(PIX) == 4) );
@@ -1823,8 +1827,7 @@ Image::halveRoIForDepth(const RectI & roi,
     assert( getComponents() == output->getComponents() );
 
     RectI dstRoI;
-    RectI srcRoI = roi;
-    srcRoI.clipIfOverlaps(srcBounds); // intersect srcRoI with the region of definition
+    const RectI srcRoI = roi.intersect(srcBounds); // intersect RoI with the region of definition
 #ifdef DEBUG_NAN
     assert(!checkForNaNsNoLock(srcRoI));
 #endif
@@ -1951,6 +1954,7 @@ Image::halveRoI(const RectI & roi,
                 bool copyBitMap,
                 Image* output) const
 {
+    assert( _bounds.contains(roi) );
     switch ( getBitDepth() ) {
     case eImageBitDepthByte:
         halveRoIForDepth<unsigned char, 255>(roi, copyBitMap,  output);
@@ -1980,6 +1984,7 @@ Image::halve1DImageForDepth(const RectI & roi,
 
     assert(width == 1 || height == 1); /// must be 1D
     assert( output->getComponents() == getComponents() );
+    assert( _bounds.contains(roi) );
 
     /// Take the lock for both bitmaps since we're about to read/write from them!
     QWriteLocker k1(&output->_entryLock);
@@ -2075,8 +2080,7 @@ Image::downscaleMipmap(const RectD& dstRod,
     ///You should not call this function with a level equal to 0.
     assert(toLevel >  fromLevel);
 
-    assert(_bounds.x1 <= roi.x1 && roi.x2 <= _bounds.x2 &&
-           _bounds.y1 <= roi.y1 && roi.y2 <= _bounds.y2);
+    assert(_bounds.contains(roi));
     double par = getPixelAspectRatio();
     unsigned int downscaleLvls = toLevel - fromLevel;
 
@@ -2279,6 +2283,8 @@ Image::buildMipmapLevel(const RectD& dstRoD,
                         bool copyBitMap,
                         Image* output) const
 {
+    assert(_bounds.contains(roi));
+
     ///The last mip map level we will make with closestPo2
     RectI lastLevelRoI = roi.downscalePowerOfTwoSmallestEnclosing(level);
 

diff --git a/Engine/Lut.cpp b/Engine/Lut.cpp
@@ -167,11 +167,12 @@ LutManager::~LutManager()
     }
 }
 
-static bool
-clip(RectI* what,
-     const RectI & to)
+static RectI
+clip2(const RectI& what,
+      const RectI& srcBounds,
+      const RectI& dstBounds)
 {
-    return what->clipIfOverlaps(to);
+    return what.intersect(srcBounds).intersect(dstBounds);
 }
 
 #ifdef DEAD_CODE
@@ -445,9 +446,9 @@ Lut::to_byte_packed(unsigned char* to,
                     bool premult) const
 {
     ///clip the conversion rect to srcBounds and dstBounds
-    RectI rect = conversionRect;
+    const RectI rect = clip2(conversionRect, srcBounds, dstBounds);
 
-    if ( !clip(&rect, srcBounds) || !clip(&rect, dstBounds) ) {
+    if ( rect.isNull() ) {
         return;
     }
 
@@ -546,9 +547,8 @@ Lut::to_float_packed(float* to,
                      bool premult) const
 {
     ///clip the conversion rect to srcBounds and dstBounds
-    RectI rect = conversionRect;
-
-    if ( !clip(&rect, srcBounds) || !clip(&rect, dstBounds) ) {
+    const RectI rect = clip2(conversionRect, srcBounds, dstBounds);
+    if ( rect.isNull()) {
         return;
     }
 
@@ -658,8 +658,8 @@ Lut::from_byte_packed(float* to,
     }
 
     ///clip the conversion rect to srcBounds and dstBounds
-    RectI rect = conversionRect;
-    if ( !clip(&rect, srcBounds) || !clip(&rect, dstBounds) ) {
+    const RectI rect = clip2(conversionRect, srcBounds, dstBounds);
+    if ( rect.isNull() ) {
         return;
     }
 
@@ -752,8 +752,8 @@ Lut::from_float_packed(float* to,
     }
 
     ///clip the conversion rect to srcBounds and dstBounds
-    RectI rect = conversionRect;
-    if ( !clip(&rect, srcBounds) || !clip(&rect, dstBounds) ) {
+    const RectI rect = clip2(conversionRect, srcBounds, dstBounds);
+    if ( rect.isNull() ) {
         return;
     }
 
@@ -863,8 +863,8 @@ from_byte_packed(float *to,
     }
 
     ///clip the conversion rect to srcBounds and dstBounds
-    RectI rect = conversionRect;
-    if ( !clip(&rect, srcBounds) || !clip(&rect, dstBounds) ) {
+    const RectI rect = clip2(conversionRect, srcBounds, dstBounds);
+    if ( rect.isNull() ) {
         return;
     }
 
@@ -933,8 +933,8 @@ from_float_packed(float *to,
 
 
     ///clip the conversion rect to srcBounds and dstBounds
-    RectI rect = conversionRect;
-    if ( !clip(&rect, srcBounds) || !clip(&rect, dstBounds) ) {
+    const RectI rect = clip2(conversionRect, srcBounds, dstBounds);
+    if ( rect.isNull()) {
         return;
     }
 
@@ -1132,8 +1132,8 @@ to_byte_packed(unsigned char* to,
     }
 
     ///clip the conversion rect to srcBounds and dstBounds
-    RectI rect = conversionRect;
-    if ( !clip(&rect, srcBounds) || !clip(&rect, dstBounds) ) {
+    const RectI rect = clip2(conversionRect, srcBounds, dstBounds);
+    if ( rect.isNull() ) {
         return;
     }
 
@@ -1241,8 +1241,8 @@ to_float_packed(float* to,
     }
 
     ///clip the conversion rect to srcBounds and dstBounds
-    RectI rect = conversionRect;
-    if ( !clip(&rect, srcBounds) || !clip(&rect, dstBounds) ) {
+    const RectI rect = clip2(conversionRect, srcBounds, dstBounds);
+    if ( rect.isNull() ) {
         return;
     }
 

diff --git a/Engine/RectD.h b/Engine/RectD.h
@@ -265,14 +265,23 @@ class RectD
         return intersect(RectD(l, b, r, t));
     }
 
+    /**
+     * @brief Updates this rectangle to the intersection of this rectangle and |rect|. If there is no overlap, this rectangle is cleared so it represents a
+     * null rectangle.
+     **/
+    void clip(const RectD& rect) {
+        if (!intersectInternal(rect, this)) {
+            // |rect| does not intersect with *this so clear this object so it becomes a null rectangle.
+            clear();
+        }
+    }
+
     /**
      * @brief Updates this rectangle to the intersection rectangle if this rectangle overlaps with rect. If there is no overlap then this rectangle remains unchanged.
-     *
-     * @returns True if the rectangles overlap and this rectangle was clipped.
      **/
-    bool clipIfOverlaps(const RectD& rect)
+    void clipIfOverlaps(const RectD& rect)
     {
-        return intersectInternal(rect, this);
+        intersectInternal(rect, this);
     }
 
     /// returns true if the rect passed as parameter is intersects this one

diff --git a/Engine/RectI.h b/Engine/RectI.h
@@ -405,14 +405,23 @@ class RectI
         return intersect(RectI(x1_, y1_, x2_, y2_));
     }
 
+    /**
+     * @brief Updates this rectangle to the intersection of this rectangle and |rect|. If there is no overlap, this rectangle is cleared so it represents a
+     * null rectangle.
+     **/
+    void clip(const RectI& rect) {
+        if (!intersectInternal(rect, this)) {
+            // |rect| does not intersect with *this so clear this object so it becomes a null rectangle.
+            clear();
+        }
+    }
+
     /**
      * @brief Updates this rectangle to the intersection rectangle if this rectangle overlaps with rect. If there is no overlap then this rectangle remains unchanged.
-     *
-     * @returns True if the rectangles overlap and this rectangle was clipped.
      **/
-    bool clipIfOverlaps(const RectI& rect)
+    void clipIfOverlaps(const RectI& rect)
     {
-        return intersectInternal(rect, this);
+        intersectInternal(rect, this);
     }
 
     /// returns true if the rect passed as parameter  intersects this one