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

Roles not properly parsing #66

Closed
orchetect opened this issue Nov 23, 2023 · 13 comments
Closed

Roles not properly parsing #66

orchetect opened this issue Nov 23, 2023 · 13 comments
Assignees
Labels
bug Something isn't working fcpxml Parsing related to fcpxml(d)
Milestone

Comments

@orchetect
Copy link
Contributor

orchetect commented Nov 23, 2023

In 0.2.5, roles are not being parsed fully in the test project (BRS_LOCKED6_20210521 - 01 Opening Scene 2023-11-22 07-59-54 - #37).

285124502-659e7bdd-a012-440f-ab11-a899607cc928 285124521-1b7772f5-f06d-4556-b6d8-3f529cbfd150
@orchetect orchetect added bug Something isn't working fcpxml Parsing related to fcpxml(d) labels Nov 23, 2023
@orchetect orchetect added this to the 0.2.6 milestone Nov 23, 2023
@orchetect orchetect self-assigned this Nov 23, 2023
@orchetect
Copy link
Contributor Author

orchetect commented Nov 23, 2023

My first guess is that roles are not yet being fully parsed from compound clips, sync clips and multicam clips. Specific parsing logic is needed for them because of their more complex internal structure. Won't know for sure until I can debug.

@orchetect orchetect modified the milestones: 0.2.6, 0.2.7, 0.2.8 Nov 25, 2023
@orchetect
Copy link
Contributor Author

orchetect commented Nov 28, 2023

I now have sync-clips parsing roles correctly, even in very complex projects. It was extremely nuanced when nested clips and multiple audio sources were involved and took some very careful fine-tuning of the parser to accommodate. MarkersExtractor will now output the roles as they are seen in Final Cut Pro (and therefore as the user expects).

Next one to tackle is multicam (mc-clips) which are equally nuanced but in different ways.

I'd like to get fixes for both in 0.2.8.

@orchetect orchetect pinned this issue Nov 29, 2023
@orchetect
Copy link
Contributor Author

orchetect commented Nov 30, 2023

While we're at it, there are two questions about how we should format data that I've had come up recently while reworking things.

1. Title Case for Default Roles

What I'm observing is that sometimes FCP exports roles as lowercase. From what I can tell, it only does that for "built-in" or "default" roles.

For example, FCP will show "Dialogue" but in the FCPXML it will output the role as "dialogue" or "dialogue.dialogue-1".

I have noticed it happen with "Dialogue" and "Music" roles, and I believe "Effects" may be lowercased as well.

(User-defined roles are always verbatim - character case is never changed when output to FCPXML.)

This means that currently, some default roles may output to the manifest file as lowercase since MarkersExtractor is taking them as-is. (The only modification currently being done is reducing the subrole when appropriate. ie: "dialogue.dialogue-1" will be reduced to "dialogue" for the manifest file.)

  • 💡 Question: Do we want to title-case these roles so they appear as they do in FCP? Essentially there would be a process added that detects default role names and makes them title-case (capitalizes first character).

2. Matching FCP's Display of Roles: Main vs. Sub-Role

FCP only shows the sub-role in the clip inspector. This is for both audio and video roles. Video roles have an automatic sub-role generated by FCP even though it's not obvious. The audio sub-role is more obvious since it breaks them out and forces you to use them. Small technical detail, but useful to know.

Currently we output both main role and sub-role for video role when it's not reducible. ie: "VFX.Smoke". However, FCP will display this in the inspector view as just its sub-role, ie: "Smoke".

role-editor

video-role-inspector

Obviously, outputting main + sub-role provides more data for us to work with.

However, with more recent refactors and bug fixes, I have made audio roles output as FCP displays them in certain cases such as sync-clips where there may be multiple audio sources.

For example, if two audio sources "Dialogue.MixL" and "Dialogue.MixR" are enabled, FCP displays it in the inspector as a comma-separated list of sub-roles, ie: "MixL, MixR". This is what MarkersExtractor will output to the manifest file for Audio Role. And this is what I suspect users will expect.

multiple-audio-roles

In consideration of making the manifest output consistent, do we also implement this for Video Role to only show sub-role?

If we don't, then do we make Audio Role show main + sub-role for all roles? So instead of "MixL, MixR" it would read as "Dialogue.MixL, Dialogue.MixR". The trouble with this is that it can get very long very quickly. In that complex project you sent me, sometimes there are upwards of 6 audio sources and it is far more concise to use FCP's behavior of only using sub-role.

  • 💡 Question: Do we want to match FCP's display of Roles in the user interface for our output manifest?

@IAmVigneswaran
Copy link
Contributor

Question: Do we want to title-case these roles so they appear as they do in FCP? Essentially there would be a process added that detects default role names and makes them title-case (capitalizes first character).

Cosmetically, it would look nice in the database if we can capitalises first character?

In consideration of making the manifest output consistent, do we also implement this for Video Role to only show sub-role?

I think it would be highly beneficial for users If we can have MainRole.Subrole. It becomes more consistent? Subrole would never be printed or list alone in the manifest output?

Users would have multiple VFX "Stems" for the shots.

Video-Sub-Roles-Images

Hence in the Database's Video Role & Subrole column, users can have VFX, VFX.Mirror Ball, VFX.Clean Plate, VFX.Main Plate values in the database. And users can easily create a search filter where Video Role & Subrole contains "VFX".

If we don't, then do we make Audio Role show main + sub-role for all roles? So instead of "MixL, MixR" it would read as "Dialogue.MixL, Dialogue.MixR". The trouble with this is that it can get very long very quickly. In that complex project you sent me, sometimes there are upwards of 6 audio sources and it is far more concise to use FCP's behavior of only using sub-role.

Question: Do we want to match FCP's display of Roles in the user interface for our output manifest?

Does it make sense to include all of them? Example - Dialogue.MixL,Dialogue.MixR,Dialogue.Boom-1,Dialogue.Boom-1 etc.

That way, all the values would become a multi-select item in the database. It also gives users insight into their timeline if they have missed anything. Just some thoughts.

PS: Don't include space when adding ",".

Using Custom Column Types

csv2notion_neo --token YOUR_TOKEN_HERE --column-types "number,multi_select" test.csv

@orchetect
Copy link
Contributor Author

orchetect commented Nov 30, 2023

I think it would be highly beneficial for users If we can have MainRole.Subrole. It becomes more consistent? Subrole would never be printed or list alone in the manifest output?

Sure, that's doable.

Does it make sense to include all of them? Example - Dialogue.MixL,Dialogue.MixR,Dialogue.Boom-1,Dialogue.Boom-1 etc.

That would then be consistent, yes. More verbose, but no information is abbreviated or lost.

Don't include space when adding ,.

Sure. FCP uses , (with the space) as the delimiter when displaying multiple roles in the UI, so I was just following that convention.

all the values would become a multi-select item in the database

Clever.

@orchetect
Copy link
Contributor Author

orchetect commented Nov 30, 2023

One possible wrinkle is that FCP allows the , comma character to be used when the user names roles. So a consumer of the CSV parsing this field may parse it incorrectly if it relies on a comma delimiter.

A possible solution might be to use a null character (ASCII code 0, or "\n" in Swift string literal) instad of a comma to separate multiple roles.

For example, the Audio Role & Subrole field may contain these three totally valid roles:

Dialogue.MixL
Dialogue.MixR
Some Role, With Comma.Some Role, With Comma-1

With a comma delimiter, it would be entered in the manifest as follows (and would not parse correctly):

Dialogue.MixL,Dialogue.MixR,Some Role, With Comma.Some Role, With Comma-1

With a null delimiter, it would be possible to parse it. (Represented as <NULL> here for demonstration.)

Dialogue.MixL<NULL>Dialogue.MixR<NULL>Some Role, With Comma.Some Role, With Comma-1

As a side note, this is a complication when using CSV for an output manifest as it is a fairly simple data format. If we used JSON instead, this would not be necessary, as it capable of nested collections.

@IAmVigneswaran
Copy link
Contributor

IAmVigneswaran commented Nov 30, 2023

As a side note, this is a complication when using CSV for an output manifest as it is a fairly simple data format. If we used JSON instead, this would not be necessary, as it capable of nested collections.

True!

Let me try to get CSV2Notion Neo to support json upload.

TheAcharya/csv2notion-neo#9

If that can be possible, we will drop csv and switch to json for both Notion and Airtable Profiles.

@orchetect
Copy link
Contributor Author

drop csv and switch to json

It's not a bad idea, honestly. As the manifest data becomes more descriptive over time, we may outgrow the capabilities of CSV any way.

@orchetect
Copy link
Contributor Author

I now have sync-clips parsing roles correctly

I have now also resolved Multicam roles parsing issues, and the fix for both will be in 0.2.8.

All roles should be parsing correctly now. That will allow us to move onto other features next-in-line, like role exclusion (#57).

@orchetect
Copy link
Contributor Author

we can capitalize first character

Default roles are now title-cased to match FCP. Will be in 0.2.8.

@orchetect
Copy link
Contributor Author

orchetect commented Nov 30, 2023

we can have MainRole.Subrole. It becomes more consistent
include all of them. Example - Dialogue.MixL,Dialogue.MixR,Dialogue.Boom-1,Dialogue.Boom-1

Multiple roles are now output to the manifest with full MainRole.Subrole (unless collapsible to just a main role), using a comma delimiter without a space (,). Will be in 0.2.8.

@IAmVigneswaran
Copy link
Contributor

Thanks for the update!

@IAmVigneswaran
Copy link
Contributor

Just tested this. 👍

roles-parsing

Will close this for now.

@IAmVigneswaran IAmVigneswaran unpinned this issue Dec 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working fcpxml Parsing related to fcpxml(d)
Projects
None yet
Development

No branches or pull requests

2 participants