delete the warning in the compilation by casting in (int) warning: …

[zebulon75018]

delete the warning in the compilation by casting in (int) warning: …
…comparison between signed and unsigned integer expressions

just for delete a compilation message.

please just tell me if it;s not interesting.
My two cents....
Cheer

[reinecke]

Oh nice, I shouldn't have let a compile warning get through.
I wonder if we should actually just switch frame_zero_padding to be an unsigned int, I don't think it has any valid semantic meaning if set negative.

@apetrynet What do you think?

Either way, this fix makes things better so we'd still be improving if we decided to merge as-is.

[meshula]

@zebulon75018 Thanks for the catch!

@reinecke All of the APIs in this cohort have an int as the frame number. If frame numbers must be >= 0, it makes sense to "repair" the API, and consequently the variables in the methods. If frame numbers can be negative, then a static_cast would be idiomatic in this diff, rather than a C style cast. I'm guessing, >= 0 is the answer, so what do you think about taking a quick pass over the API?

[apetrynet]

@reinecke I think frame_zero_padding quite safely could be set to unsigned int. A negative frame padding would imply not having a frame number at all which makes little sense in an image sequence :)

@meshula Negative frame numbers are used in some cases where the preroll of a simulation needs to go below frame zero for instance. So, even if not that common, frame numbers should support negative numbers.

[meshula]

If negative frame numbers are allowable, then the zero padding algorithm should do this ~

bool isNegative = frame < 0
frame = abs(frame)
zeroPad(amount - isNegative?1:0)
if (isNegative)
   prepend('-')

~ shouldn't it?

[apetrynet]

@meshula I'm sorry, but don't think I understand what your suggesting here.

[meshula]

The existing code, if zero padded to 4, will do this: 1 becomes 0001. -1 becomes 00-1. I'm proposing that 1 should become -001.

[apetrynet]

Hmm. I'm unable to replicate that. I might of course be doing something strange here. @reinecke do you have any input?

[meshula]

I'll try to repro after ASWF OpenSource Days wraps later today

[meshula]

I was quite mistaken, sorry! When I was reviewing the code, every time I looked, I didn't spot that the image_num_string conversion has an abs in it. The logic does make sense for negative numbers :)

[apetrynet]

Great! Thanks for double-checking!

…

On Fri, Aug 21, 2020, 00:35 Nick Porcino ***@***.***> wrote: I was quite mistaken, sorry! When I was reviewing the code, every time I looked, I didn't spot that the image_num_string conversion has an abs in it. The logic does make sense for negative numbers :) — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#774 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAIW7YJ7HP6JUO54AXPCHELSBWQMLANCNFSM4QEH3LMQ> .

[reinecke]

Yep, negative frame numbers are allowed and (I think) have good test coverage for the examples discussed: https://github.com/PixarAnimationStudios/OpenTimelineIO/blob/887575ae1f9febe605bb9bd7889aa6e700112193/tests/test_image_sequence_reference.py#L552-L613

@meshula would you agree switching frame_zero_padding to be an unsigned int would be a good idea?

This would probably imply fixing up static_cast in other places. Also some additional error handling on deserialization (since al JSON ints are signed) here: https://github.com/PixarAnimationStudios/OpenTimelineIO/blob/887575ae1f9febe605bb9bd7889aa6e700112193/src/opentimelineio/imageSequenceReference.cpp#L129

Just below that point in the code is a good example of how to bubble up deserialization errors: https://github.com/PixarAnimationStudios/OpenTimelineIO/blob/887575ae1f9febe605bb9bd7889aa6e700112193/src/opentimelineio/imageSequenceReference.cpp#L148-L151

I think this would be, overall, a good update to the API and would prevent some potentially odd behavior.

@zebulon75018 Sorry to blow out the scope on what is supposed to be a simple compile warning fix, please let us know if doing all that is more than you'd want to take on.

[meshula]

@reinecke yes to the uint question

[zebulon75018]

Waou , I just want to give to the project my 2 cents ... :) , that's produced an interesting discution :).
I never imagine, but I hope that help you a little bit.

[codecov-commenter]

Codecov Report

Merging #774 into master will increase coverage by 0.76%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master     #774      +/-   ##
==========================================
+ Coverage   83.21%   83.98%   +0.76%     
==========================================
  Files          74       74              
  Lines        2884     3122     +238     
==========================================
+ Hits         2400     2622     +222     
- Misses        484      500      +16     

Flag	Coverage Δ
#py27	83.25% <ø> (+0.12%)	⬆️
#py36	83.32% <ø> (+0.12%)	⬆️
#py37	83.32% <ø> (+0.12%)	⬆️
#unittests	84.13% <ø> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
src/opentimelineio/imageSequenceReference.cpp	92.10% <ø> (+0.67%)	⬆️
...pentimelineio-bindings/otio_errorStatusHandler.cpp	62.26% <0.00%> (-6.97%)	⬇️
...lineio/opentime-bindings/opentime_rationalTime.cpp	97.43% <0.00%> (-1.08%)	⬇️
src/opentimelineio/serializableObject.cpp	57.97% <0.00%> (-0.86%)	⬇️
src/opentime/timeRange.h	100.00% <0.00%> (ø)
src/opentime/errorStatus.h	100.00% <0.00%> (ø)
src/opentimelineio/gap.cpp	100.00% <0.00%> (ø)
src/opentimelineio/effect.cpp	100.00% <0.00%> (ø)
src/opentimelineio/marker.cpp	100.00% <0.00%> (ø)
src/opentimelineio/timeline.cpp	100.00% <0.00%> (ø)
... and 35 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 992b3c2...90fc31c. Read the comment docs.

[reinecke]

@zebulon75018 Thank you for pointing this one out, you managed to find a little indicator of a bigger oversight I made.