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.

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

AY-6731_Allow CSV ingest to create new shots. #1010

Merged
merged 9 commits into from
Nov 20, 2024

Conversation

robin-ynput
Copy link
Contributor

@robin-ynput robin-ynput commented Nov 12, 2024

Changelog Description

This work goes along CSV ingest file tweaks on ynput/ayon-traypublisher#36.
It allows ayon-traypublisher to leverage Collect Hierarchy and Extract hierarchy to AYON and benefit from shot logic when ingesting CSV. This make it able to create new shots.

Testing notes:

  1. This needs to be tested with Allow CSV ingest to create new shots. ayon-traypublisher#36 ingesting CSV files.

@ynbot ynbot added the size/XS label Nov 12, 2024
@iLLiCiTiT
Copy link
Member

iLLiCiTiT commented Nov 13, 2024

We're adding a lot of csv ingest specific logic into core global plugin. There is "csv_ingest_shot" family, why it is not "shot" in traypublisher? Why we don't set attributes during csv ingest, could we handle that in a different way?

@robin-ynput
Copy link
Contributor Author

robin-ynput commented Nov 13, 2024

We're adding a lot of csv ingest specific logic into core global plugin. There is "csv_ingest_shot" family, why it is not "shot" in traypublisher? Why we don't set attributes during csv ingest, could we handle that in a different way?

That's a very good point. I tried to explain it in the other PR but let me report it here as well.

The "shot" family expects a lot of timeline/resolution data that comes mostly from OTIO and the editorial host but which do not not exist for CSV ingestion. So I thought about those options:

  • A- tweak ayon_core to allow CSV to re-use a subset of the "shot" plugins for shot creation
  • B- duplicate some "shot" core logic and create new plugins for CSV ingestion in ayon_traypublisher
  • C- Somehow compute some OTIO and resolution data from ingested material in CSV. This'd imply a lot of assumptions and workflow decisions to be made (e.g. how to determine shot range/fps/resolution when multiple products are ingested for a shot ?).

Imo options B was making me duplicate a lot of code and C was too way out of the scope of the initial request so I went with A.. But very happy to change this if you guys disagree or think of something better.

Copy link
Member

@jakubjezek001 jakubjezek001 left a comment

Choose a reason for hiding this comment

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

Looking good

@iLLiCiTiT
Copy link
Member

iLLiCiTiT commented Nov 15, 2024

100% add comments why are we doing stuff when there is csv publish for future reference (nobody will remember why it is there).

My point is that we might not make it based on csv_ingest_shot but on some more "generic" family (without csv and ingest in it). Or make the logic data based instead of family based. We're skipping attribute because they are not set on instance when in traypublisher? Well maybe we should not set it if are not filled (you don't need family check for that).

And maybe think about why heroTrack is not filled in traypublisher?

@jakubjezek001
Copy link
Member

Imo options B was making me duplicate a lot of code and C was too way out of the scope of the initial request so I went with A.. But very happy to change this if you guys disagree or think of something better.

I like A too. Perhaps if it would be possible we could just simply enhance ayon-traypublisher\client\ayon_traypublisher\plugins\publish\collect_shot_instances.py plugin to allow both shot type instances. Since you can identify editorial type of shot instances by "creator_identifier" instance data key, it would make more sense perhaps stau within the scope of shot family but change the way the data are collected in mentioned plugin.

creator_identifier = instance.data["creator_identifier"]
    if creator_identifier == "editorial_shot":
          # do editorial related otio clip evaluation
	else:
		# do csv ingest logic.

@jakubjezek001 jakubjezek001 changed the title Allow CSV ingest to create new shots. AY-6731_Allow CSV ingest to create new shots. Nov 19, 2024
@ynbot
Copy link
Contributor

ynbot commented Nov 19, 2024

@iLLiCiTiT
Copy link
Member

Noice!

Copy link
Member

@jakubjezek001 jakubjezek001 left a comment

Choose a reason for hiding this comment

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

it seems that all works as it suppose to.

@robin-ynput robin-ynput merged commit 9293c28 into develop Nov 20, 2024
5 checks passed
@robin-ynput robin-ynput deleted the enhancements/allow_csv_ingest_to_create_shot branch November 20, 2024 13:16
@krishnaavril
Copy link

Hey! Can I get the csv template/sample test file, please? for testing

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

5 participants