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

TypedObjectPlugBinding : Add CompoundData->CompoundObject conversion #6029

Open
wants to merge 1 commit into
base: 1.4_maintenance
Choose a base branch
from

Conversation

johnhaddon
Copy link
Member

@danieldresser-ie, this is one possible approach to solving the DeadlineDispatcher userData problem reported on the Discord. I'm not sure how much I like it - see what you think...

This allows CompoundObjectPlug.setValue( compoundData ) to work, meaning we can remove the attributesCompatibility.py shim that was doing this manually for just Attributes.extraAttributes.

What I don't like about this is the mismatch between the C++ and Python APIs : it's still not possible to call CompoundObjectPlug::setValue( CompoundData ) in C++. And that's also mismatched with CompoundObjectPlug::setFrom() which is implemented to allow connections from CompoundDataPlug. Still, it seems like progress compared to the compatibility shim.

The immediate motivation for this is that it's not currently possible to have userDefault metadata for a CompoundObjectPlug, because metadata values must all be Data. This allows the registation of a CompoundData value which is converted to a CompoundObject when applied to the plug.

This allows `CompoundObjectPlug.setValue( compoundData )` to work, meaning we can remove the `attributesCompatibility.py` shim that was doing this manually for just `Attributes.extraAttributes`.

What I don't like about this is the mismatch between the C++ and Python APIs : it's still not possible to call `CompoundObjectPlug::setValue( CompoundData )` in C++. And that's also mismatched with `CompoundObjectPlug::setFrom()` which is implemented to allow connections from CompoundDataPlug. Still, it seems like progress compared to the compatibility shim.

The immediate motivation for this is that it's not currently possible to have `userDefault` metadata for a CompoundObjectPlug, because metadata values must all be `Data`. This allows the registation of a CompoundData value which is converted to a CompoundObject when applied to the plug.
@johnhaddon johnhaddon self-assigned this Sep 6, 2024
@danieldresser-ie
Copy link
Contributor

Hmm, that does feel kinda hacky. And I guess we'd be stuck with it fairly permanently if we went with this approach?

It sounds like Eric's got a solution to the short-term issue, so maybe the main question is the long term solution. You had mentioned allowing Metadata to be any Object, rather than just Data ... do you see any problem with using that solution in the long term?

@danieldresser-ie
Copy link
Contributor

Alternatively, if we do actually want this conversion as a long term feature, it would probably be doable to add it in C++?

@johnhaddon
Copy link
Member Author

You had mentioned allowing Metadata to be any Object, rather than just Data ... do you see any problem with using that solution in the long term?

I think this is logical in the long term (or even as early as 1.5), but we couldn't do it for 1.4, which is why I was looking for other solutions.

Alternatively, if we do actually want this conversion as a long term feature, it would probably be doable to add it in C++?

It does feel like we should maybe allow it in C++. It feels weird that you can connect AtomicCompoundDataPlug->CompoundObjectPlug but can't call setValue( CompoundData ). Since CompoundDataPlug is just a template instantiation I don't think we can just add a setValue( CompoundDataPtr ) method though. Maybe the template can define setValue( T ) with a non-compilable implementation and then we specialise for instantiations that make sense?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Pending Review
Development

Successfully merging this pull request may close these issues.

2 participants