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

Refactor 'targets' internal control #351

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

davidlatwe
Copy link
Collaborator

Problem 1

The plugins was discover in parent-process (server side), and formatted into JSON then send to child-process (client side), next, the plugins_by_targets logic was handled there.

So say we have 10 plugins total, 5 for target "A" and the other 5 for target "B". In current data workflow described above, there's high chance that server is sending all 10 plugins into client and filter out 5 plugins which won't needed. Which I think can be improved.

Proposal 1

Moving the target filtering process from client to server.


Problem 2

In #335, @BigRoy mentioned:

Also note: pyblish.api.register_target does not propagate to an open Pyblish QML session.

The live QML session does not whatsoever update to any registered targets
...
I think this might be a deliberate design choice so the explicitly defined target on launch does not require the targets to be globally registered. As such, you can target the specific Pyblish QML session without changing the environment which is useful.

In this design, in other words, you can not trust pyblish.api.registered_targets inside plugin which runs in pyblish-qml.

Proposal 2

Implement targets query function into pyblish_qml.api.

  • pyblish_qml.api.current_targets

And with proposal 1 implemented, we can spend no more server-client communication for querying current targets.


Extra Proposal

I often used pyblish_qml.api.current_server().service._context to get the pyblish.api.Context object in pyblish_qml session.

So I propose to implement a convenient function for that propose as well.

  • pyblish_qml.api.current_context

@davidlatwe
Copy link
Collaborator Author

I think this PR also resolved @mottosso 's little worry in #347. 😉

@davidlatwe
Copy link
Collaborator Author

I think I need to explain my motivation on proposal 2 a little :

One of my plugins was checking on current targets so to filter out instances in working scene by family, before instance is created.

For example, I have a context plugin which responsible to create instances and runs in all targets, and if the current target is "remote publish", then I don't want instance which it's family is like model or rig being created at first place. So I would need to firstly checking on current targets, then avoid those families instances.

@tokejepsen
Copy link
Member

For example, I have a context plugin which responsible to create instances and runs in all targets, and if the current target is "remote publish", then I don't want instance which it's family is like model or rig being created at first place. So I would need to firstly checking on current targets, then avoid those families instances.

Could you do the same workflow but just inherit a plugin class and changing the target?
I may not see the use-case correctly.

@davidlatwe
Copy link
Collaborator Author

Could you do the same workflow but just inherit a plugin class and changing the target?

Yes, it still can be done with current mechanism, but have to write a bit more code.

Anyway, the example above was not the main problem, it's not a "must-solve" issue, but could solve better if proposal 1 has implemented. So the point still is the Problem 1.

Also note that, this PR will NOT change any current features, only tweaked the internal data flow.

Copy link
Member

@BigRoy BigRoy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I accidentally approved this PR which I haven't actually checked thoroughly. I meant to approve another one.

Seems a merge conflict must be resolved @davidlatwe

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

Successfully merging this pull request may close these issues.

3 participants