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

Allow to construct AnyVector and AnyDictionary with values from Python #1574

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Allow to construct AnyVector and AnyDictionary with values from Python #1574

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b1f4bc8
Allow to construct AnyVector and AnyDictionary with values from Pytho…
JeanChristopheMorinPerso Apr 7, 2023
51d62b5
Fix formatting
JeanChristopheMorinPerso Apr 7, 2023
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
7 changes: 7 additions & 0 deletions src/py-opentimelineio/opentimelineio-bindings/otio_anyDictionary.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,13 @@ void otio_any_dictionary_bindings(py::module m) {

py::class_<AnyDictionaryProxy>(m, "AnyDictionary")
.def(py::init<>())
.def(py::init([](py::dict item) {
AnyDictionary d = py_to_any_dictionary(item);
auto proxy = new AnyDictionaryProxy;
proxy->fetch_any_dictionary().swap(*d.get_or_create_mutation_stamp()->any_dictionary);

return proxy;
}))
.def("__getitem__", &AnyDictionaryProxy::get_item, "key"_a)
.def("__internal_setitem__", &AnyDictionaryProxy::set_item, "key"_a, "item"_a)
.def("__delitem__", &AnyDictionaryProxy::del_item, "key"_a)
Expand Down
11 changes: 9 additions & 2 deletions src/py-opentimelineio/opentimelineio-bindings/otio_anyDictionary.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,10 +11,17 @@
namespace py = pybind11;

struct AnyDictionaryProxy : public AnyDictionary::MutationStamp {
using MutationStamp = AnyDictionary::MutationStamp;

AnyDictionaryProxy() {}

// TODO: Should we instead just pass an AnyDictionary?
AnyDictionaryProxy(MutationStamp *d) {
any_dictionary = d->any_dictionary;
}

~AnyDictionaryProxy() {
}

using MutationStamp = AnyDictionary::MutationStamp;

static void throw_dictionary_was_deleted() {
throw py::value_error("Underlying C++ AnyDictionary has been destroyed");
Expand Down
8 changes: 8 additions & 0 deletions src/py-opentimelineio/opentimelineio-bindings/otio_anyVector.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,14 @@ void otio_any_vector_bindings(py::module m) {

py::class_<AnyVectorProxy>(m, "AnyVector")
.def(py::init<>())
.def(py::init([](const py::iterable &it) {
any a;
py_to_any(it, &a);
AnyVector v = safely_cast_any_vector_any(a);
auto proxy = new AnyVectorProxy;
proxy->fetch_any_vector().swap(*v.get_or_create_mutation_stamp()->any_vector);
return proxy;
}))
Comment on lines +22 to +29
Copy link
Collaborator

Choose a reason for hiding this comment

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

is this copying the data?

Copy link
Member Author

Choose a reason for hiding this comment

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

If by data you mean everything (like a deep copy in Python), no. If you mean the top level list/vector, yes. See https://github.com/AcademySoftwareFoundation/OpenTimelineIO/pull/1574/files#r1165693980.

What should be the behavior?

.def("__internal_getitem__", &AnyVectorProxy::get_item, "index"_a)
.def("__internal_setitem__", &AnyVectorProxy::set_item, "index"_a, "item"_a)
.def("__internal_delitem__", &AnyVectorProxy::del_item, "index"_a)
Expand Down
7 changes: 7 additions & 0 deletions src/py-opentimelineio/opentimelineio-bindings/otio_anyVector.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,13 @@ namespace py = pybind11;
struct AnyVectorProxy : public AnyVector::MutationStamp {
using MutationStamp = AnyVector::MutationStamp;

AnyVectorProxy() {}

// TODO: Should we instead just pass an AnyVector?
AnyVectorProxy(MutationStamp *v) {
any_vector = v->any_vector;
}

static void throw_array_was_deleted() {
throw py::value_error("Underlying C++ AnyVector object has been destroyed");
}
Expand Down
2 changes: 1 addition & 1 deletion src/py-opentimelineio/opentimelineio-bindings/otio_utils.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,7 +96,7 @@ void _build_any_to_py_dispatch_table() {

static py::object _value_to_any = py::none();

static void py_to_any(py::object const& o, any* result) {
void py_to_any(py::object const& o, any* result) {
if (_value_to_any.is_none()) {
py::object core = py::module::import("opentimelineio.core");
_value_to_any = core.attr("_value_to_any");
Expand Down
1 change: 1 addition & 0 deletions src/py-opentimelineio/opentimelineio-bindings/otio_utils.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -152,6 +152,7 @@ struct PyAny {
pybind11::object any_to_py(any const& a, bool top_level = false);
pybind11::object plain_string(std::string const& s);
pybind11::object plain_int(int i);
void py_to_any(pybind11::object const& o, any* result);
AnyDictionary py_to_any_dictionary(pybind11::object const& o);

bool compare_typeids(std::type_info const& lhs, std::type_info const& rhs);
72 changes: 72 additions & 0 deletions tests/test_core_utils.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
import unittest

import opentimelineio._otio
import opentimelineio.opentime
import opentimelineio.core._core_utils


Expand Down Expand Up @@ -61,6 +62,42 @@ def test_raise_on_mutation_during_iter(self):
for key in d:
del d['b']

def test_construct_with_values(self):
d1 = opentimelineio.core._core_utils.AnyDictionary(
{'key1': 1234, 'key_2': {'asdasdasd': 5.6}}
)
v = opentimelineio.core._core_utils.AnyVector()
v.append(1)
v.append('inside any vector')

so = opentimelineio._otio.SerializableObject()
d2 = opentimelineio.core._core_utils.AnyDictionary(
{
'string': 'myvalue',
'int': -999999999999,
'list': [1, 2.5, 'asd'],
'dict': {'map1': [345]},
'AnyVector': v,
'AnyDictionary': d1,
Copy link
Collaborator

Choose a reason for hiding this comment

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

related to the question about copies, can you edit d1 in place and have d2["AnyDictionary"] change?

Copy link
Member Author

Choose a reason for hiding this comment

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

Items removed or added in d1 in place won't be added/removed in d2, but values are shared. As far as I see, this is an existing behavior. For example, this passes:

    def test_construct_with_values(self):
        # Contruct with values
        d1 = opentimelineio.core._core_utils.AnyDictionary(
            {'key1': 1234, 'key_2': {'asdasdasd': 5.6}}
        )

        # Construct without values and only add a value after instantiation.
        d3 = opentimelineio.core._core_utils.AnyDictionary()
        d3['key4'] = 'value4'

        # Create an SO and add it to d1 and d3.
        so = opentimelineio._otio.SerializableObjectWithMetadata('initial name')
        d1['so'] = so
        d3['so'] = so

        d2 = opentimelineio.core._core_utils.AnyDictionary(
            {
                'd1': d1,
                'd3': d3,
                'so': so
            }
        )
        self.assertEqual(d2['d1'], d1)
        self.assertEqual(d2['d3'], d3)

        # Edit d1 in place
        d1['key3'] = 'value3'
        d1['so'].name = 'new name set on d1["so"]'
        # Confirm that d2['d1'] is not updated
        self.assertEqual(d2['d1'], {'key1': 1234, 'key_2': {'asdasdasd': 5.6}, 'so': so})
        self.assertEqual(d1, {'key1': 1234, 'key_2': {'asdasdasd': 5.6}, 'key3': 'value3', 'so': so})
        # But values that were already there are shared.
        # We previously updated the name of so by doing d1['so'].name = '...'
        # and d2['so'] got updated.
        self.assertEqual(d2['so'].name, 'new name set on d1["so"]')

        # Same behavior here.
        d3['key5'] = 'value5'
        d3['so'].name = 'new name set on d3["so"]'
        self.assertEqual(d2['d3'], {'key4': 'value4', 'so': so})
        self.assertEqual(d3, {'key4': 'value4', 'key5': 'value5', 'so': so})
        self.assertEqual(d2['so'].name, 'new name set on d3["so"]')

Copy link
Member Author

Choose a reason for hiding this comment

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

But just to be sure, I'll do some A/B testing with the main branch.

Copy link
Member Author

Choose a reason for hiding this comment

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

Same result on the main branch (with slight modification in the code example to not construct with values):

    def test_construct_with_values(self):
        d1 = opentimelineio.core._core_utils.AnyDictionary()
        d1['key1'] = 1234
        d1['key_2'] = {'asdasdasd': 5.6}

        # Construct without values and only add a value after instantiation.
        d3 = opentimelineio.core._core_utils.AnyDictionary()
        d3['key4'] = 'value4'

        # Create an SO and add it to d1 and d3.
        so = opentimelineio._otio.SerializableObjectWithMetadata('initial name')
        d1['so'] = so
        d3['so'] = so

        d2 = opentimelineio.core._core_utils.AnyDictionary()
        d2['d1'] = d1
        d2['d3'] = d3
        d2['so'] = so
        self.assertEqual(d2['d1'], d1)
        self.assertEqual(d2['d3'], d3)

        # Edit d1 in place
        d1['key3'] = 'value3'
        d1['so'].name = 'new name set on d1["so"]'
        # Confirm that d2['d1'] is not updated
        self.assertEqual(d2['d1'], {'key1': 1234, 'key_2': {'asdasdasd': 5.6}, 'so': so})
        self.assertEqual(d1, {'key1': 1234, 'key_2': {'asdasdasd': 5.6}, 'key3': 'value3', 'so': so})
        # But values that were already there are shared.
        # We previously updated the name of so by doing d1['so'].name = '...'
        # and d2['so'] got updated.
        self.assertEqual(d2['so'].name, 'new name set on d1["so"]')

        # Same behavior here.
        d3['key5'] = 'value5'
        d3['so'].name = 'new name set on d3["so"]'
        self.assertEqual(d2['d3'], {'key4': 'value4', 'so': so})
        self.assertEqual(d3, {'key4': 'value4', 'key5': 'value5', 'so': so})
        self.assertEqual(d2['so'].name, 'new name set on d3["so"]')

'RationalTime': opentimelineio.opentime.RationalTime(
value=10.0,
rate=5.0
),
'TimeRange': opentimelineio.opentime.TimeRange(
opentimelineio.opentime.RationalTime(value=1.0),
opentimelineio.opentime.RationalTime(value=100.0)
),
'TimeTransform': opentimelineio.opentime.TimeTransform(
offset=opentimelineio.opentime.RationalTime(value=55.0),
scale=999
),
'SerializableObject': so
}
)
self.assertEqual(d2['string'], 'myvalue')
self.assertEqual(d2['SerializableObject'], so)
self.assertEqual(d2['AnyDictionary'], d1)

def test_raises_if_ref_destroyed(self):
d1 = opentimelineio.core._core_utils.AnyDictionary()
opentimelineio._otio._testing.test_AnyDictionary_destroy(d1)
Expand Down Expand Up @@ -196,6 +233,41 @@ def test_main(self):
# Appending copies data, completely removing references to it.
self.assertIsNot(v5[0], tmplist)

def test_construct_with_values(self):
d = opentimelineio.core._core_utils.AnyDictionary()
d['key_1'] = 1234
d['key_2'] = {'asdasdasd': 5.6}

v1 = opentimelineio.core._core_utils.AnyVector([1, 'inside any vector'])

so = opentimelineio._otio.SerializableObject()
v2 = opentimelineio.core._core_utils.AnyVector(
[
'myvalue',
-999999999999,
[1, 2.5, 'asd'],
{'map1': [345]},
v1,
d,
opentimelineio.opentime.RationalTime(
value=10.0,
rate=5.0
),
opentimelineio.opentime.TimeRange(
opentimelineio.opentime.RationalTime(value=1.0),
opentimelineio.opentime.RationalTime(value=100.0)
),
opentimelineio.opentime.TimeTransform(
offset=opentimelineio.opentime.RationalTime(value=55.0),
scale=999
),
so
]
)
self.assertEqual(v2[0], 'myvalue')
self.assertEqual(v2[-1], so)
self.assertEqual(v2[5], d)

def test_raises_if_ref_destroyed(self):
v1 = opentimelineio.core._core_utils.AnyVector()
opentimelineio._otio._testing.test_AnyVector_destroy(v1)
Expand Down