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

This change removes the return value for clipIfOverlaps() and updates all callers. This was done to address a comment made in NatronGitHub#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().
acolwell · Feb 13, 2024 · 3e5fc8b · 3e5fc8b
1 parent ac93534
commit 3e5fc8b
Show file tree

Hide file tree

Showing 7 changed files with 100 additions and 76 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;
         }
@@ -1823,8 +1826,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

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
+clip(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 = clip(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 = clip(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 = clip(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 = clip(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 = clip(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 = clip(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 = clip(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 = clip(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

diff --git a/Engine/ViewerInstance.cpp b/Engine/ViewerInstance.cpp
@@ -1003,7 +1003,8 @@ ViewerInstance::getViewerRoIAndTexture(const RectD& rod,
             for (std::list<RectD>::iterator it = partialRects.begin(); it != partialRects.end(); ++it) {
                 RectI pixelRect = it->toPixelEnclosing(mipmapLevel, outArgs->params->pixelAspectRatio);
                 ///Intersect to the RoI
-                if ( pixelRect.clipIfOverlaps(outArgs->params->roi) ) {
+                pixelRect.clip(outArgs->params->roi);
+                if ( !pixelRect.isNull() ) {
                     tile.rect.set(pixelRect);
                     tile.rectRounded  = pixelRect;
                     tile.rect.closestPo2 = 1 << mipmapLevel;

diff --git a/Gui/ViewerGLPrivate.cpp b/Gui/ViewerGLPrivate.cpp
@@ -294,7 +294,8 @@ ViewerGL::Implementation::drawRenderingVAO(unsigned int mipmapLevel,
         {
             QMutexLocker l(&this->userRoIMutex);
             //if the userRoI isn't intersecting the rod, just don't render anything
-            if ( !rod.clipIfOverlaps(this->userRoI) ) {
+            rod.clip(this->userRoI);
+            if ( rod.isNull() ) {
                 return;
             }
         }