Add enabled flag to Item

[ThomasWilshaw]

Fixes #1102

Updates the schema to add an enabled flag to Item. Proposal can be seen here. Any comments or suggestions are very welcome.

[jminor]

This is looking great. It would be nice to have a simple unit test which toggles the enabled boolean and ensures that it loads/saves correctly.
After that, then adding support to one or more adapters would be great. There is some code to handle muted clips in the AAF adapter which could be modified to use this. If you want to do that in a separate PR, we could land this one into a branch to allow for adapter work based on it.

[jminor]

Oops, I pressed the "Ready for Review" button which made this no longer a "draft" PR. I meant to do that to a different PR. I'm not sure how to undo that...

[JeanChristopheMorinPerso]

Should the term "enabled" be documented so that the semantic around it are clear? Like what does this field do and how should it be used? It's the in proposal but I feel like it could potentially be documented in OTIO directly.

Opinions?

[reinecke]

We'd talked about adding enabled to Effect as well (these are not Item subclasses). Do we want to address that in this PR, or add a second PR?

[ssteinbach]

Should the term "enabled" be documented so that the semantic around it are clear? Like what does this field do and how should it be used? It's the in proposal but I feel like it could potentially be documented in OTIO directly.

Opinions?

Agree.

There are a bunch of algorithms that potentially interact with this.

Questions:

• Does a disabled clip contribute to the length of a track?
• Does a disabled clip composite transparently or opaquely?
• Does the result of .visible() change w/ the state of enable?
• Does each_clip return disabled clips as well?
• does the result of .overlapping() change for enabled/disabled clips?
• What happens if a transition includes a disabled clip?

[jminor]

I think all of those questions are answered by "a disabled Item behaves just like a Gap." In more detail:

• Does a disabled clip contribute to the length of a track?

Yes, just like a Gap. Toggling the enable/disable state of an Item should not change the duration of that Item, nor anything else on the Track it lives in.

• Does a disabled clip composite transparently or opaquely?

Transparently, just like a Gap.

• Does the result of .visible() change w/ the state of enable?

Yes. When enabled==true an Item should behave like a Gap, which includes returning false from .visible(). This one change should, I think, make the flatten algorithm behave correctly.

• Does each_clip return disabled clips as well?

Yes. It is up to the caller to decide what to do with disabled clips. We could consider offering a convenience to find only enabled clips.

• does the result of .overlapping() change for enabled/disabled clips?

No. Only Transitions are overlapping.

• What happens if a transition includes a disabled clip?

Nothing special. It works just like a Gap.

[meshula]

The default for the flag for any item is enabled = true. Structures in most languages default initialize elements to zero. Would it make sense to have an opposite sense flag as the default?

For example, "omitted", might be a word that communicates the flag intention better, and has a zero default? Or maybe "is_gap" would answer all of Stephan's questions immediately without searching documentation, while also being a zero default? The documentation could then clarify that the difference between Gap and Item::is_gap == true is that this is intended as a way to make a placeholder, or omitted item behave as a gap without losing its authored information?

[jminor]

We have plenty of other things that initialize to non-zero values (e.g RationalTime's rate) and I think we should prioritize semantic clarity over implementation details.
Using "is_gap" doesn't feel right to me. A disabled clip is still a clip, not a gap. It just behaves like a gap.
Using "omitted" instead of "enabled" makes sense, but I worry about double-negatives in conditional statements. I find "if enabled" more clear than "if not omitted".
Code that needs to treat Gaps and disabled Items uniformly should use item.visible() which will return false for both. There's an argument that "visible" isn't the right word for audio clips, but that could be a different change than this PR.

[meshula]

omitted doesn't really carry the idea that it still is part of the timing of the composition. Just chewing on the semantics of "enabled" a little more... The only thing that still makes me a bit squirmy, is that the information carried within an item includes whether or not it's visible. In other respects, enabled==false just means that the item does not compose visually or audibly. The core information about the clip being visible seems a property of the clip. If it's meaning changes according to enabled, we can't ask a clip if it's intended to be visible, without first enabling it. If the flatten algorithm asked about visible() && enabled() it's still at trivial tweak to get this functionality, and doesn't hide any of the it's inherent qualities.

.... (edit)

I guess that's moot. visible() is an implementation detail that is not serializable, and whose implementation could currently be rewritten as return dynamic_cast<Gap*>(item) == nullptr;

I had had it in my mind that visible was authorable qualia.

[ThomasWilshaw]

Thanks for all the feedback. I'll try and reply to all the points here.

Unit Tests

Should I just add a new method to ItemTest called something like test_enabled(self)? I guess I don't need to add tests for enabled for any of the other classes like Clip etc. as it should be inherited?

Should I also extend the other Item tests to include the enabled flag, for exmaple the constructor test?

Adapter support

I'm happy to have a go at adding an adapter but if I get bogged down in it I'll move it to another PR.

Documentation

I don't see any harm in adding some documentation, where would be the best place for it?

Effect

I'll add enabled to Effect in this PR for completeness.

Transitions
Transitions aren't Items so I guess I need to add enabled there as well?

Algorithms

Would flatten be a good place to start with these?

General Questions

1. Should I add a convenience function each_enabled_clip?
2. How should a Gap behave if enabled==false?
3. Do I need to update all instances of visable() to return false if disabled?
4. With the code as is, in Python I can write item = otio.schema.Item(enabled=False) but not clip = otio.schema.Clip(enabled=False). Do I need to go through and explicitly add enabled to all the constructors in both the C++ and Python code to be able to do this? I've never used PyBind before so what I've done so far has been slightly guess work.
5. In general should I try and keep everything in one PR or would you rather I get smaller changes completed and merged before moving on?

[reinecke]

Timelines have a lot of use cases. When talking about visible or enabled what we're getting at is the composition renderer use case - does this item contribute to the rendering of the composition? If the visible computed property were renamed to something like should_be_rendered it would clarify the semantics of what that serves.

[reinecke]

As discussed in the TSC meeting, ImageSequenceReference has some good examples about how to add documentation to properties in the python bindings, see here.

[reinecke]

In response to the question of whether or not to add enabled to the constructors: I think we can leave it out of the subclass constructors for now. If, at some point, we decide we want to add it, we can go through and do that separately.

[ThomasWilshaw]

Thanks. Just to clarify would you leave enabled the Item constructor or shall I remove it?

[reinecke]

I don't have strong a strong opinion about the enabled flag on the Item constructor - Item isn't typically used directly so I think it's fine for it to be a bit more verbose.

[ThomasWilshaw]

Okay I'll leave it for now then.

[codecov-commenter]

Codecov Report

Merging #1175 (4470726) into main (4d2f668) will decrease coverage by 0.07%.
The diff coverage is 81.51%.

@@            Coverage Diff             @@
##             main    #1175      +/-   ##
==========================================
- Coverage   86.21%   86.14%   -0.08%     
==========================================
  Files         191      191              
  Lines       19047    19279     +232     
  Branches     2104     2295     +191     
==========================================
+ Hits        16422    16607     +185     
- Misses       2079     2117      +38     
- Partials      546      555       +9     

Flag	Coverage Δ
unittests	86.14% <81.51%> (-0.08%)	⬇️

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

Impacted Files	Coverage Δ
src/py-opentimelineio/opentimelineio/__init__.py	100.00% <ø> (ø)
...-opentimelineio/opentimelineio/console/__init__.py	100.00% <ø> (ø)
tests/test_console.py	92.41% <50.00%> (-1.25%)	⬇️
...opentimelineio/opentimelineio/adapters/cmx_3600.py	85.56% <71.42%> (+0.07%)	⬆️
...neio_contrib/adapters/advanced_authoring_format.py	81.42% <73.64%> (-5.00%)	⬇️
...elineio_contrib/adapters/tests/test_aaf_adapter.py	98.36% <100.00%> (+0.09%)	⬆️
src/opentimelineio/item.cpp	90.76% <100.00%> (+0.29%)	⬆️
src/opentimelineio/item.h	100.00% <100.00%> (ø)
src/opentimelineio/stackAlgorithm.cpp	80.39% <100.00%> (+0.39%)	⬆️
...entimelineio-bindings/otio_serializableObjects.cpp	91.41% <100.00%> (+0.03%)	⬆️
... and 15 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 6be5d24...4470726. Read the comment docs.

[ThomasWilshaw]

I've updated otioview to grey out clips and tracks (see below) but I'm now wondering, if a track is disabled should everything in it have enabled set to false or should views/algorithms take care of that discontinuity? Same would apply for nested items as well I guess?

[meshula]

With regards hierarchical enabling, consider this scenario:

1. (track (clip1 clip2 clip3))
2. clip2.enable=false
3. track.enable = false

Now consider if clips are according to track, all clips are enable=false

4. track.enable = true

Now all clips are enable=true

we've lost information from step 2

So I'd say the enable flag is a compositional information, not an explict datum. In other words, if a track is enabled=false, then the composition of the track should not be evaluated.

[meshula]

I'm approving, with a tweak suggested to the docstring. Code LGTM.

I'm also requesting a second approval due to the size of the diff.

[ThomasWilshaw]

Thanks, I've implemented the change in the latest commit.

R.e. the hierarchical enabling I see what you mean yes, however in otioview I've set the enabled status of everything in a track to False but I guess that's acceptable in this case as otioview is read only?

[meshula]

otioview is indeed view only :)

[jminor]

This is looking really good, and seems to be almost complete. I have two questions:

1. The virtual bool visible() const function is now arguably redundant since bool enabled() const returns the same thing. Should we mark visible() as deprecated & plan to remove it later? Or is there any use case where we would want to keep that? Notably, visible() is virtual and enabled() is not and Gap has overridden visible() to always be false. Is that enough of a reason to keep both? If so, we should clearly document the difference.

2. Since this PR introduces a new field on Item, it should maybe also increment the schema version for Item. This allows for an upgrade function to fill in the default value - but is that needed in this case? If we read JSON with an old Item without this field does it default to true? It might not have come up, since we rarely serialize an Item directly. Everything is typically a Clip, Gap, etc. which are subclasses of Item. This brings up the question of whether all the subclasses need to increment their schema version also. Are we just fortunate that the default value of this can be provided without incrementing the version? There's both a conceptual and a pragmatic decision rolled together here.

[jminor]

We discussed the questions from my previous comment in yesterday's OTIO TSC meeting.

We decided that the visible() and enabled() accessors should stay as-is for now since Gap's always-invisible behavior is working well for flattening. There may be some future work to reconcile the terminology of visible/enabled for audio where the word "muted" is typically used for the same idea.

For the schema versioning question, we decided to not increment the Item schema version number since the new enabled field is optional, and defaults to true when reading files that don't have that field. This is the easiest case of backwards compatibility since the upgrade is trivial. The only case to watch out for is that older software will read & write an OTIO dropping the enabled field, and thus some workflows involving a mixture of OTIO versions could inadvertently cause all the clips in an OTIO to become enabled. While that may be unexpected, it is still better than simply not supporting enabled/disabled clips at all. We can keep this case in mind as we work to make our forward/backward compatibility strategy more robust.

[jminor]

Thanks for the contribution @ThomasWilshaw ! This is a great improvement for OTIO and you did an excellent job of working with the community to make this PR very clean & robust.