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

Reintroduce support for chunk $texture2DSAMPLE #4431

Merged
merged 7 commits into from
Jul 14, 2022

Conversation

slimbuck
Copy link
Member

We recently removed support for $texture2DSAMPLE(...) macro in shaders since it's completely unnecessary ( #4405). The way to do this now is: $DECODE(texture2D(...)).

However, almost all overridden lightmap and emissive chunks use this macro and so this PR re-introduces the macro to remain backwards compatible.

In the process, we also shuffle gammaCorrect function to use the already-available decodeXXX functions instead of reimplementing them.

@slimbuck slimbuck added enhancement area: graphics Graphics related issue labels Jul 14, 2022
@slimbuck slimbuck requested a review from a team July 14, 2022 10:16
@slimbuck slimbuck self-assigned this Jul 14, 2022
return pow(color, vec3(0.45));
#endif
#else
return pow(color + 0.0000001, vec3(1.0 / 2.2));
Copy link
Contributor

Choose a reason for hiding this comment

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

doesn't need to be vec3(0.0000001) ?

Copy link
Member Author

Choose a reason for hiding this comment

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

nope this works. the float is added to vector components.


// continue to support $texture2DSAMPLE
if (subCode.indexOf('$texture2DSAMPLE')) {
const decodeTable = {
Copy link
Contributor

Choose a reason for hiding this comment

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

keep this object at the global scope?

Copy link
Member Author

Choose a reason for hiding this comment

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

I am convinced the compiler does not construct/allocate this object every time the function is called (but rather hoists it globally or something similar - equivalent to c++ const block). Having it declared here where it is used is much clearer for the reader.

Now just need to find some confirmation of my conviction on the webs...

@@ -371,6 +383,14 @@ const standard = {
code.append(this._addMap("light", lightmapChunkPropName, options, litShader.chunks, options.lightMapEncoding));
func.append("getLightMap();");
}

// only add the legacy chunk if it's referenced
if (code.code.indexOf('texture2DSRGB') !== -1 ||
Copy link
Contributor

Choose a reason for hiding this comment

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

could this be tracked differently than using .indexOf?
Likely not I assume, as any chunk can just call this directly.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah this is no nasty :`(

Alternatively since only emissive and lightmap chunks do this, we could somehow pass flags around tracking the use of these functions instead. This code is just so much simpler.

Copy link
Contributor

@mvaligursky mvaligursky left a comment

Choose a reason for hiding this comment

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

approving with some minor comments

@slimbuck slimbuck merged commit 8869c36 into playcanvas:main Jul 14, 2022
@slimbuck slimbuck deleted the texture-sample branch July 14, 2022 14:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: graphics Graphics related issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants