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

Add shader textureBias #4088

Merged
merged 16 commits into from
Mar 8, 2022
Merged

Add shader textureBias #4088

merged 16 commits into from
Mar 8, 2022

Conversation

slimbuck
Copy link
Member

@slimbuck slimbuck commented Mar 6, 2022

This PR:

  • adds a global textureBias uniform to control biased texture reads across standard material textures
  • enables WEBGL_depth_texture extension on webgl1 devices (used when creating texture-based render targets)
  • updates example package-lock file to version 2

These changes aid with the implementation of a high quality, multi-frame rendering solution.

Questions:

  1. the biased version of texture2D is supported out-of-the-box on webgl 1.0, but is there any overhead calling this version of the function when bias is 0? (I expect probably not, but how do we check?)

Answers:

  1. likely no performance impact

@slimbuck slimbuck added the area: graphics Graphics related issue label Mar 6, 2022
@slimbuck slimbuck requested a review from a team March 6, 2022 16:21
@slimbuck slimbuck self-assigned this Mar 6, 2022
@willeastcott
Copy link
Contributor

willeastcott commented Mar 8, 2022

we don't want GrabPass to become official public API yet: do we flag the class as @private or @ignore?

Use @ignore.

@private is just for class fields and methods and not classes themselves.

@slimbuck slimbuck changed the title [DRAFT] Support multiframe rendering Add shader globalTextureBias Mar 8, 2022
@willeastcott
Copy link
Contributor

Did you mean to update pacakge-lock.json in this PR?

@slimbuck
Copy link
Member Author

slimbuck commented Mar 8, 2022

Did you mean to update pacakge-lock.json in this PR?

Yes! but forgot to add to the PR description.

@slimbuck slimbuck marked this pull request as ready for review March 8, 2022 14:51
Copy link
Contributor

@willeastcott willeastcott left a comment

Choose a reason for hiding this comment

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

Awesome. Maybe edit the PR title tho.

@mvaligursky
Copy link
Contributor

related to #2856

@@ -568,6 +568,9 @@ class WebglGraphicsDevice extends GraphicsDevice {
}

this.constantTexSource = this.scope.resolve("source");
this.textureBias = this.scope.resolve("textureBias");

this.textureBias.setValue(0.0);
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should have a public API for this. Ideally the viewer code you test it on would not do
this.scope.resolve("textureBias").SetValue(...);

Copy link
Contributor

Choose a reason for hiding this comment

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

This transform feedback example already uses this API. This uniform management is long overdue a redesign.

Copy link
Contributor

Choose a reason for hiding this comment

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

That's true, but transform feedback example is accessing this API to set uniforms in its own shader.
But here we're adding new functionality to the engine that the Viewer wants to use .. and that should be different API, allowing us to even rename the variable later if needed. Maybe `Scene.standardMaterialTextureBias' or a shorter version of it? Something we publicly document.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd prefer something like Scene#textureBias and then potentially add Texture#bias which would take precedence.

Copy link
Member Author

Choose a reason for hiding this comment

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

Could be a scene setting... though I would prefer creating and exposing public API in followup PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

Fine by me.

@slimbuck slimbuck changed the title Add shader globalTextureBias Add shader textureBias Mar 8, 2022
@slimbuck slimbuck merged commit e4b734d into playcanvas:dev Mar 8, 2022
@slimbuck slimbuck deleted the multiframe branch March 8, 2022 17:24
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