Autodesk / sitoa

Arnold plugin for Softimage
Apache License 2.0
33 stars 16 forks source link

SItoA for Arnold 5.2 to master #29

Closed JenusL closed 5 years ago

JenusL commented 5 years ago

I must say that it has been very fun to code for this project so far. The code is so nice and structured that it has been a breeze to navigate and read. Now after #28 has been merged into develop I feel that it's time to merge this into master and make a real release. Or should we include something more?

Before we merge, lets discuss how we should deal with versioning and branching going forward. I changed the versioning to match Arnold version because that just makes more sense to me. But that goes away from the old versioning. How do you guys feel? Last version of SItoA was 4.1. Is it ok to jump to 5.2 and sync with the Arnold version or should we name this 4.2 and stick with the old versioning that's unrelated to Arnold version?

This goes together with branching a bit. If we should remove the develop branch and always PR to master, we would probably like to version up on every PR whether it's a new feature or a fix. That can end up being a lot of versions. I think that means someone at AD needs to be responsible for the versioning because it probably needs to be done as a separate commit directly to the branch after a batch of PRs has been merged. What do you think?

caron commented 5 years ago

@JenusL , I think you have done a great job! I feel the plugin is in a useful state even if incomplete compared to Arnold core features. I won't object if @sjannuz and @kikou decide its time to 'release' it. BUT I would say let's get some effort into making sure noice denoising is usable (variance aovs and filter setups). Supporting Optix, operators, and texture baking are going to take some effort so I wouldn't want it to block people using the latest Arnold core on those features.

Regarding versioning, I also don't particularly care. Typing out loud here but I can see it go either way. Maybe moving to the sync'd SItoA and Arnold core versions can also signal the new open source era which probably will only get updated when a new core version comes out anyways. BUT then that would mean we would need to release new features like Optix denoising, operators, etc. by incrementing 'fix version' and not the 'minor version' number. Sooo could be easier to keep them unrelated...

Lastly, regarding the branches I have no problem doing it like we have been doing it, staging changes in develop and only merging with master when everything checks out but its really up to the guys. Of course we should be tagging the master branch with whatever version we are calling this.

JenusL commented 5 years ago

I have no problem waiting for noice (#14) so lets do that and keep this PR open until then.

Regarding versioning, I also don't particularly care. Typing out loud here but I can see it go either way. Maybe moving to the sync'd SItoA and Arnold core versions can also signal the new open source era which probably will only get updated when a new core version comes out anyways. BUT then that would mean we would need to release new features like Optix denoising, operators, etc. by incrementing 'fix version' and not the 'minor version' number. Sooo could be easier to keep them unrelated…

This is exactly how I feel as well. While it makes sense to follow Arnold versioning it might be easier to keep them unrelated. I think all the other plugins are unrelated.

I also think this type of branching with the develop branch has worked well for this type of project. I only ask because there was some questioning about it when we first started and I just want to check so that everybody think its been working well. If we were to change to a more Github Flow kind of dev working only with a master branch (Continous Integration) we probably need a different version scheme like MAXtoA has. I think that has MAJOR.MINOR.BUILD since the last two versions we used of MAXtoA is 2.0.938 and 2.1.945. This only complicates things in my opinion.

After this thought experiment I think the best and simplest thing is to go back to the old SItoA versioning and keep develop branch and just continue as we have done.

sjannuz commented 5 years ago

I like the idea of matching the Arnold version. We can probably modify the packager so that the xsiaddon name includes all the 4 digits. About the branching strategy, I also like what we're doing: use develop for the work in progress, and merge it into master at release time. It's a clean way to work for a small team, I think. And yes, great job, @JenusL . Are you doing this for your personal "fun", or for production porposes ?

JenusL commented 5 years ago

So if we want to match Arnold version, maybe we can du a hybrid? We could add a digit and make the first two digits be the first two of Arnold, and the last two can be feature and fix for SItoA. In that case this would be SItoA 5.2.0.0 If there's a need for a quick fix it would be called 5.2.0.1 and if we add non-breaking features to SItoA we make that 5.2.1.0. Breaking features would only be allowed on core updates. Would that make sense?

Ok let's keep the current branching strategy :)

Are you doing this for your personal "fun", or for production porposes ?

Well both to be honest :) I thought I would give it a try and then I found it to be way faster to work with and more fun than expected. I also like the idea that when I need Softimage in production, which happens from time to time, SItoA is up to date and ready for (almost) anything I throw at it.

sjannuz commented 5 years ago

Would that make sense?

The release cycle of SItoA will be always (I think) slower than the Arnold one, that releases about once a month. So, I think that using the plain Arnold digits will be work. Of course, we must be sure to always use the most recent Arnold version, when releasing SItoA.

JenusL commented 5 years ago

Yeah but If we need to create bugfixes just for SItoA, how would we version those? Maybe we just stick to how it is now. v5.2.0 5.2 is Arnold and the last digit is plugin fixes (and maybe small features but nothing breaking). The last two digits in Arnolds version is very rare to add or brake stuff and many times in the past (in the middle of production) I've just replaced ai.dll to get those fixes, and sometimes, features.

caron commented 5 years ago

I am leaning more to just independent versioning, regardless if the next release gets a version bump to match current core version.

sjannuz commented 5 years ago

So, let's go with that, @JenusL . Use the Arnold's first 2 digits (5.2 right now) and the third for SItoA. So we're also partially independent, as @caron suggests. Arnold 5.2.0.1 is scheduled for next week, and I'll be on vacation until the 17. So, 'let's plan the merge and release for then. Sounds good ?

JenusL commented 5 years ago

Ok that sounds good to me.

sjannuz commented 5 years ago

@JenusL I am back. Have you upgraded to Arnold-5.2.0.1 already ?

sjannuz commented 5 years ago

@JenusL I am back. Have you upgraded to Arnold-5.2.0.1 already ?

Yes, I see you did. So, ready to go master ?

JenusL commented 5 years ago

@sjannuz I'm testing out the last PR about the pass shaders and I found some issues. Let's fix that first. I'll create a Issue soon.

caron commented 5 years ago

BTW, I no longer think we need to hold up merging to master for the 'output denoising aovs' #34. As long as master builds and passes most tests I think its worth merging.

sjannuz commented 5 years ago

Arnold-5.2.1.0 is coming in a few days, including the round corners shader. There are no other major improvements. I'd say to wait for it, and finally release.

JenusL commented 5 years ago

Ok cool! Give me a few days after the release of 5.2.1 then to add the round corners shader as well.

JenusL commented 5 years ago

@sjannuz Any chance you guys have a little more time for SItoA?

sjannuz commented 5 years ago

You are absolutely right, I'll find some time this week.

JenusL commented 5 years ago

Would be nice to have support for the Arnold Denoiser as well before merging. @sjannuz When's a new core coming out? I saw at least some hair anisotropy stuff has been added to MtoA for it.

sjannuz commented 5 years ago

Very very shortly. Yes, there are 2 extra parameters in standard_hair, and a different default for round_corners. Nothing else to be exposed.

JenusL commented 5 years ago

@sjannuz When you have merged the remaining open PRs I feel it's time to go master. What do you think?

sjannuz commented 5 years ago

Hi @JenusL , sure, I just need a couple of hours to dedicate to this. Thankyou again

JenusL commented 5 years ago

@sjannuz @kikou Think u got time this week?

JenusL commented 5 years ago

Thank you so much @sjannuz for taking the time to finally go master! Do you think you can find the time to create a release with a xsiaddon for both Win and Linux as well?

sjannuz commented 5 years ago

Hi @JenusL , I should have some time before the end of this week. Thankyou for all your work.

sjannuz commented 5 years ago

52a0a4b:

sjannuz commented 5 years ago

I have the addons ready, built against 5.2.2.1. I've packed different addons for Windows and Linux, since each is now about 370M. Any volunteer to test the Linux one ?

JenusL commented 5 years ago

Awesome! I reached out to si-community to see if anyone can test on Linux. I will reach out to some friends tomorrow morning as well.

JenusL commented 5 years ago

I wrote down a changelog as well if you want to use it for the release: https://gist.github.com/JenusL/a8ac760ebd867356035f5e2913990254