Examples: use .texture of input render targets. #8787
Conversation
| case 'RGBM16': newEnvMap = rgbmCubeRenderTarget; break; | ||
| case 'LDR': newEnvMap = ldrCubeRenderTarget ? ldrCubeRenderTarget.texture : undefined; break; | ||
| case 'HDR': newEnvMap = hdrCubeRenderTarget ? hdrCubeRenderTarget.texture : undefined; break; | ||
| case 'RGBM16': newEnvMap = rgbmCubeRenderTarget ? rgbmCubeRenderTarget.texture : undefined; break; |
There was a problem hiding this comment.
I would try to never assign undefined. Either use null or use if and don't assign at all (I guess not possible here).
There was a problem hiding this comment.
I usually avoid assigning undefined, too. When I was refactoring this example a couple of weeks ago, I initially did assign null, but then the envMap didn't get rendered. I now tried assigning null again and it works just fine. I think that the issue I encountered back then must have had to do with the PMREM files.
Anyway, I went a little bit further while adjusting this example and moved the logic out of the render loop. As you can see here, the dat.GUI parameters are evaluated in there. Would you prefer a solution that avoids that or do you like the current setup (just with null instead of undefined)?
There was a problem hiding this comment.
When it's tidying / shows better practice, I guess it's fine. Where's the code?
Just forward-push it in a separate commit? Then you can easily take it out again via git reset HEAD~1 (or git rebase -i <base commit> if it's not the last - I recently wrote a Wiki page that explains changing the history in a little more detail).
There was a problem hiding this comment.
Oh, I didn't know this was possible inside of PRs, but I guess it makes sense! I've read your Git-guide and it helped me make my first feature branch for this PR :D
There was a problem hiding this comment.
Oh, I didn't know this was possible inside of PRs
Moving your branches' HEADs "randomly" is especially useful in PRs! Same happens when you rebase. We wouldn't want Ricardo to do it with dev, though - it'd be trouble :)
|
Other than that looks good to me. Did you test all the examples you edited? |
Yes, I checked every single example first to find the ones that used render target inputs. Then I applied the changes individually and checked them again. I also encountered some examples that were already broken. For example, the Sky shader example fails to run because the GridHelper doesn't have public color properties anymore. But that's out of the scope of this PR. |
| case 'HDR': newEnvMap = hdrCubeRenderTarget.texture; break; | ||
| case 'RGBM16': newEnvMap = rgbmCubeRenderTarget.texture; break; | ||
|
|
||
| } |
There was a problem hiding this comment.
I messed up here: it's still necessary to check if the *CubeRenderTarget variables are undefined. Otherwise, selecting a different envMap in the GUI while textures are still loading would result in an error.
There was a problem hiding this comment.
The updateEnvMap refactor is slightly tidying, but, hey, it's just an example. I'd just take the commit out and replace undefined with null. Then let's merge it :).
There was a problem hiding this comment.
Yea, I agree. I'll try that now.
Good!
Yes.
Oh! Your |
|
Looks like it worked :) Thanks for the help! |
|
Thanks thanks! |
* Examples: use .texture of input render targets. * Assign null instead of undefined.
Updated examples according to the changes in #8658.