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

Support for integer/data textures in WebGL2 and WebGPU #5921

Merged
merged 46 commits into from
Jan 19, 2024

Conversation

liamdon
Copy link
Contributor

@liamdon liamdon commented Dec 28, 2023

Add support for many of the signed/unsigned integer formats supported by WebGL 2.

Integer textures are very useful for emulating compute shaders in WebGL using data textures, voxel rendering and more.

With that said, new formats do require a few lines of repetitive definition code - 573 net lines here. I won't be offended if you decide that the potential use-cases are too niche to justify the added weight.

Happy Holidays!

I confirm I have read the contributing guidelines and signed the Contributor License Agreement.

Example:

CleanShot.2024-01-15.at.08.47.45_1_2.mp4

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.

Impressive PR - thanks for all the effort on this! 🙏 LGTM, but deferring to @mvaligursky for final approval before we can merge.

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.

Great PR, but I left a bunch of comments / fixes / suggestions before this can be merged.

@mvaligursky
Copy link
Contributor

Just to add here - I'd love to have an engine example here showing the use of some of these. Any thoughts on this @liamdon ?

@liamdon
Copy link
Contributor Author

liamdon commented Jan 11, 2024

Thank you for all the comments @mvaligursky! I'll address the issues above.

Regarding the engine example: yes, I thought about doing this but wanted to make sure you were OK with the idea before investing time in an example. I'll add an example to this PR, but likely over the weekend.

@liamdon
Copy link
Contributor Author

liamdon commented Jan 16, 2024

@mvaligursky addressed all your comments above - I really appreciate the rounds of review on this, the PR is much better now!

Want to make sure you saw my note on comparison here. I think we should leave this as-is and potentially change it if we discover that we're setting up our samplers incorrectly elsewhere. For now, we are definitely covering the most common use-case of using texelFetch with an int or uint texture.

@mvaligursky
Copy link
Contributor

Want to make sure you saw my note on comparison here.

I'm not sure you added the note. Perhaps it'd be the best to add a comment in the code to explain.

liamdon and others added 2 commits January 17, 2024 07:47
Co-authored-by: Will Eastcott <will@playcanvas.com>
Co-authored-by: Will Eastcott <will@playcanvas.com>
liamdon and others added 2 commits January 17, 2024 07:49
Co-authored-by: Will Eastcott <will@playcanvas.com>
Co-authored-by: Will Eastcott <will@playcanvas.com>
@willeastcott
Copy link
Contributor

Sorry for the flurry of edits/suggestions. We generally try to keep JSDoc comments to 100 columns....

@liamdon
Copy link
Contributor Author

liamdon commented Jan 17, 2024

@willeastcott No problem! Do you use a formatter or IDE extension that flags those for you, or just eyeballing?

@willeastcott
Copy link
Contributor

Just eyeballed it. 😄

@mvaligursky
Copy link
Contributor

in VisualStudio code, you can add a ruler to draw a line at column 100. Go to settings, search for rulers, Edit .. and add something like this

    "editor.rulers": [
        {
          "column": 100,
          "color": "#693d69"
        }
      ]

@liamdon
Copy link
Contributor Author

liamdon commented Jan 17, 2024

Awesome, just enabled that. There's always a new VSCode feature to discover!

I realized that my earlier replies/comments to you were not submitted @mvaligursky, I did not toggle on the Github review mode 🤦‍♂️
Here is that last comment I was talking about, but I also added similar comment in the code to clarify the use of 'comparison'. After we've merged this PR, I will experiment with using/supporting other types of samplers with integer textures. But, I think the PR as-is will cover the most common way they are used.

Let me know if I missed anything else, otherwise I think we are good to go!

@mvaligursky
Copy link
Contributor

The only remaining issue for me is to change the pixel format 'UI to U, to keep the type a single letter.
We already have F for float (PIXELFORMAT_RGB16F), we're adding I for int types, and I'd prefer U for uint types instead of UI.
You mention we can later rename those .. but lets avoid that, as deprecating them and having new names means the engine needs to support both, and it creates a possibly more work to fix examples, Editor projects and similar.

Single letter type is something I'm already using for uniform types.

Naming compatibility with WebGL naming is not very relevant in our API.

@liamdon
Copy link
Contributor Author

liamdon commented Jan 18, 2024

Done - was not a strong opinion against that from me, but I wanted to present the 'WebGL' reasoning in case it was important. We are now using a single character for all the type suffixes.

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.

Fantastic PR @liamdon , thanks for the contribution!

@mvaligursky mvaligursky merged commit 7283e5a into playcanvas:main Jan 19, 2024
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants