[ShaderGraph] [bugfix 1283782] Fix for previews when selecting "None" as your texture property texture #2182
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
elizabeth-legros
approved these changes
Oct 12, 2020
ghost
approved these changes
Oct 12, 2020
|
Yamato green except VFX_HDRP Win DX11, which is also failing on master |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Purpose of this PR
Fix for https://fogbugz.unity3d.com/f/cases/1283782/
The core of the issue is that we cannot remove a Texture (or any property really) from a MaterialPropertyBlock, and we also cannot set the Texture to null (error). So effectively we cannot go back to the unassigned Texture state that would allow us to automatically pick up the default texture values.
As a workaround, we now pass the defaults into the preview property for texture2Ds, so we can properly assign the texture in the MaterialPropertyBlock with the corresponding value.
Talking with Yao, it should be possible to actually make public RemoveProperty() functions on MaterialPropertyBlock, which would greatly simplify all of this, but that would be a feature in trunk, so not until 21.1 at the earliest.
Testing status
Tested that all four default types show up properly in previews, and in the main preview, and match the color in scene.
Tested that assigning default types works with undo/redo/assembly reload, and that the previews update correctly.
Tested that assigning and unassigning (set to none) textures still works properly, with undo/redo/assembly reload and preview update.
Tested that non-2D texture types (2DArray, 3D, cubemap) still work as expected.
Comments to reviewers
Notes for the reviewers you have assigned.