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

Merge all review tags into one #4569

Closed
EmberLightVFX opened this issue Mar 3, 2023 · 13 comments
Closed

Merge all review tags into one #4569

EmberLightVFX opened this issue Mar 3, 2023 · 13 comments
Assignees
Labels
type: refactor Structural changes not affecting functionality

Comments

@EmberLightVFX
Copy link
Collaborator

EmberLightVFX commented Mar 3, 2023

Currently we have 3 different review tags. ftrackreview, shotgridreview and kitsureview.
I don't see a reason why we would have 3 different review tags instead of just one.

Maybe there are some extreme edge-case where a production uses two of theses review tools but I have never heard about it myself.
Thoughts?

[cuID:OP-5182]

@BigRoy
Copy link
Collaborator

BigRoy commented Mar 13, 2023

Should we still somehow keep a review-upload (just as example; replacement for the 3 tags you mentioned) tag separate from review itself? Or should we just always upload a review by default?

@EmberLightVFX
Copy link
Collaborator Author

Should we still somehow keep a review-upload (just as example; replacement for the 3 tags you mentioned) tag separate from review itself? Or should we just always upload a review by default?

That's a good question.

I myself have never bin in a situation where I haven't uploaded a review file so I would say always upload a review.

What about you?

@BigRoy BigRoy added the type: refactor Structural changes not affecting functionality label Mar 17, 2023
@Tilix4
Copy link
Collaborator

Tilix4 commented Mar 30, 2023

IMO review is sufficient. Review means "to be looked at by who is supposed to", therefore it must be uploaded to every production tracker we enabled.

@BigRoy BigRoy mentioned this issue Mar 30, 2023
3 tasks
@BigRoy BigRoy self-assigned this Mar 30, 2023
@iLLiCiTiT
Copy link
Member

Just to note, we have use-cases when something is marked as review so ExtractReview can create outputs but not all outputs are uploaded to "manager". So I disagree that "review" and "ftrackreview" are interchangeable.

Ad merging them, I think that @mkolar have an opinion on that front.

@EmberLightVFX
Copy link
Collaborator Author

I can see that the solo review tag should separated but ftrackreview, shotgridreview and kitsureview could be merged into one. Maybe rename it to something like Upload to review platform to really show what the tag does.

@Tilix4
Copy link
Collaborator

Tilix4 commented Mar 31, 2023

Things are becoming clearer 🥳 I'd have proposed the same thing as @EmberLightVFX did.

@BigRoy
Copy link
Collaborator

BigRoy commented Mar 31, 2023

If I understand correctly then the current conclusion is that having "review" + "review upload" separate should be fine.
We could also instead say that "review" is the upload, and there's a "create-reviewable" tag or something.

If there's a better preference than review + review-upload tags let me know @mkolar so I can maybe try and tweak the #4757 PR accordingly.

@EmberLightVFX
Copy link
Collaborator Author

create-reviewable and review upload sounds good to me

@iLLiCiTiT
Copy link
Member

Note: If we'll do any of these changes it should go to release branch and not to develop.

@Tilix4
Copy link
Collaborator

Tilix4 commented Mar 31, 2023

I suggest tags should have different syntax from families, therefore @EmberLightVFX's proposal looks perfect to me (not having a review tag, which is very confusing)

@BigRoy
Copy link
Collaborator

BigRoy commented Apr 6, 2023

@mkolar are you able to share your thoughts about this?

@mkolar
Copy link
Member

mkolar commented Apr 17, 2023

I'd agree that having review in tags and in families is confusing. So changing tag review to create-reviewable has some merit. Even though I'm not sure it's enough to justify the change that has a potential to break a lot of things if we're not extremely careful. even then, I'd argue that it should then be even more generic. For example transcode, considering that's what's actually happening in the extract review plugin that is interested in the tag.

Merging the three other tag, however, I don't think is a good idea. It's being less specific and assumes too much. We're at the start of syncsketch integration for example and will probably add others later, frame.io, vimeo, whatever. Suddenly it make perfect sense to upload one review output to ftrack (internal review with burnins), but another output to syncsketch for a client review with different burnins, or with watermark.

Point being. If we want to tell individual outputs where they should ultimately appear, we can't just assume its always project management.

@mkolar
Copy link
Member

mkolar commented May 30, 2023

Guys this might seem like its simplifying something, but it would take functionality that is crucial at the moment away. Hence I'm moving it to discussions for posterity, but don't see it as an issue.

@mkolar mkolar closed this as completed May 30, 2023
@ynput ynput locked and limited conversation to collaborators May 30, 2023
@mkolar mkolar converted this issue into discussion #5067 May 30, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
type: refactor Structural changes not affecting functionality
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants