Fix rendering bugs caused by intersecting rects with different mipMapLevels. #918
Conversation
acolwell
force-pushed
the
fix_mipmaplevel_mismatches
branch
from
31f65c3 to
3cda4f1
Compare
- Updated rod computation so that rod could be declared as const. This is to make it easier to prove that this variable does not change beyond this point. - Moved initial roi computation down to the image bounds computation code since that is where it is initially used and gets updated. - Introduced isSupportedRenderScale() helper to remove duplicate code related to RenderScale support.
…Levels. This change fixes 2 bugs where RectI objects containing information for different mipMapLevels were being intersected with eachother. This was causing visible compositing errors in the viewer when viewing the image zoomed out and partially clipped by the edge of the viewer. This fix just makes sure the RectIs are converted to the proper mipMapLevel before being intersected.
acolwell
force-pushed
the
fix_mipmaplevel_mismatches
branch
from
3cda4f1 to
b612b20
Compare
| 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.
| //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)) { |
There was a problem hiding this comment.
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:
if (frameArgs->tilesSupported) {
auto clipped = roi.clipIfOverlaps(upscaledImageBoundsNc);
if (!clipped) {
return eRenderRoIRetCodeOk;
}
}
but I'm not sure that's the intent here, because a few lines below there's const RectI originalRoI = roi; but roiis not the original but the clipped roi... what do you think?
There was a problem hiding this comment.
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.
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.
- Fixed assert() in Tests/FileSystemModel_Test.cpp that broke the debug build. - Fixed asserts in Engine/EffectInstanceRenderRoI.cpp so that they are only checked if tiling is supported. The assert was accidentally applied to all cases when the code was refactored by NatronGitHub#918 - Updated Linux CI to build and test release AND debug builds. This should help avoid such issues from sneaking in going forward.
- Fixed assert() in Tests/FileSystemModel_Test.cpp that broke the debug build. - Fixed asserts in Engine/EffectInstanceRenderRoI.cpp so that they are only checked if tiling is supported. The assert was accidentally applied to all cases when the code was refactored by NatronGitHub#918 - Updated Linux CI to build and test release AND debug builds. This should help avoid such issues from sneaking in going forward.
- Fixed assert() in Tests/FileSystemModel_Test.cpp that broke the debug build. - Fixed asserts in Engine/EffectInstanceRenderRoI.cpp so that they are only checked if tiling is supported. The assert was accidentally applied to all cases when the code was refactored by #918 - Updated Linux CI to build and test release AND debug builds. This should help avoid such issues from sneaking in going forward.
Thanks for submitting a pull request! Please provide enough information so that others can review your pull request. Additionally, make sure you've done all of these things:
PR Description
What type of PR is this? (Check one of the boxes below)
What does this pull request do?
This change fixes 2 bugs where RectI objects containing information for different mipMapLevels were being intersected with each other. This was causing visible compositing errors in the viewer when viewing the image zoomed out, partially clipped by the edge of the viewer, and then pressing the "Forces a new render for the current frame" button. This fix just makes sure the RectIs are converted to the proper mipMapLevel before being intersected.
I also included a few cleanup changes that do not modify existing functionality but make the code a little easier to follow and verify correctness.
Show a few screenshots (if this is a visual change)
Have you tested your changes (if applicable)? If so, how?
Yes. I've verified that my change fixes the rendering issues as shown in the image above.
Futher details of this pull request
These changes likely fix rendering issues in other scenarios as well. The one mentioned above is the easily repeatable one that I discovered and used for testing.
Technically, the isSupportedRenderScale() change isn't entirely necessary, but I noticed the cleanup opportunity while updating the rod code.