Skip to content

Conversation

@douglascomet
Copy link
Contributor

@douglascomet douglascomet commented May 5, 2022

Potentially starts to fix the below issues.

Fixes #629
Fixes #627

Looking for feedback on this approach with the svg adapter and then I'll move on to the other adapters

@meshula
Copy link
Collaborator

meshula commented May 6, 2022

I think this looks reasonable. Have you got a before/after svg rendering to show what the impact of the changes is?

available_range = clip.source_range
if clip.media_reference.available_range:
available_range = clip.available_range()
return available_range
Copy link
Collaborator

@jminor jminor May 6, 2022

Choose a reason for hiding this comment

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

This would read more clearly as something like:

if clip.media_reference and clip.media_reference.available_range:
    return clip.available_range()
else:
    return clip.source_range

Copy link
Contributor Author

@douglascomet douglascomet May 8, 2022

Choose a reason for hiding this comment

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

I landed on this because pylint said the else was unnecessary. Thoughts?

Also, I noticed that the repo's setup.cfg had some settings for flake8 and not pylint but the CI/CD tests do use pylint so not sure which linter I should be using.

Suggested change
return available_range
if clip.media_reference and clip.media_reference.available_range:
return clip.available_range()
return clip.source_range

@jminor
Copy link
Collaborator

jminor commented May 6, 2022

The SVG adapter was aiming to create diagrams similar to the ones on this documentation page: https://opentimelineio.readthedocs.io/en/latest/tutorials/otio-timeline-structure.html

Those were made by hand (and I'd love to not have to do it by hand again...) One of the tricky things that came up when we made those was trying to find a clear way to show when the source_range was or was not set. If you look closely, some of them intentionally have no source_range in an attempt to show how available_range is used instead. However that important detail is really hard to see, even if you're looking for it.

For this PR, you have sort of the opposite problem. If available_range is not know, then you can guess that the source_range could be used instead - but it hides an important detail - that available_range is null.

I wonder if there's a way to draw attention to the fact that either available_range or source_range is missing? Note: If both are missing, then the clip is invalid and has no way of knowing its duration.

@douglascomet douglascomet force-pushed the fix_svg_target_url_bug branch from 77b9506 to 063e7ec Compare May 14, 2022 21:00
@douglascomet douglascomet force-pushed the fix_svg_target_url_bug branch from 063e7ec to 4fb0574 Compare May 14, 2022 21:08
@reinecke reinecke added the Adapters Relating to the adapters that live in the separate repos label Oct 27, 2023
@reinecke
Copy link
Collaborator

We should move this PR over to OpenTimelineIO/otio-svg-adapter.

@douglascomet
Copy link
Contributor Author

Thanks for the reminder @reinecke! I completely forgot about this PR. I think the actual fixes this PR introduced have been lost because of all the code style choices I made and in hindsight shouldn't have done. I will go through git history and pull over the necessary stuff to the svg adapter repo as you described and then close out this PR.

@douglascomet
Copy link
Contributor Author

Closing this PR because it was continued in the SVG adapter repo: OpenTimelineIO/otio-svg-adapter#7 and is out of date with the current state of main in the OTIO repo.

@douglascomet douglascomet deleted the fix_svg_target_url_bug branch October 31, 2023 01:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Adapters Relating to the adapters that live in the separate repos

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants