Fix rendering bugs caused by intersecting rects with different mipMapLevels. #918
There are no files selected for viewing
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
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -387,7 +387,7 @@ EffectInstance::renderRoI(const RenderRoIArgs & args, | |
| renderMappedMipMapLevel = args.mipMapLevel; | ||
| } | ||
| RenderScale renderMappedScale( Image::getScaleFromMipMapLevel(renderMappedMipMapLevel) ); | ||
| assert(isSupportedRenderScale(supportsRS, renderMappedScale)); | ||
|
|
||
|
|
||
| const FrameViewRequest* requestPassData = 0; | ||
|
|
@@ -397,26 +397,27 @@ EffectInstance::renderRoI(const RenderRoIArgs & args, | |
|
|
||
| //////////////////////////////////////////////////////////////////////////////////////////////////////////////////////// | ||
| ////////////////////////////// Get the RoD /////////////////////////////////////////////////////////////// | ||
|
|
||
| RectD rodNc; //!< rod is in canonical coordinates | ||
| bool isProjectFormat = false; | ||
| { | ||
| ///if the rod is already passed as parameter, just use it and don't call getRegionOfDefinition | ||
| if ( !args.preComputedRoD.isNull() ) { | ||
| rodNc = args.preComputedRoD; | ||
| } else { | ||
| ///Check if the pre-pass already has the RoD | ||
| if (requestPassData) { | ||
| rodNc = requestPassData->globalData.rod; | ||
| isProjectFormat = requestPassData->globalData.isProjectFormat; | ||
| } else { | ||
| assert(isSupportedRenderScale(supportsRS, renderMappedScale)); | ||
| StatusEnum stat = getRegionOfDefinition_public(nodeHash, args.time, renderMappedScale, args.view, &rodNc, &isProjectFormat); | ||
|
|
||
| ///The rod might be NULL for a roto that has no beziers and no input | ||
| if (stat == eStatusFailed) { | ||
| ///if getRoD fails, this might be because the RoD is null after all (e.g: an empty Roto node), we don't want the render to fail | ||
| return eRenderRoIRetCodeOk; | ||
| } else if ( rodNc.isNull() ) { | ||
| //Nothing to render | ||
| return eRenderRoIRetCodeOk; | ||
| } | ||
|
|
@@ -432,18 +433,9 @@ EffectInstance::renderRoI(const RenderRoIArgs & args, | |
| } | ||
| } | ||
| } | ||
| const RectD rod = rodNc; | ||
| //////////////////////////////////////////////////////////////////////////////////////////////////////////////////////// | ||
| ////////////////////////////// End get RoD /////////////////////////////////////////////////////////////// | ||
|
|
||
| ///Determine needed planes | ||
| ComponentsNeededMapPtr neededComps = std::make_shared<ComponentsNeededMap>(); | ||
|
|
@@ -553,7 +545,7 @@ EffectInstance::renderRoI(const RenderRoIArgs & args, | |
| double inputTimeIdentity = 0.; | ||
| int inputNbIdentity; | ||
| ViewIdx inputIdentityView(args.view); | ||
| assert(isSupportedRenderScale(supportsRS, renderMappedScale)); | ||
| bool identity; | ||
| const RectI pixelRod = rod.toPixelEnclosing(args.mipMapLevel, par); | ||
| ViewInvarianceLevel viewInvariance = isViewInvariant(); | ||
|
|
@@ -736,39 +728,44 @@ EffectInstance::renderRoI(const RenderRoIArgs & args, | |
| //////////////////////////////////////////////////////////////////////////////////////////////////////////////////////// | ||
| ////////////////////////////// Compute RoI depending on render scale /////////////////////////////////////////////////// | ||
|
|
||
| RectI downscaledImageBoundsNc = rod.toPixelEnclosing(args.mipMapLevel, par); | ||
| RectI upscaledImageBoundsNc = rod.toPixelEnclosing(0, par); | ||
| RectI roi; | ||
|
|
||
| if (renderFullScaleThenDownscale) { | ||
| //We cache 'image', hence the RoI should be expressed in its coordinates | ||
| //renderRoIInternal should check the bitmap of 'image' and not downscaledImage! | ||
| roi = args.roi.toNewMipMapLevel(args.mipMapLevel, 0, par, rod); | ||
|
|
||
| if (frameArgs->tilesSupported && !roi.clipIfOverlaps(upscaledImageBoundsNc)) { | ||
| return eRenderRoIRetCodeOk; | ||
| } | ||
| assert( upscaledImageBoundsNc.contains(roi)); | ||
| } else { | ||
| roi = args.roi; | ||
|
|
||
| if (frameArgs->tilesSupported && !roi.clipIfOverlaps(downscaledImageBoundsNc)) { | ||
| return eRenderRoIRetCodeOk; | ||
| } | ||
| assert(downscaledImageBoundsNc.contains(roi)); | ||
|
There was a problem hiding this comment. Attach any input to viewer (and/or scale) and assert fail. There was a problem hiding this comment. Fixed by #930 . I accidentally made the assert apply to the case where tiles are not supported, which does not match the original code. I've restored the old behavior so the assert only applies when tiles are supported. |
||
| } | ||
|
|
||
| /* | ||
| * Keep in memory what the user has requested, and change the roi to the full bounds if the effect doesn't support tiles | ||
| */ | ||
| const RectI originalRoI = roi; | ||
| // Store the mipMapLevel for originalRoI here because renderFullScaleThenDownscale may change later and we'll lose | ||
| // the ability to recover this information when we potentially need it for a downscale operation later. | ||
| const unsigned int originalRoIMipMapLevel = renderFullScaleThenDownscale ? 0 : args.mipMapLevel; | ||
|
|
||
| if (frameArgs->tilesSupported) { | ||
| #ifndef NATRON_ALWAYS_ALLOCATE_FULL_IMAGE_BOUNDS | ||
| ///just allocate the roi | ||
| const RectI upscaledRoI = renderFullScaleThenDownscale ? roi : roi.toNewMipMapLevel(args.mipMapLevel, 0, par, rod); | ||
| upscaledImageBoundsNc.clipIfOverlaps(upscaledRoI); | ||
| downscaledImageBoundsNc.clipIfOverlaps(args.roi); | ||
| #endif | ||
| } else { | ||
| roi = renderFullScaleThenDownscale ? upscaledImageBoundsNc : downscaledImageBoundsNc; | ||
| } | ||
|
|
||
|
|
@@ -1199,10 +1196,9 @@ EffectInstance::renderRoI(const RenderRoIArgs & args, | |
| if (!input) { | ||
| continue; | ||
| } | ||
| ParallelRenderArgsPtr inputFrameArgs = input->getParallelRenderArgsTLS(); | ||
| U64 inputHash = (inputFrameArgs) ? inputFrameArgs->nodeHash : input->getHash(); | ||
| StatusEnum stat = input->getRegionOfDefinition_public(inputHash, args.time, args.scale, args.view, &inputRod, nullptr); | ||
| if ( (stat != eStatusOK) && !inputRod.isNull() ) { | ||
| break; | ||
| } | ||
|
|
@@ -1757,7 +1753,8 @@ EffectInstance::renderRoI(const RenderRoIArgs & args, | |
| assert(comp); | ||
| ///The image might need to be converted to fit the original requested format | ||
| if (comp) { | ||
| const RectI downscaledOriginalRoI = originalRoI.toNewMipMapLevel(originalRoIMipMapLevel, args.mipMapLevel, par, rod); | ||
| it->second.downscaleImage = convertPlanesFormatsIfNeeded(getApp(), it->second.downscaleImage, downscaledOriginalRoI, *comp, args.bitdepth, useAlpha0ForRGBToRGBAConversion, planesToRender->outputPremult, -1); | ||
| assert(it->second.downscaleImage->getComponents() == *comp && it->second.downscaleImage->getBitDepth() == args.bitdepth); | ||
|
|
||
| StorageModeEnum imageStorage = it->second.downscaleImage->getStorageMode(); | ||
|
|
@@ -1909,7 +1906,7 @@ EffectInstance::renderRoIInternal(EffectInstance* self, | |
|
|
||
|
|
||
| if (callBegin) { | ||
| assert( self->isSupportedRenderScale(self->supportsRenderScaleMaybe(), renderMappedScale) ); | ||
| if (self->beginSequenceRender_public(time, time, 1, !appPTR->isBackground(), renderMappedScale, isSequentialRender, | ||
| isRenderMadeInResponseToUserInteraction, frameArgs->draftMode, view, planesToRender->useOpenGL, planesToRender->glContextData) == eStatusFailed) { | ||
| renderStatus = eRenderingFunctorRetFailed; | ||
|
|
@@ -1997,7 +1994,7 @@ EffectInstance::renderRoIInternal(EffectInstance* self, | |
|
|
||
| ///never call endsequence render here if the render is sequential | ||
| if (callBegin) { | ||
| assert( self->isSupportedRenderScale(self->supportsRenderScaleMaybe(), renderMappedScale) ); | ||
| if (self->endSequenceRender_public(time, time, time, false, renderMappedScale, | ||
| isSequentialRender, | ||
| isRenderMadeInResponseToUserInteraction, | ||
|
|
||
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
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think it's good practice to have the condition actually modify the context. I think we should move all clipIfOverlaps() calls out of the conditions.
This condition would then look like:
but I'm not sure that's the intent here, because a few lines below there's
const RectI originalRoI = roi;butroiis not the original but the clipped roi... what do you think?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If roi was used elsewhere in the conditional I might agree, but it isn't so I disagree with the proposed substitute. In my opinion the proposal is unnecessarily verbose and it assigns a name to a value that really is only needed for the conditional that follows. In my experience, assigning names to values is usually a signal that they will be used more than once, but in cases like this it is not true which is why I don't really see the need for giving it a name.
Unfortunately I don't know the history of why the variable below is called originalRoI, but I suspect it is intended to store the value of roi before any further modifications occur after that point. I'm not sure what a better name might be given the complexity of this function and how clipping is applied(or not) based on a variety of conditions. I too was initially confused by the use of the term 'original' given all the possible roi transformations that occur before that declaration.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think side effects in conditions are bad practice in general (except for some constructs where the language instructs you to use side effects like for or while), because a condition should check the state, not modify it. This makes the code error-prone and harder to read.