TheAcharya / MarkersExtractor

Extract Markers from Final Cut Pro FCPXML
MIT License
36 stars 2 forks source link

Roles not properly parsing #66

Closed orchetect closed 10 months ago

orchetect commented 10 months ago

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 commented 10 months ago

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 commented 10 months ago

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 commented 10 months ago

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.)

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.

IAmVigneswaran commented 10 months ago

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

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

orchetect commented 10 months ago

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 commented 10 months ago

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 commented 10 months ago

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.

https://github.com/TheAcharya/csv2notion-neo/issues/9

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

orchetect commented 10 months ago

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 commented 10 months ago

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 commented 10 months ago

we can capitalize first character

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

orchetect commented 10 months ago

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 commented 10 months ago

Thanks for the update!

IAmVigneswaran commented 10 months ago

Just tested this. 👍

roles-parsing

Will close this for now.