Skip to content
This repository has been archived by the owner on Sep 20, 2024. It is now read-only.

Burnin: Get data from context with defined keys. #1897

Merged
merged 6 commits into from
Oct 6, 2021

Conversation

tokejepsen
Copy link
Member

@tokejepsen tokejepsen commented Aug 4, 2021

This PR enables studios to grab data from the pyblish context to use on the burnins.

Our use case is to get the task time and inject into the context context.data["task_time"] with a studio plugin. Then in the settings for the burnin plugin we can use this key:

context[task_time]

[cuID:yjx9e9]

@tokejepsen tokejepsen self-assigned this Aug 4, 2021
@mkolar
Copy link
Member

mkolar commented Aug 4, 2021

Very nice!

We'll do some testing, but I love the idea

@tokejepsen
Copy link
Member Author

Its currently not as simple as it maybe should be. The pyblish context cant be serialized and its also quite a bit of data to pass to the burnin script, so I'm trying to lighten the load by finding which key is being queried by the settings and only send that to the burnin script.

@iLLiCiTiT
Copy link
Member

I think it would be better to have defined one specific key on context which could hold possible "additional data" for burnins so it define that "change value of this key will affect your burnin values" instead of using all context data which is undocumentable, abstract and dangerous as I wouldn't want to go through all places where key changed value in context.data to find out why burnins have this value and not what I expected.

@tokejepsen
Copy link
Member Author

I think it would be better to have defined one specific key on context which could hold possible "additional data" for burnins so it define that "change value of this key will affect your burnin values" instead of using all context data which is undocumentable, abstract and dangerous as I wouldn't want to go through all places where key changed value in context.data to find out why burnins have this value and not what I expected.

That sounds good to me. Got any naming preference?

@iLLiCiTiT
Copy link
Member

Got any naming preference?

Not really. I have thought about burnins_data but maybe something more suitable :)

@tokejepsen
Copy link
Member Author

Got any naming preference?

Not really. I have thought about burnins_data but maybe something more suitable :)

Just so I get this right. We want to query context.data["burnin_data"] (variable name pending), and in the settings we input context["task_time"]?
This would mean that burnin_data will need to be a dict, which might be a bit implicit.

@iLLiCiTiT
Copy link
Member

Well it's against the idea which is in description of using "context". I'm just scared about giving such huge unpredictable access to burnins. It's much safer to define which keys can be from context used in code than give full access to it through template.

@jakubjezek001
Copy link
Member

In fact this could be collected already in openpype\plugins\publish\collect_anatomy_instance_data.py, since those data are then passed into burnins openpype\plugins\publish\extract_burnin.py:391. And I guess context level could be skipped only to task_time key.

@tokejepsen
Copy link
Member Author

Well it's against the idea which is in description of using "context". I'm just scared about giving such huge unpredictable access to burnins. It's much safer to define which keys can be from context used in code than give full access to it through template.

Yeah, I'm all for this. More explicit the better.
My question is more towards naming etc. Maybe burnin_context is a better fit? Then inputting context into the burnin settings might seem more logical.

@tokejepsen
Copy link
Member Author

Have simplified this a lot, so it just really needs documentation. The burnin_context data member could be anything really depending on how you access it from the settings, but having a dictionary is probably most useful.

@mkolar mkolar added the type: enhancement Enhancements to existing functionality label Aug 19, 2021
Co-authored-by: Milan Kolar <mkolar@users.noreply.github.com>
Co-authored-by: Milan Kolar <mkolar@users.noreply.github.com>
@mkolar mkolar self-requested a review September 24, 2021 16:45
@mkolar mkolar merged commit a61d9c0 into ynput:develop Oct 6, 2021
@tokejepsen tokejepsen deleted the feature/burnin_context_data branch October 6, 2021 16:24
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
type: enhancement Enhancements to existing functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants