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.

Sign up for GitHub

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

Jump to bottom

Encoding cleanup #4405

Merged
merged 8 commits into from
Jul 8, 2022
Merged

Encoding cleanup #4405

merged 8 commits into from
Jul 8, 2022

Conversation

slimbuck
Copy link
Member

@slimbuck slimbuck commented Jul 7, 2022

This PR:

  • simplifies handling of texture encoding logic in the standard material
  • splits out encode, decode and spherical helper chunks
  • removes the hideous texture2DRGBM shader functions
  • skybox can now render any of the supported encodings

@slimbuck slimbuck added enhancement area: graphics Graphics related issue labels Jul 7, 2022
@slimbuck slimbuck requested a review from a team July 7, 2022 10:47
@slimbuck slimbuck self-assigned this Jul 7, 2022
willeastcott
willeastcott approved these changes Jul 7, 2022
View reviewed changes
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.

Really nice refactor - cool!

mvaligursky
mvaligursky reviewed Jul 8, 2022
View reviewed changes
src/graphics/program-lib/chunks/common/frag/envAtlas.js
@@ -0,0 +1,23 @@
export default /* glsl */`
// the envAtlas is fixed at 512 pixels. every equirect is generated with 1 pixel boundary.
const float atlasSize = 512.0;
Copy link
Contributor

Choose a reason for hiding this comment

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

not related to this PR, but don't we handle larger atlases now?

Copy link
Member Author

Choose a reason for hiding this comment

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

We do, but actually still layout them out as if they were 512.

mvaligursky
mvaligursky approved these changes Jul 8, 2022
View reviewed changes
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.

looks great. any problem with changed options - I suspect the old options would not be commonly used?

@slimbuck
Copy link
Member Author

slimbuck commented Jul 8, 2022

looks great. any problem with changed options - I suspect the old options would not be commonly used?

The StandardMaterialOptionsBuilder options? They should never be stored offline, completely dynamic. It may be that some users are doing things based on these, but I should hope not!

@slimbuck slimbuck merged commit 6127eee into playcanvas:main Jul 8, 2022
@slimbuck slimbuck deleted the encoding-cleanup branch July 8, 2022 15:16
This was referenced Jul 12, 2022
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@willeastcott willeastcott willeastcott approved these changes

@mvaligursky mvaligursky mvaligursky approved these changes

Assignees

@slimbuck slimbuck

Labels
area: graphics Graphics related issue
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@slimbuck @willeastcott @mvaligursky