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

Appleseed update pr #2980

Open
wants to merge 15 commits into
base: master
from

Conversation

Projects
None yet
3 participants
@est77
Copy link
Member

est77 commented Jan 15, 2019

Gafferseed update.
Depends on appleseed 2.0.5.

Changes:

  • Use appleseed shaders.
  • Adaptive sampler.
  • Added new AOVs.
  • Added new render settings.
  • Bug fixes.
  • More unit tests.
  • Updated BeginnerTutorial.

Dependencies

ImageEngine/cortex#903

Checklist

  • I have read the contribution guidelines.
  • I have updated the documentation, if applicable.
  • I have tested my change(s) in the test suite, and added new test cases where necessary.
  • My code follows the Gaffer project's prevailing coding style and conventions.

@est77 est77 force-pushed the appleseedhq:appleseed_update_PR branch from 8554e4c to 1f1a441 Jan 15, 2019

@medubelko medubelko self-requested a review Jan 16, 2019

@johnhaddon

This comment has been minimized.

Copy link
Member

johnhaddon commented Jan 16, 2019

Thanks Est! Just so you know, I'm not ignoring you, but I plan to make a 0.53 release before merging this, so this will go into 0.54. We have a few important features to get into production and I'd like to do that with as little upheaval as possible...

@medubelko

This comment has been minimized.

Copy link
Collaborator

medubelko commented Jan 16, 2019

@est77 It's failing the Travis build, and I get the same error on my local builds.

@est77

This comment has been minimized.

Copy link
Member Author

est77 commented Jan 16, 2019

Just so you know, I'm not ignoring you

No problem. It took me some time to finish in the first place.

@medubelko The build is expected to fail, because we need to update appleseed in the deps and merge my Cortex PR.

@johnhaddon

This comment has been minimized.

Copy link
Member

johnhaddon commented Feb 15, 2019

Do I need to update OSL to make this work? If so, what version? I'm having trouble getting the tests to pass at my end, with oslc failures when building the test shaders :

/disk1/john/dev/build/gaffer/appleseed/shaders/as_osl_extensions.h:58: error: Syntax error: syntax error
@est77

This comment has been minimized.

Copy link
Member Author

est77 commented Feb 15, 2019

Do I need to update OSL to make this work?

No. I am using OSL 1.9.8 here, 1 minor version less that the one bundled with Gaffer.
Which shader are you trying to compile? The error message is not very helpful.

@johnhaddon

This comment has been minimized.

Copy link
Member

johnhaddon commented Feb 15, 2019

Which shader are you trying to compile? The error message is not very helpful.

It's python/GafferAppleseedTest/shaders/constant.osl. First I had to fix AppleseedTest.compileOSLShader() because it was trying to use $APPLESEED/bin/oslc, which doesn't exist in Gaffer builds. Then I added #include "as_osl_extensions.h" to the shader, because oslc wasn't able to find as_npr_shading(). Then I got that syntax error. I can do more digging now I know I don't need to update OSL, but if you have any bright ideas do let me know...

@est77

This comment has been minimized.

Copy link
Member Author

est77 commented Feb 15, 2019

it was trying to use $APPLESEED/bin/oslc

Yes, I am trying to use the oslc inside appleseed/bin.
If you use it, it automatically includes the correct stdosl.h header and that header includes as_osl_extensions.h

One issue that I sometimes have with cmake is that when installing oslc it does not preserve the executable permission. But I don't think that is the problem.

@johnhaddon

This comment has been minimized.

Copy link
Member

johnhaddon commented Feb 15, 2019

Yes, I am trying to use the oslc inside appleseed/bin.

We don't include that with Gaffer - we just ship $GAFFER_ROOT/bin/oslc, which is a vanilla oslc.

If you use it, it automatically includes the correct stdosl.h header and that header includes as_osl_extensions.h

So $APPLESEED_ROOT/bin/oslc isn't vanilla oslc? I think that's problematic, as is modifying stdosl.h. OSL is an ecosystem with lots of different renderers, and in Gaffer we want to support them all, in one process, as well as allowing users to write and compile shaders on-the-fly in nodes like OSLCode. I believe the only way for this to work well is to use vanilla oslc and vanilla stdosl.h, and have users explicitly include things like as_osl_extensions.h when they want functionality specific to a particular renderer. The other benefit of that is that it makes it obvious when you are writing a shader which isn't portable between renderers. Is there a flaw in that approach?

@est77

This comment has been minimized.

Copy link
Member Author

est77 commented Feb 15, 2019

So $APPLESEED_ROOT/bin/oslc isn't vanilla oslc?

It is vanilla oslc. The only difference is that by using it you get the automatic include paths correct.

@johnhaddon

This comment has been minimized.

Copy link
Member

johnhaddon commented Feb 18, 2019

It is vanilla oslc. The only difference is that by using it you get the automatic include paths correct.

Ah, I see, because it looks for "../shaders/stdosl.h"? I stand by what I said about modifying "stdosl.h" though - it seems Arnold is doing it as well, which makes it impossible for us to find the "right" one when compiling OSLCode nodes. Can we sidestep the issue for now by avoiding Appleseed-specific closures in the test shaders?

@est77

This comment has been minimized.

Copy link
Member Author

est77 commented Feb 18, 2019

I stand by what I said about modifying "stdosl.h" though

As far as I know, all renderers modify stdosl.h. I guess part of it is offering users a simple include-less experience when writting shaders and removing closures you don't support and the other part is that
compatibility between renderers is not an explicit OSL goal.

Can we sidestep the issue for now by avoiding Appleseed-specific closures in the test shaders?

Yes. We can mark the test that uses the constant shader as an expected failure.

@johnhaddon

This comment has been minimized.

Copy link
Member

johnhaddon commented Feb 21, 2019

As far as I know, all renderers modify stdosl.h. I guess part of it is offering users a simple include-less experience when writting shaders and removing closures you don't support and the other part is that
compatibility between renderers is not an explicit OSL goal.

I can understand that from the point of view of a single renderer, but I think it is bad for the ecosystem as a whole. People move between renderers, and DCCs have to support multiple renderers, so avoiding obstacles to that is worthwhile. Cross-renderer-compatibility might not be an explicit goal for OSL, but it's certainly an attraction from a user point of view.

Yes. We can mark the test that uses the constant shader as an expected failure.

I was hoping we could just use a precompiled shader - perhaps the Surface/Constant shader that comes with Gaffer, or as_emission_surface? I don't really understand why we're using as_npr_shading() for the constant, and I'm reluctant to disable tests when we could really do with more...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment