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

Refining how lights work in hdCycles #49

Merged
merged 2 commits into from
Jan 28, 2021

Conversation

boberfly
Copy link
Contributor

  • Parameter changes on-sync will re-create a new shader graph and be applied via set_graph() calls
  • As a result, removed excess shader member pointers they are not needed & prevents crashes
  • Added support for textures on rect/quad lights (TODO: need to check if texture co-ords are consistent with other delegates)
  • Fixed HDRI co-ordinates on dome/background lights (matches Houdini GL)

This was referenced Jan 22, 2021
@boberfly boberfly requested a review from Vochsel January 22, 2021 02:39
- Parameter changes on-sync will re-create a new shader graph and be applied via set_graph() calls
- As a result, removed excess shader member pointers they are not needed & prevents crashes
- Added support for textures on rect/quad lights (TODO: need to check if texture co-ords are consistent with other delegates)
- Fixed HDRI co-ordinates on dome/background lights (matches Houdini GL)
@boberfly boberfly force-pushed the lighting_improvements branch from 27da6d8 to ecc9f33 Compare January 22, 2021 18:50
Copy link
Contributor

@Vochsel Vochsel left a comment

Choose a reason for hiding this comment

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

Built locally and works great. If possible an environment variable for up axis with the default to Z (Blender) would be the best course of action. Internally we can set to Y, however I think it's best to keep the default behaviour of this repo to match Blender USD exports. I can't find any way to get the up axis from Hydra, so env var is probably the only way to go.

The default hydra behaviour with "Default dome lights" seems to be that with no lights present, a gray dome light is used, but once any light (dome or otherwise) is added, the default light is removed. We partially had this functionality before (I think it broke some time ago, and now a dome light is required if you want to disable it). Not sure if you have the time to check this in this PR, perhaps we can create another PR soon to fix this.

Changes:

  • ENV var toggle for up axis behaviour
  • Maybe fix the default dome light not being removed (Optional)

Thanks for your work, really appreciated. Cheers!

plugin/hdCycles/light.cpp Outdated Show resolved Hide resolved
plugin/hdCycles/light.cpp Outdated Show resolved Hide resolved
plugin/hdCycles/light.cpp Outdated Show resolved Hide resolved
@Vochsel Vochsel added this to the 0.8.4 milestone Jan 24, 2021
@Vochsel Vochsel linked an issue Jan 25, 2021 that may be closed by this pull request
Copy link
Contributor

@Vochsel Vochsel left a comment

Choose a reason for hiding this comment

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

Looks solid, thanks for that up axis change.

- HD_CYCLES_UP_AXIS is an environment variable, renderParam now has a GetUpAxis() function call so code can check against (only dome light orientation is using this for now)
- Addressed PR notes eg. check string length instead
- The code will only rebuild the graph if any combination of IES/texture/temperature is set or changed

Misc features/fixes/changes made:
- Added IES light support, probably needs a little testing but it looked "correct", normalize/scale isn't supported yet
- Dome light with temperature now working also
- Background/dome transform node will now be located from the graph and tweaked and not cached from a pointer
@boberfly boberfly force-pushed the lighting_improvements branch from 5a3e45a to 2e6db72 Compare January 26, 2021 23:40
@Vochsel Vochsel merged commit d34fca2 into tangent-opensource:main Jan 28, 2021
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.

HDR image offset
2 participants