There is a pattern in the Python bindings (both opentime and opentimelineio) where parameters of type string, list and
dict and bool are typed as py::object
.
The values are converted in the constructors using string_or_none_converter
, py_to_vector
and py_to_any_dictionary
utility functions.
One such example is the Track
class:
py::class_<Track, Composition, managing_ptr<Track>>(m, "Track", py::dynamic_attr())
.def(py::init([](py::object name,
py::object children,
optional<TimeRange> const& source_range,
std::string const& kind,
py::object metadata) {
auto composable_children = py_to_vector<Composable*>(children);
Track* t = new Track(
string_or_none_converter(name),
source_range,
kind,
py_to_any_dictionary(metadata)
);
if (!composable_children.empty())
t->set_children(composable_children, ErrorStatusHandler());
return t;
}),
py::arg_v("name"_a = std::string()),
"children"_a = py::none(),
"source_range"_a = nullopt,
"kind"_a = std::string(Track::Kind::video),
py::arg_v("metadata"_a = py::none()))
The main problem with this practice is that the signature becomes:
| __init__(...)
| __init__(self: opentimelineio._otio.Track, name: object = '', children: object = None, source_range: Optional[opentimelineio._opentime.TimeRange] = None, kind: str = 'Video', metadata: object = None) -> None
It is impossible to know what type of objects we can pass to the name
, children
and metadata
parameters
from Python without actually trying it. For example:
>>> opentimelineio.schema.Track(name={}, children=1)
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
File "/home/jcmorin/jcmenv/aswf/OpenTimelineIO/build/lib.linux-x86_64-3.10/opentimelineio/core/_core_utils.py", line 152, in _value_to_so_vector
raise TypeError(
TypeError: Expected list/sequence of SerializableObjects; found type '<class 'int'>'
In this example there is two things to note. First, the error doesn't tell us which argument is of wrong type.
Second, it doesn't tell us which specific types should be in the list/sequence. From the code,
the list/sequence passed to the children
parameter is not all subclasses of SerializableObject
. It only accepts
a list/sequence of type Composable
and its sublcasses.
It is also good to note in this example the Python > C++ > Python > C++ roundtrip to get the type error.
Another example, but this time in opentime:
.def("to_timecode", [](RationalTime rt, double rate, py::object drop_frame) {
return rt.to_timecode(
rate,
df_enum_converter(drop_frame),
ErrorStatusConverter()
);
}, "rate"_a, "drop_frame"_a)
where df_enum_converter
is:
IsDropFrameRate df_enum_converter(py::object& df) {
if (df.is(py::none())) {
return IsDropFrameRate::InferFromRate;
} else if (py::cast<bool>(py::bool_(df))) {
return IsDropFrameRate::ForceYes;
} else {
return IsDropFrameRate::ForceNo;
}
}
In this case, it would be better to use optional<bool>
and modifying df_enum_converter
to take an optional bool would do the trick.
Some classes accept a name
parameter of type py::string
while others accept py::object
.
Reference commit: https://github.com/PixarAnimationStudios/OpenTimelineIO/pull/540/commits/49f0b133847ed08f0958ee2d6cec7e1dd04dfcbf
Some enum-like classes are pure classes while others are real enums.
For example, opentimelineio.schema.Marker.Color
and opentimelineio.schema.Track.Kind
are standard classes while
opentimelineio.schema.Track.NeighborGapPolicy
and opentimelineio.schema.ImageReference.MissingFramePolicy
are real enums.
AcademySoftwareFoundation/OpenTimelineIO#407
Classes in opentimelineio._otio
have their __str__
and __repr__
methods implemented in Python while their
class is implmented in C++.
This creates some problems:
- Someone looking at modifying these method will have to search where they are defined.
- Import time cost.
- Method implementation is sperarated from the class implementation.
I think these methods could be defined in C++ without too much troubles. This is already done in opentime
.
opentimelineio.opentime
exports 4 functions that are unnecessarily implemented in Python.
to_timecode
to_frames
to_seconds
to_time_string
I understand from these that they are there for backward compatibility reasons since they "wrap" methods of the opentime.RationalTime` class.
Edit: Thanks for taking time to read this and comment!