Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

FrameRange : Prevent creation of FrameRanges with negative steps #1372

Merged
merged 1 commit into from
Jul 14, 2023
Merged

FrameRange : Prevent creation of FrameRanges with negative steps #1372

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions Changes
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,11 @@
10.4.x.x (relative to 10.4.10.0)
========

Fixes
-----

- FrameRange : Prevented creation of FrameRanges with negative steps

10.4.10.0 (relative to 10.4.9.1)
========

Expand Down
8 changes: 8 additions & 0 deletions src/IECore/FrameRange.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,10 @@ FrameRange::FrameRange( Frame start, Frame end, Frame step ) : m_start( start ),
{
throw Exception( "FrameRange step cannot be zero" );
}
if ( step < 0 )
{
throw Exception( "FrameRange step cannot be negative. Consider using the reverse suffix instead." );
}
Comment on lines +61 to +64
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rejecting negative steps (rather than updating asList() to support them) is consistent with the start > end checks we're already doing, so this seems like the right fix. Looks like we also need to apply the step < 0 check in setStep() though, in which case maybe having the constructor call setStep() is simpler than duplicating the checks?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added the extra check on setStep() as well.
I decided against changing the constructor to call setStep() as I thought it would also imply we should do the same for setStart() and setEnd(), but then you get into a bunch of details on whether we should pre-initialize the values before calling those methods, and things actually became quite difficult to read.
In the current PR, yes, you end up with duplicated checks, but at least it's fully consistent and readable.

}

FrameRange::~FrameRange()
Expand Down Expand Up @@ -102,6 +106,10 @@ void FrameRange::setStep( Frame step )
{
throw Exception( "FrameRange step cannot be zero" );
}
if ( step < 0 )
{
throw Exception( "FrameRange step cannot be negative. Consider using the reverse suffix instead." );
}
m_step = step;
}

Expand Down
19 changes: 18 additions & 1 deletion test/IECore/FrameList.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,24 @@ def testReverseConstruction( self ) :
self.assertEqual( f.asList(), [ 5, 4, 3, 2, 1 ] )
self.assertEqual( IECore.frameListFromList( [ 5, 4, 3, 2, 1 ] ), f )

def testFrameRange( self ) :

f = IECore.FrameList.parse( "1-5" )
self.assertTrue( isinstance( f, IECore.FrameRange ) )
self.assertEqual( f.asList(), [ 1, 2, 3, 4, 5 ] )
# test step
f = IECore.FrameList.parse( "10-20x5" )
self.assertTrue( isinstance( f, IECore.FrameRange ) )
self.assertEqual( f.asList(), [ 10, 15, 20 ] )
# start must be smaller or equal to end
self.assertRaises( Exception, IECore.FrameList.parse, "5-1" )
# step must be positive
self.assertRaises( Exception, IECore.FrameList.parse, "1-5x0" )
self.assertRaises( Exception, IECore.FrameList.parse, "1-5x-1" )
self.assertRaises( Exception, IECore.FrameList.parse, "5-1x-1" )


## \todo: there should probably be a lot more tests in here...

if __name__ == "__main__":
unittest.main()
unittest.main()
Loading