Proposed C++ API Documentation #391
Conversation
Codecov Report
@@ Coverage Diff @@
## master #391 +/- ##
==========================================
- Coverage 87.15% 86.92% -0.24%
==========================================
Files 67 63 -4
Lines 6734 5604 -1130
==========================================
- Hits 5869 4871 -998
+ Misses 865 733 -132
Continue to review full report at Codecov.
|
docs/cxx-proposal/bridges.md
Outdated
| Since OTIO originated as Python (and has an extensive test suite, in Python), our starting position | ||
| is that existing Python code (adapters, plugins, schemadefs) must continue to work, as currently | ||
| written. Python code in the `core` or `schema` directories will of course be rewritten, but Python code | ||
| outside those modules should not be aware of any change. |
There was a problem hiding this comment.
We have already made a few pre-emptive changes to the Python API to ease the transition to C++, so this statement is stronger than we really plan. I don't want to soften this too much, but we should be honest by noting that some changes are happening, but only when necessary or desirable (e.g. the immutability of TimeRange, etc.) and that we are doing this ahead of time to give people time to adjust.
| clip2->metadata()["other_clip"] = clip1; | ||
|
|
||
| will work just fine: writing/reading or simply cloning ``clip1`` would yield | ||
| a new ``clip1`` that pointed to a new ``clip2`` and vice versa. |
There was a problem hiding this comment.
We should have a larger discussion about either enforcing the tree structure at the schema level, or explicitly allowing non-tree structures.
We suspect that some future schemas/features will want to use this (e.g. Effects that use multiple clips as sources, or linked audio & video clips on two different tracks) but so far we have been very happy with the simplicity of the tree structure as an aid for making OTIO useful and easy to learn.
In any case, if/when we choose to allow for non-tree structures, we should be explicit and intentional about it.
|
A Clip's media_reference field can also be null. If this is a problem, we could perhaps enforce the use of MissingReference instead of null.
When you set the clip to None, the existing implementation makes it be missing reference.
At any rate, having a nullptr for a SerializableObject* in the C++ implementation writes out a null value to json.
So for SerializableObject*, there is no need of optional<> because the nullptr case implicitly handles this.
Should I flesh that out in the doc?
… |
|
+The need for an "optional" (i.e. a container class that holds either no value
+or some specific value, for a given type T) is currently small, but does occur
+in one key place (schemas which need to hold either a ``TimeRange`` or
+indicate their time range is undefined).
or perhaps I misunderstood. Is "optional" only needed for non-SerializableObject types?
Right, it’s only needed for non-Serializable types.
|
|
I’ve fixed the typos you flagged, thanks for the careful reading. (And I wrestled readthedocs into seeing my just-the-docs branch, after about an hour of pleading with it.)
We should have a larger discussion about either enforcing the tree structure at the schema level, or explicitly allowing non-tree structures.
We suspect that some future schemas/features will want to use this (e.g. Effects that use multiple clips as sources, or linked audio & video clips on two different tracks) but so far we have been very happy with the simplicity of the tree structure as an aid for making OTIO useful and easy to learn.
I just discovered that the constructor for Clip does a deep copy of its media_reference, precisely to avoid this problem, I’m guessing. The setter on the property however doesn’t, which is inconsistent and thus a bug. In fact, I think the fact that it’s deep copying the media reference, so that trying to modify the media reference i just set on it won’t do anything is the surprise here! It certainly isn’t documented.
So, yes, we should discuss what the policy should be. You can avoid non-trees by cloning when you pass things in, in cases like this where it’s not determinable if a media reference has been reused or not. the case of composition is easier, because there you do enforce things being a tree.
In any case, if/when we choose to allow for non-tree structures, we should be explicit and intentional about it.
Sure.
What happens in this case?
Clip* c2 = new Clip;
t->add_child(c2);
t->remove_child(0);
Removing the 0th child nuked c2 (remember, you “gave it away” when you installed it as a child).
So the next line
std::cout << c2->name();
is likely a crash, as is the next.
I thought about letting the API specify something like
t->remove_child(0, WITHOUT_DELETING);
which would stop the automatic deletion, but decided this is not going to come up much, and if it does, the example of retaining before deleting it is the solution. I am happy to revisit however; i wanted to make the most common thing be the least typing.
… c2->possibly_delete();
t->possibly_delete();
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub <#391 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AMFkpTe8rgw-TN6oh2wYW2dt7H34VxDxks5uzzzTgaJpZM4Y4nLj>.
|
|
Thanks for the changes. This looks great. I will send this out to a few people for comments and then we can open it up for wider discussion. |
| The need for an "optional" (i.e. a container class that holds either no value | ||
| or some specific value, for a given type T) is currently small, but does occur | ||
| in one key place (schemas which need to hold either a ``TimeRange`` or | ||
| indicate their time range is undefined). |
There was a problem hiding this comment.
A "no value" sentinel value in TimeRange is arguably preferable to optional. The argument is that an algebra can be defined on TimeRange. If optional is involved, then there is necessarily a layer of checking and conditional structure that must be invoked before deciding to access a TimeRange; whereas a sentinel value would allow the "no value" condition to participate in an algebra, via identity, associativity, commutativity, etc.
| the count of the instance is zero, and if so deletes the object in question. | ||
| - An ``any`` instance holding a ``SerializableObject*`` actually holds a | ||
| ``Retainer<SerializableObject>``. That is, blind data safely retains schema instances. | ||
|
|
There was a problem hiding this comment.
What's the mechanism for safety under concurrency? On the face of it, if I return a pointer, and Retainer the return value, isn't there a window where the pointer could be deleted before it is Retainer'd? Or is the idea that objects subject to this regime are owned at a higher scope - the schema from which the object was brokered holds the ultimate reference that in the usual case prevents this? Secondarily, I imagine the reference count is fenced and/or atomic?
|
I don't see anything in this proposal about exceptions. I have comments in another issue about not being in favor of exception heavy designs, so I won't belabor the point here, but would hope to see a strategy for error handling laid out in the proposal. Overall, the proposal looks nice to me. |
|
Begin forwarded message:
From: Nick Porcino ***@***.***>
Subject: Re: [PixarAnimationStudios/OpenTimelineIO] Proposed C++ API Documentation (#391)
Date: November 29, 2018 at 12:07:24 PM PST
To: PixarAnimationStudios/OpenTimelineIO ***@***.***>
Cc: davidbaraff ***@***.***>, Author ***@***.***>
Reply-To: PixarAnimationStudios/OpenTimelineIO ***@***.***>
@meshula commented on this pull request.
In docs/cxx-proposal/cxx.rst <#391 (comment)>:
> +* `OTIO C++ simplified header files <https://github.com/davidbaraff/OpenTimelineIO/tree/master/proposed-c%2B%2B-api/otio>`_.
+
+Dependencies
+++++++++++++
+
+The C++ OpentimelineIO (OTIO) library implementation will have the following dependencies:
+
+ * `rapidjson <https://github.com/Tencent/rapidjson>`_
+ * any (C++ class)
+ * optional (C++ class)
+ * The C++ Opentime library (see below)
+
+The need for an "optional" (i.e. a container class that holds either no value
+or some specific value, for a given type T) is currently small, but does occur
+in one key place (schemas which need to hold either a ``TimeRange`` or
+indicate their time range is undefined).
A "no value" sentinel value in TimeRange is arguably preferable to optional.
I will note however that currently schemas can hold None for their time range in certain cases; if that wants to change to instead always be a well-defined sentinel value it can, but it doesn’t affect the C++ proposal (except, perhaps, to maybe eliminate the need for optional). Taking things back *out* of the proposal is easy. :)
… The argument is that an algebra can be defined on TimeRange. If optional is involved, then there is necessarily a layer of checking and conditional structure that must be invoked before deciding to access a TimeRange; whereas a sentinel value would allow the "no value" condition to participate in an algebra, via identity, associativity, commutativity, etc.
In docs/cxx-proposal/cxx.rst <#391 (comment)>:
> +conversion between ``Retainer<MediaReference>`` and ``MediaReference*`` which keeps the code
+simple. Even testing if the property is set (as ``best_or_other()`` does) is done as
+if we were using raw pointers.
+
+The implementation of all this works as follow:
+
+- Creating a new schema instance starts the instance with an internal count of 0.
+- Putting a schema instance into a ``Retainer`` object increases the count by 1.
+- Destroying the retainer object or reassigning a new instance to it decreases the
+ count by 1 of the object if any in the retainer object. If this causes the count
+ to reach zero, the schema instance is destroyed.
+- The ``possibly_delete()`` member function of ``SerializableObject*`` checks that
+ the count of the instance is zero, and if so deletes the object in question.
+- An ``any`` instance holding a ``SerializableObject*`` actually holds a
+ ``Retainer<SerializableObject>``. That is, blind data safely retains schema instances.
+
What's the mechanism for safety under concurrency? On the face of it, if I return a pointer, and Retainer the return value, isn't there a window where the pointer could be deleted before it is Retainer'd? Or is the idea that objects subject to this regime are owned at a higher scope - the schema from which the object was brokered holds the ultimate reference that in the usual case prevents this? Secondarily, I imagine the reference count is fenced and/or atomic?
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub <#391 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AMFkpSQafVoQjvAkn7v4euzidr_cpqcbks5u0D58gaJpZM4Y4nLj>.
|
|
Thank you. I totally forgot to mention this.
We’re going for not throwing exceptions in C++. Instead, the pattern is
bool func_that_might_fail(<arguments>, std::string* err_msg);
A function with this signature signals that it might fail; if it does so, and err_msg is non-null, it will return false and set *err_msg accordingly.
In C++, it is up to the caller to check.
In Python, the bridging code checks and turns this kind of failure into an exception. Ditto for Swift (someday).
I will add this.
Begin forwarded message:
… From: Nick Porcino ***@***.***>
Subject: Re: [PixarAnimationStudios/OpenTimelineIO] Proposed C++ API Documentation (#391)
Date: November 29, 2018 at 12:09:36 PM PST
To: PixarAnimationStudios/OpenTimelineIO ***@***.***>
Cc: davidbaraff ***@***.***>, Author ***@***.***>
Reply-To: PixarAnimationStudios/OpenTimelineIO ***@***.***>
I don't see anything in this proposal about exceptions. I have comments in another issue about not being in favor of exception heavy designs, so I won't belabor the point here, but would hope to see a strategy for error handling laid out in the proposal.
Overall, the proposal looks nice to me.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub <#391 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AMFkpQRTBvdA_oF-K8xIiLYCGdGnvMbzks5u0D8AgaJpZM4Y4nLj>.
|
|
What's the mechanism for safety under concurrency? On the face of it, if I return a pointer, and Retainer the return value, isn't there a window where the pointer could be deleted before it is Retainer'd? Or is the idea that objects subject to this regime are owned at a higher scope - the schema from which the object was brokered holds the ultimate reference that in the usual case prevents this?
Certainly only a single thread could be mutating the graph returned from deserializing something at any given time, and it would be on the code doing the mutating to ensure that, and also to ensure nobody else is trying to read it either.
If we consider just looking at the graph, and believe that ought to be doable from multiple threads at the same time, then since reading has the potential of creating new temporary Retainer objects, depending on how you structure your code, I’d think we’d opt for making the counter be an atomic integer.
I *think* that would be sufficient, because reading could modify the count, but not cause any other structural change.
Do you agree with this?
The TypeRegistry I was careful to make thread-safe.
|
|
What's the mechanism for safety under concurrency? On the face of it, if I return a pointer, and Retainer the return value, isn't there a window where the pointer could be deleted before it is Retainer'd? Or is the idea that objects subject to this regime are owned at a higher scope - the schema from which the object was brokered holds the ultimate reference that in the usual case prevents this? Secondarily, I imagine the reference count is fenced and/or atomic?
This may be harder than I thought, even in just the “reading” case. How important is it that multiple threads be able to traverse, without modifying, the same graph at the same time? Because creating a new “Retainer” object, even temporarily, is actually pretty much like a mutation, even if it is only done locally, in a function.
I’d love to know if this is something we think is truly important or not. Multiple threads working on completely separate instances of objects is no problem of course.
… —
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub <#391 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AMFkpSQafVoQjvAkn7v4euzidr_cpqcbks5u0D58gaJpZM4Y4nLj>.
|
|
> What's the mechanism for safety under concurrency? On the face of it, if I return a pointer, and Retainer the return value, isn't there a window where the pointer could be deleted before it is Retainer'd? Or is the idea that objects subject to this regime are owned at a higher scope - the schema from which the object was brokered holds the ultimate reference that in the usual case prevents this? Secondarily, I imagine the reference count is fenced and/or atomic?
>
This may be harder than I thought, even in just the “reading” case. How important is it that multiple threads be able to traverse, without modifying, the same graph at the same time? Because creating a new “Retainer” object, even temporarily, is actually pretty much like a mutation, even if it is only done locally, in a function.
I’d love to know if this is something we think is truly important or not. Multiple threads working on completely separate instances of objects is no problem of course.]
Never mind, I figured it out. Shouldn’t be a problem for the multiple readers case, even with Python (or Swift) thrown in the mix. (This was never an issue with pure C++. Just combo cases.)
|
|
Thanks for the thoughtful responses. Edit: conversation about sentinel values moved to https://github.com/PixarAnimationStudio/OpenTimelineIO/issues/394
This is easy to bind to other languages. I'm wondering if there is a case where it's actually a good idea to pass nullptr for err_msg? One could simply force that the user must supply a string that might be populated with an error message. This would likely result in end user code that actually reports problems. I'd probably encourage a result enumeration instead of a bool return. The scenario I am imagining is that without an enumerated return value, applications might resort to parsing the string in order to handle certain types of problems. For example, an application might desire a different resolution strategy for each of these cases: (a) invalid range specified, (b) referenced file not found at indicated URI. With an enumerated result return, I wouldn't care if the err_msg is a pointer or a reference.
You've probably thought of this already, but the canonical use case is MVC pretty much needs the UI on one thread and processing on another. OpenTimelineIO is no doubt going to be traversed simultaneously for rendering a timeline, and performing some operation, like splitting a ranged item. One might imagine that OTIO operations will always be trivially fast and therefore don't need to be scaled, which is an invitation for someone somewhere to put 100,000 items intersecting at one time point :) |
|
I’m not entering into a discussion about sentinel values in the open time types. That is for someone else to decide, and is orthogonal to a c++ port. I.e. you can have this discussion about the current python API just as well.
…Sent from my iPad
On Nov 30, 2018, at 10:34 AM, Nick Porcino ***@***.***> wrote:
Thanks for the thoughtful responses.
currently schemas can hold None for their time range
I'd like if there were constexpr in TimeRange for None, Never and Always
With these rules embodied in union and intersection:
t U None = t
t U Never = t
t U Always = Always
t ^ None = t
t ^ Never = Never
t ^ Always = t
associativity, commutativity, and distribution appear to work correctly.
it will return false and set *err_msg accordingly
This is easy to bind to other languages. I'm wondering if there is a case where it's actually a good idea to pass nullptr for err_msg? One could simply force that the user must supply a string that might be populated with an error message. This would likely result in end user code that actually reports problems. I'd probably encourage a result enumeration instead of a bool return.
The scenario I am imagining is that without an enumerated return value, applications might resort to parsing the string in order to handle certain types of problems. For example, an application might desire a different resolution strategy for each of these cases: (a) invalid range specified, (b) referenced file not found at indicated URI.
With an enumerated result return, I wouldn't care if the err_msg is a pointer or a reference.
concurrency ... Never mind, I figured it out.
You've probably thought of this already, but the canonical use case is MVC pretty much needs the UI on one thread and processing on another. OpenTimelineIO is no doubt going to be traversed simultaneously for rendering a timeline, and performing some operation, like splitting a ranged item. One might imagine that OTIO operations will always be trivially fast and therefore don't need to be scaled, which is an invitation for someone somewhere to put 100,000 items intersecting at one time point :)
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub, or mute the thread.
|
|
Agreed. Moved the sentinel value comment to a new issue to keep the conversation focused. |
Change from using plain string for error messages to a more structured form.
|
From the SG Autodesk team, here are our comments. Note that several of those come from comparing to our own current C++ implementation, therefore might be more relevant to our usage of OTIO than others. Overall, very clean and modern code. We like it!
• Clip -> Item -> Composable -> SerializableObjectWithMetadata -> SerializableObject
public: I prefer this: public: private:
TX, The SG Autodesk team |
|
A function which can “fail” will indicate this by taking an argument std::string* err_msg which it will set with a readable error message string if the pointer is non-null.
• I would prefer passing an int instead since this makes it much easier if we are checking for specific known errors.
• There could be a separate namespace/class for translating the codes to a human-readable string -- it would also make localization much easier on the client side.
I updated the docs on this just a couple minutes before you sent this! (The header files aren’t yet changed, but please see if what we propose in the doc make sense. ) In fact:
struct ErrorStatus {
operator bool () {
return outcome != Outcome::OK;
}
enum Outcome {
OK,
NOT_IMPLEMENTED,
...
};
// (some constructors, an operator=)
Outcome outcome;
std::string details;
static std::string outcome_to_string(Outcome);
};
|
|
The ErrorStatus object addresses what I was looking for, thanks! |
|
Because we’d like to keep opentime as a separate library from opentimelineio, we will actually replicate the ErrorStatus structure in the two libraries; they will have very different enum code lists. In fact, the opentime one is just:
struct ErrorStatus {
operator bool () {
return outcome != Outcome::OK;
}
enum Outcome {
OK,
INVALID_TIMECODE_RATE,
NON_DROPFRAME_RATE,
INVALID_TIMECODE_STRING,
INVALID_TIME_STRING,
TIMECODE_RATE_MISMATCH,
NEGATIVE_VALUE,
};
...
};
while I think the opentimelineio will have a lot more enum codes. So there will be both
opentime::ErrorStatus
and
opentimelineio::ErrorStatus
though they will have the same API apart from the enum codes. We haven't thought of a good way to combine them given that we want two separate libraries.
Begin forwarded message:
… From: Nick Porcino ***@***.***>
Subject: Re: [PixarAnimationStudios/OpenTimelineIO] Proposed C++ API Documentation (#391)
Date: December 5, 2018 at 1:39:20 PM PST
To: PixarAnimationStudios/OpenTimelineIO ***@***.***>
Cc: davidbaraff ***@***.***>, Author ***@***.***>
Reply-To: PixarAnimationStudios/OpenTimelineIO ***@***.***>
The ErrorStatus object addresses what I was looking for, thanks!
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub <#391 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AMFkpRYPZpdBbx3AI-Ozzz6eHrz6XI_aks5u2D0IgaJpZM4Y4nLj>.
|
|
@eidmi, we have some questions about your questions
I'll let @davidbaraff comment on the other items. |
|
• Can I used a shared_ptr with the deleter function set to call possibly_delete?
I believe so, if the fact that *other* parts of the code (e.g. a Python bridge) are not using shared_ptr (but just dealing with raw pointers and Retainers) doesn’t present an issue. So yes, if that doesn’t mess up the reference counting aspects, then you’d be free to use shared_ptr.
This is just a style preference, but instead of a constructor with a bunch of default values, especially for invalid values like this:
Sure.
Where is the Timeline object? Global start time?
Sorry, we accidentially skipped Timeline. we’ve implemented it though:
class Timeline : public SerializableObjectWithMetadata {
public:
struct Schema {
static auto constexpr name = "Timeline";
static int constexpr version = 1;
};
using Parent = SerializableObjectWithMetadata;
Timeline(std::string const& name = std::string(),
RationalTime global_start_time = RationalTime(0, 24),
AnyDictionary const& metadata = AnyDictionary());
Stack const* tracks() const;
Stack* tracks();
RationalTime global_start_time() const;
void set_global_start_time(RationalTime global_start_time);
...
};
8. we’re exposing the fact that we are based on rapidjson. On your side, you say your implementation depends on it, but I did not see any rapidjson object in the API.
There isn’t any in the API. It’s a completely internal detail. We were just saying for now we’re depending on it, in order to build the library.
• It is convenient to use rapidjson to compare a whole document using rapidjson::Document::operator==.
Not sure if this matters, but we never construct the Document, using rapidjson: we use only streaming reading/writing, to ensure we keep the memory down.
• You seem to rely on the fact that your underlying data container is an std::map and always sorts its entries based on the key. Not very convenient to compare OTIO strings that could come from another parser.
Not understanding. Certainly, we want to sort things (typically) to the JSON file to make it easy to do comparisons. In Python, we asked json to sort things. here, we’re depending on std::map to give us a sort order, which is nice.
If things come from another parser and have a very different order, how is the fact that we used std::map to order things make it any more or less convient to compare them?
Said differently, we want the ability to fix the output order determinstically. why is the fact that using std::map does this a problem?
• "SerializableObject::is_equivalent_to()" mentions in its doc that it uses "==" for everything except schema comparison, but I don't see any operator== in their API.
When you get down to the level of comparing two C++ any instances that each hold something, if both hold a SerializableObejct, you pull the objects out and use is_equivalent_to() to compare them. Otherwise, if the any() holds a primimtive type, you pull the primitive types out, and do == on the primitive types (assuming the types match). For AnyVector/AnyDictionary you recurse through the elements. So the == is coming from comparisons between bool, int, double, RationalTime.
9. If base classes like SerializableObject are not meant to be persisted without derivation, shouldn't they be pure virtual and NOT have a schema name/version for those classes?
It turns out to be very convenient to allow them to exist without derivation, when considering the bridge to other languages. In Python, when I want to derive my own concrete class off of, say, Item, we end up actually creating a C++ instance of Item, but it’s schema name is set to a new name that Python defined. In this way, the bindings that describe the member of an Item to Python do double-duty: they both set the stage for the concrete instances in C++ that derive from Item, and also pave the way for someone in Python to derive off of Item themselves, and make a concrete class. It’s also nice for regression testing.
|
|
Thanks for the responses! These questions were an amalgamation of questions coming from Eric and myself. I'll respond to the questions I asked here best I can. I'll let Eric respond to the other ones.
The issue is that exposing STL in headers requires that anyone linking with the library use the same compiler and compiler version, since STL is implementation specific (different ABI) and you can get weird random behaviour if this is not the case. We also have libraries that could benefit from using otio, but thus far we've avoid imposing any restrictions on compilers. There are essentially two ways of doing this. One way is to put the code that uses STL in the headers (so the client compiles it with their compiler) -- rapidjson is an example of this. The other is to wrap any STL with a private-implementation idiom (ex. return your own Vector to clients that only has a void* in the header and exposes vector methods. To implement the class, reinterpret_cast back to an std::vector in your implementation and call the corresponding method). This way, the STL you need is eseentially part of your library instead of the STL implementation. For what it's worth our particular libraries use a combination of both of those techniques. Neither option is particularly appealing I realize, which is why things like the VxF platform exist. There's a good paper here on the challenges of writing libraries (ELF libraries, so posix systems):
Yes, thanks!
Yes, I generally prefer to write libraries where clients provide their own "polymorphic" behaviour by specializing a template for their own structure/class. Not only because its faster (compile time over runtime), but also because it allows the library to keep all inheritance as a private implementation detail. This allows you the flexibility of changing interfaces without having to worry about breaking client code. And it's also nice not to have to impose that the client inherits from the library's class structure, particularly when the client may want to use a given class with multiple libraries that do this. By removing inheritance from the equation, it also allows for value semantics instead of reference semantics, which can be quite convenient when they're used for multithreading. But again, this is a preference that comes from writing code that has to fit into large complex hierarchies and not a hard requirement. There's a talk/code example on this approach here, if you are interested in this subject:
Combining error codes is always a bit painful. One simple solution worth considering is starting the otio error codes at an offset. Something like enum class Outcome { At least you don't end up with overlapping error codes that way (besides OK set to zero, which is a good thing). Also note that I used an enum class instead on enum. Since you're writing new code from scratch, I would encourage it's use over an enum.
Great. I was just asking because I was wondering if there was something "special" that the Retainer was doing. If this is the case, you might be able to simplify by the Retainer class by implementing it with a shared_ptr with a custom deleter function. You could potentially also add a ScopeGuard that does the same trick with a unique_ptr.
This was Eric's question so he might expand a bit on this, but this is good for me. The one point I would like to make would be to encourage you to define RAPIDJSON_NAMESPACE so you don't violate the ODR with clients linking with your library that are also using rapidjson. |
|
Thanks for the quick and clear answers!
So, from a user point-of-view, if I have 2 instances of Timeline objects (each one being the root node of a complete OTIO tree) and I want to compare both trees, I can use the SerializableObject::is_equivalent_to function to do timeline1.is_equivalent_to(timeline2)? If this function is recursive, I don't see how it can reach each children's primitive. For instance, how would it reach the LinearTimeWarp's "_time_scalar" property? The only virtual functions in the LinearTimeWarp class are read_from/write_to and those are for serialization, so probably not used in the implementation of the (non-virtual) is_equivalent_to function. And I don't see a way for the derived classes (eg.: LinearTimeWarp) to push its primitives in a base class container (of anys). I might be missing something here though. :) Also, my questions/concerns about exposing (or not) rapidjson Document and us using it to compare OTIO trees are actually answered by the fact that we could do the comparison using 2 C++ trees (agani, with SerializableObject::is_equivalent_to function). The fact that you use std::map internally is not a problem at all. |
|
So, from a user point-of-view, if I have 2 instances of Timeline objects (each one being the root node of a complete OTIO tree) and I want to compare both trees, I can use the SerializableObject::is_equivalent_to function to do timeline1.is_equivalent_to(timeline2)? If this function is recursive, I don't see how it can reach each children's primitive. For instance, how would it reach the LinearTimeWarp's "_time_scalar" property?
Ah. I see your concern. The Python version of ‘is_equivalent_to’ basically serialized to JSON and compared, which we all found a bit, er unsettling.
The C++ version is similar, but faster/less work. It serializes both datastructures as if it was going to do json output, but stops when it reaches the level of int/string/bool/RationalTime, etc. then it just compares those (which are sitting in any’s).
Think of serialization/deserialization as producing/reading from a std::map<string,any> down to the “atomic level”. At the end, the comparison problem is trivial.
Basically, by defining how to serialize/deserialize your class using the write_to() and read_from() methods, and registering with the type registry, you’ve let me (the library implementor) write clone() and is_equivalent_to() for all the schemas/derived classes you come up with. Even if you put properties in member variables that i couldn’t possibly know anything about, it still works just great.
… The only virtual functions in the LinearTimeWarp class are read_from/write_to and those are for serialization, so probably not used in the implementation of the (non-virtual) is_equivalent_to function. And I don't see a way for the derived classes (eg.: LinearTimeWarp) to push its primitives in a base class container (of anys). I might be missing something here though. :)
Also, my questions/concerns about exposing (or not) rapidjson Document and us using it to compare OTIO trees are actually answered by the fact that we could do the comparison using 2 C++ trees (agani, with SerializableObject::is_equivalent_to function). The fact that you use std::map internally is not a problem at all.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub <#391 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AMFkpaGap4t7st6MJiXtZeSsKsMwj2I6ks5u2V86gaJpZM4Y4nLj>.
|
|
Just to make it easy for people to find, the headers linked by the documentation that @davidbaraff is adding can be found at this link: |
|
Lets integrate these documentation changes back into the master branch now that we've landed the C++ branch into master. |
|
Addressed by #610 |
This adds a proposal for the new C++ strategy for OTIO. Sample header files to be left on my forked branch and not put in the true PixarAnimationStudios/OpenTimelineIO repository.