Fix asserts and build buster in debug build. #930
Merged
Conversation
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
- 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.
10 tasks
acolwell
referenced
this pull request
Oct 17, 2023
- Added helper functions to reduce redundant code, improve readability, and help clarify intent. - Added getSplitPath() and cleanPath() to FileSystemModel to make it easier to test and extend this functionality. - Added unit tests for some FileSystemModel functions to document existing behavior and make it easier to see how behavior changes with bug fixes. - Deleted unused code.
devernay
reviewed
Oct 18, 2023
| if (frameArgs->tilesSupported && !roi.clipIfOverlaps(downscaledImageBoundsNc)) { | ||
| return eRenderRoIRetCodeOk; | ||
| if (frameArgs->tilesSupported) { | ||
| if (!roi.clipIfOverlaps(downscaledImageBoundsNc)) { |
There was a problem hiding this comment.
Could we avoid side effects in conditions, and either have a const function overlaps(), and do the clip outside of the condition?
I think this clipIfOverlaps() function should be removed from the code, because it is used as a test at many places but also has a dangerous and misleading side effect.
No one expects a condition to modify the variable being tested. See c++ core guideline es.40 https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#es40-avoid-complicated-expressions
There was a problem hiding this comment.
I take your point and have some ideas on how to rework the clipIfOverlap() call sites so they don't need this type of function in a conditional. Can I defer those changes to a follow-up PR? I'd like to land this and #929 so that Windows and debug builds start working again.
There was a problem hiding this comment.
Friendly ping. I'd like to land this and #929 so the CI builds start working again. I'm happy to follow up with clipIfOverlap() fixes once the build and tests have been restored.
acolwell
added a commit
to acolwell/Natron
that referenced
this pull request
Dec 29, 2023
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
added a commit
to acolwell/Natron
that referenced
this pull request
Feb 13, 2024
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
added a commit
to acolwell/Natron
that referenced
this pull request
Feb 13, 2024
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
added a commit
to acolwell/Natron
that referenced
this pull request
Feb 13, 2024
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
added a commit
to acolwell/Natron
that referenced
this pull request
Feb 13, 2024
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
added a commit
to acolwell/Natron
that referenced
this pull request
Feb 14, 2024
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
added a commit
to acolwell/Natron
that referenced
this pull request
Feb 17, 2024
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().
10 tasks
acolwell
added a commit
that referenced
this pull request
Feb 20, 2024
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().
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
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?
It fixes some assert() issues in debug builds that were introduced by #901 and #918 . It also updates the Tests GitHub Action so that it also runs a Linux debug build and runs the test for that build.
Have you tested your changes (if applicable)? If so, how?
I've verified locally on Linux that debug builds work and pass their tests. I've also verified the GitHub action builds the debug and release builds on Linux. (https://github.com/acolwell/Natron/actions/runs/6552600450/job/17796244295) Currently the windows build is failing because #929 has not been approved and committed yet.