[WIP] Test Implementation of OpenTime in C++ #319
Conversation
- Ended up pushing it into the pybind11 wrapper instead of doing something down in C++. - This could be shifted around but seems fine for starting the conversation.
cpp_api/opentime.h
Outdated
| // number of decimal places, if you just std::cerr << total_seconds | ||
| /* DEBUG_PRINT(total_seconds); */ | ||
|
|
||
| auto try2 = time_obj.value_rescaled_to(1.0); |
meshula
left a comment
meshula
left a comment
There was a problem hiding this comment.
Nice to see!
In addition to nerfing out exceptions, and considering whether or not exact time representation is a requirement, I'd recommend an adaptor for std::chrono as well.
| && not ARRAY_CONTAINS(VALID_NON_DROPFRAME_TIMECODE_RATES, rate) | ||
| ) | ||
| { | ||
| throw std::invalid_argument( |
There was a problem hiding this comment.
throw would be most unpleasant in a validate function. Maybe return a std::error_code if you wish more granularity than true and false.
| // @TODO: Is this the right policy for C++ | ||
| if (time_obj.value < 0) | ||
| { | ||
| throw std::invalid_argument("time_obj has a negative value"); |
There was a problem hiding this comment.
"negative value" would be one of those error_codes. I'd prefer not to have to right exception handling code around something as foundational as to_timecode. I'd suggest returning std::pair<std::error_code, std::string> and use error_code::success as the check value.
| } | ||
| else | ||
| { | ||
| if (rate == static_cast<rt_rate_t>(29.97)) |
There was a problem hiding this comment.
probably going to need something more subtle here, as there are PAL rates to consider as well. Probably don't want to get into exhaustively enumerating all of them.
There was a problem hiding this comment.
Cool, good call. At the moment, this is what the python implementation is doing, so if its wrong here, its wrong there too. Do you have any suggestions about what that more subtle thing might be?
| { | ||
| if (ARRAY_CONTAINS(time_str, ';')) | ||
| { | ||
| throw std::invalid_argument("Drop frame timecode not supported."); |
There was a problem hiding this comment.
ditto on a std::pair<std::error_code, ...> return, exception allergy in play :)
| { | ||
| return ( | ||
| (std::hash<opentime::rt_value_t>{}(rt.value)) | ||
| ^ ((std::hash<opentime::rt_rate_t>{}(rt.rate)) >> 1) |
There was a problem hiding this comment.
a robust hash_combine is considered
hash = hash1 ^ (hash2 + 0x9e3779b9 + (hash1<<6) + (hash1>>2);
There was a problem hiding this comment.
Awesome, I'll snag that. There is another question: do you think these types should be hashable? I'm open to making them not hashable.
| bool validate_timecode_rate(const rt_rate_t rate); | ||
|
|
||
| /// A point in time, value*rate samples after 0. | ||
| class RationalTime |
There was a problem hiding this comment.
RationalTime classes usually carry an expectation of allowing exact representations, which would require at least templating rt_value_t and rt_rate_t.
There was a problem hiding this comment.
Yeah, we've talked about that but haven't built that out yet. I'm open to that as well. Thanks!
|
@ssteinbach I'm very interested into seing this PR merged. How can I help? |
|
See #391 |
4 participants
Hi all, this pull request adds a new subdirectory,
cpp_apiin which a test implementation of opentime in C++ resides, along with a pybind11 python binding. I've also copied in thetest_opentime.pysuite so you can see what changes had to be made in order to make things pass. This is intended more to draw out conversation and feedback, and to serve as a reference point for conversations with folks we're having about a C++ API. Let us know if you have any thoughts!Before this can be merged in:
opentimelineiomodule can find itThanks everyone!
Quick Reminder If you're curious about some of the previous discussion around this issue, see:
https://github.com/PixarAnimationStudios/OpenTimelineIO/wiki/Wrapping-OTIO-C-or-C-Plus-Plus-In-Python