-
Notifications
You must be signed in to change notification settings - Fork 127
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
Fix: Hero representation of version is not synchronized. #4191
Conversation
project_name, | ||
version_ids=[hero_version["_id"]], | ||
fields=["_id"], | ||
context_filters={"ext": [representation["context"]["ext"]]}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think it should be filtrered by extension in context but by "name"
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think so, I have representations which have name 'thumbnail'.
(But I have also some old representations without representation["context"]["ext"]
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So it should be filtered by representation name instead of extension. Extension on context can be not unique (there can be more then one representation with extension .mov
under single version). So this is unsave and could potentially cause sync of more representations.
representation = get_representation_by_id( | ||
project_name, representation_id | ||
) | ||
representation_parents = get_representation_parents( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You don't need all parents but only version. Or? It looks like all you need is
version_doc = get_version_by_id(representation["parent"])
representation_parents = get_representation_parents( | ||
project_name, representation | ||
) | ||
hero_version = get_hero_version_by_subset_id( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should check if hero version matches the version doc.
repre_doc = get_representation_by_id(
project_name, representation_id, fields=["_id", "parent"]
)
representation_ids = [repre_doc["_id"]]
version_doc = get_version_by_id(repre_doc["parent"])
if version_doc["type"] != "hero_version":
hero_version = get_hero_version_by_subset_id(
project_name, version_doc["parent"], fields=["_id", "version_id"]
)
if hero_version and hero_version["version_id"] == version_doc["_id"]:
hero_repre_doc = get_representation_by_name(
project_name,
repre_doc["name"],
hero_version["_id"]
)
representation_ids.append(hero_repre_doc["_id"])
query = {"_id": {"$in": representation_ids}}
...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would actually prefer to yank this logic out to openpype/plugins/load/add_site
(maybe check if it couldnt be handled in get_linked_representation_id
).
_update_site
will go away in v4 and we would need to redo this for v4 again.
I don't think also it should be responsibility of lowest method to do some complex logic.
(I understand that do this here would catch all possible ways this should get triggered, but I don't think it is too readable.)
@kalisp I'm out of work for three weeks. Therefore if you have some time to fix this painful issue don't wait for me! I was proposing things but I don't have enough time to make it clean quickly. |
Should be replaced with #4359 |
Replaced with #4359 |
Brief description
Hero version is not created on remote site when a representation is downloaded. This adds the related hero representation in
_update_site
when a version representation is fetched.Fix #4154
Replaces #4186
Testing notes: