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

[WebGL2] Texture Internal Format discussion #14625

Closed
DavidPeicho opened this issue Aug 2, 2018 · 13 comments
Closed

[WebGL2] Texture Internal Format discussion #14625

DavidPeicho opened this issue Aug 2, 2018 · 13 comments

Comments

@DavidPeicho
Copy link
Contributor

I would like to open a discussion about internal format, and how we could integrate them without breaking the current Three.js API.

So, right now, the internal format is derived from the format, like this.

What I did on my side, is to add an attribute inside the Texture class, which represents the internal format, but is not given through the constructor, but rather explicitely:

const texture = new THREE.DataTexture(data, width, height);
texture.internalFormat = THREE.RGBA8UIFormat;

I also added support for unsgined integer sampler, etc... And everything seems to work great.

What do you think about this change? I agree it's not the best regarding the API, but at least it backward compatible with WebGL1.

@takahirox
Copy link
Collaborator

I'm thinking of adding internalFormat constructor parameter

function Texture( image, mapping, wrapS, wrapT, magFilter, minFilter, format, type, anisotropy, encoding, internalFormat ) {

@DavidPeicho
Copy link
Contributor Author

That works the same, so I am fine with it!

If you want I can separate my code to a commit that handle that, I already changed WebGLTextures.

@takahirox
Copy link
Collaborator

Nice, PR is welcome.

@twilson7755
Copy link

Continuing texture discussion from this comment in #13717 ... @yoshikiohshima the desire for utilizing R16UI and R16I texture formats is that most medical images (CT, MR, X-Ray, etc) are signed/unsigned 16 bit images (well actually they are anywhere from 10-16 bits but required 2 bytes of data per pixel). Hence I would like to pass in the images to my shader for rendering using these texture formats. Currently I encode the 2 bytes of data into 2 of the channels in an RGB texture format and then reconstruct the 16 bit values in the shader. This works fine but it would be nice to reduce my texture memory footprint by 1/3 by utilizing these 2 byte texture formats directly.

@DavidPeicho
Copy link
Contributor Author

Did not have time to work on it sorry, I was attending the SIGGRAPH. I implemented that on my own fork already, I just have to separate the work to make a PR on three.js, not the most fun work but I have to do it.
For medical images it is indeed needed, not only for the memory footprint, but also because packing induces some imprecision.

@mrdoob
Copy link
Owner

mrdoob commented Aug 25, 2018

Instead of adding another parameter to the constructor, I would add a new method:

texture.setInternalFormat( THREE.RGBA8UIFormat );

@mrdoob mrdoob added this to the r97 milestone Aug 25, 2018
@takahirox
Copy link
Collaborator

Because we lean toward .setXXX() method from constructor parameters? #14726 (comment)

@mrdoob
Copy link
Owner

mrdoob commented Aug 26, 2018

Yeah. That kind of API tends to be more resilient to API changes.

@takahirox
Copy link
Collaborator

I see.

Until we move to such style APIs, only custom internal format needs to be setup via .setXXX() method, not via constructor. But it may be ok, I guess not so many users wanna set internal format by themselves. It may be acceptable.

@takahirox
Copy link
Collaborator

Any updates? @DavidPeicho

@mrdoob mrdoob modified the milestones: r97, r98 Sep 25, 2018
@DavidPeicho
Copy link
Contributor Author

Sorry, I was in long holidays. It will be done this week end. @takahirox

@mrdoob mrdoob modified the milestones: r98, r99 Oct 31, 2018
@mrdoob mrdoob modified the milestones: r99, r100 Nov 29, 2018
@mrdoob mrdoob modified the milestones: r100, r101 Dec 28, 2018
@twilson7755
Copy link

Hey guys, just wondering if there is anything happening in regards to this issue ... we would really like to start taking advantage of this capability once implemented ... thx?

@DavidPeicho
Copy link
Contributor Author

DavidPeicho commented Jan 16, 2019

Hey, I added the internal format support in a PR. You can follow the status here: #15121

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants