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

Nuke: storing attributes in separated knobs with backward compatibility #438

Conversation

jakubjezek001
Copy link
Contributor

What's changed?

  • the way Avalon data are stored in nodes. Before it was text knob (attribute) with dictionary stored as toml dump, now each attribute has own knob which some of those are editable.
  • adding proper docstrigs to functions supporting Argument, Examples, etc
  • adding new asset attributes and ensuring backward compatibility on frameStart/End and resolutionWidth/Height
  • some minor avalon.nuke api code improvments

avalon/nuke/pipeline.py Outdated Show resolved Hide resolved
avalon/nuke/lib.py Outdated Show resolved Hide resolved
avalon/nuke/lib.py Outdated Show resolved Hide resolved
avalon/nuke/lib.py Outdated Show resolved Hide resolved
avalon/nuke/lib.py Outdated Show resolved Hide resolved
avalon/nuke/lib.py Outdated Show resolved Hide resolved
avalon/nuke/lib.py Outdated Show resolved Hide resolved
avalon/nuke/lib.py Outdated Show resolved Hide resolved
avalon/nuke/lib.py Outdated Show resolved Hide resolved
avalon/nuke/lib.py Outdated Show resolved Hide resolved
@jakubjezek001
Copy link
Contributor Author

have resolved all Hound-bot issues but it seems it is still not happy with something.. I am not sure what is the issue..

Copy link
Collaborator

@davidlatwe davidlatwe left a comment

Choose a reason for hiding this comment

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

Sorry for the delay, just spot a few more issues that might need to be discussed. :)

avalon/nuke/pipeline.py Outdated Show resolved Hide resolved
avalon/nuke/pipeline.py Show resolved Hide resolved
@BigRoy
Copy link
Collaborator

BigRoy commented Nov 4, 2019

@davidlatwe any status update on your end on testing this?

@jezscha according to Github there seems to be a conflict on avalon/nuke/pipeline.py - could you take a look?

@davidlatwe
Copy link
Collaborator

Just started an hour ago ! Still trying to get things to work, Avalon + Nuke day 1.

avalon/nuke/pipeline.py Outdated Show resolved Hide resolved
avalon/nuke/lib.py Outdated Show resolved Hide resolved
@jakubjezek001
Copy link
Contributor Author

@jezscha according to Github there seems to be a conflict on avalon/nuke/pipeline.py - could you take a look?

Hi @BigRoy I cannot see any other conflict. Where is it?

@davidlatwe
Copy link
Collaborator

Haha, I solved the conflict yesterday for testing on my end. 😬

@jakubjezek001
Copy link
Contributor Author

Haha, I solved the conflict yesterday for testing on my end. 😬

great! ;) thanks

@davidlatwe
Copy link
Collaborator

davidlatwe commented Nov 6, 2019

After take a deeper look into this PR and have a go on those new functions, although they could work but I think some of them are misplaced at the very first beginning.

I think get_handles and reset_frame_range_handles should NOT be in pipeline.py, instead they should be in command.py like the one in Maya, or in lib.py.

Because to me, pipeline.py should be containing something that responsible to build Avalon framework on top of Nuke, not something like component which can be used in multiple cases or able to be opt-out in production.

Next, lib.imprint shoud NOT be wrapping add_publish_knob and set_avalon_knob_data, should be in other way around instead.

Since the name "imprint", did not give a hint that it's working on a specific goal but to imprint any kind of data into node. But what has been implemented was only for writing Avalon related data at the end.

Now, I am not sure that we should merge this at the moment (sorry). I could, because at least I can confirm that nothing is broken here. ☺️

But we should not, because there are things we need to refactor in prior (those mentioned above). 🚧

What I have in mind for now is to start a couple of PRs for shipping concepts from this PR piece by piece. What do you guys think ?

davidlatwe added a commit to davidlatwe/core that referenced this pull request Nov 8, 2019
davidlatwe added a commit to davidlatwe/core that referenced this pull request Nov 8, 2019
avalon/nuke/pipeline.py Show resolved Hide resolved
avalon/nuke/pipeline.py Show resolved Hide resolved
avalon/nuke/pipeline.py Show resolved Hide resolved
avalon/nuke/lib.py Outdated Show resolved Hide resolved
@jakubjezek001
Copy link
Contributor Author

Hi David, thank you very much for your time to look into it! Please read bellow my comments.

I think get_handles and reset_frame_range_handles should NOT be in pipeline.py, instead they should be in command.py like the one in Maya, or in lib.py.

Good point, will move it into command.py

Because to me, pipeline.py should be containing something that responsible to build Avalon framework on top of Nuke, not something like component which can be used in multiple cases or able to be opt-out in production.

We will look into it with @mkolar and make it more consistent with Maya

Next, lib.imprint shoud NOT be wrapping add_publish_knob and set_avalon_knob_data, should be in other way around instead.

Yes. Also I am thinking to even move add_publish_knob out of lib.imprint, perhaps the function could be used from outside of imprint

Since the name "imprint", did not give a hint that it's working on a specific goal but to imprint any kind of data into node. But what has been implemented was only for writing Avalon related data at the end.

Not sure exactly what do you mean with: ut what has been implemented was only for writing Avalon related data at the end.

Now, I am not sure that we should merge this at the moment (sorry). I could, because at least I can confirm that nothing is broken here. ☺️
What I have in mind for now is to start a couple of PRs for shipping concepts from this PR piece by piece. What do you guys think ?

Again, we will look into it with @mkolar next week and for sure will split it into several PR and push them separately - this PR has got too big already.
If you have done something already let us know so we don't duplicate work to each other ;)

@davidlatwe
Copy link
Collaborator

If you have done something already let us know so we don't duplicate work to each other ;)

Thanks @jezscha !

I have pushed a PR for new imprint #473 , and submitted an issue for discussing Nuke containerizing #474 . Hope you guys could have a look. 😬

davidlatwe added a commit to davidlatwe/core that referenced this pull request Nov 20, 2019
davidlatwe added a commit to davidlatwe/core that referenced this pull request Dec 13, 2019
@davidlatwe davidlatwe mentioned this pull request Dec 13, 2019
@jakubjezek001
Copy link
Contributor Author

I guess this could be closed.. All of changes here were rewritten by @davidlatwe.

@mkolar mkolar deleted the cleanup/PYPE-493-nuke-pr-to-avalon-core2 branch January 26, 2021 08:54
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.

7 participants