Skip to content

Conversation

@DavidPeicho
Copy link
Contributor

@DavidPeicho DavidPeicho commented Oct 24, 2018

  • Adds internalFormat attribute to Texture Object;
  • Adds internalFormat copy to Texture.copy();
  • Adds internal formats to constants.js;
  • Adds internal formats conversion to WebGLUtils;
  • Adds singed / unsigned SAMPLER_3D, and SAMPLER_2D_ARRAY to WebGLUniforms

Sorry if sometimes the name are not consistant between each internal format. Just tell me what you like the most between R16FFormat and R16FloatFormat for instance, it is quite quick to change.

@takahirox sorry for the time I took for that. My own fork already had this feature, and my work was a priority.

@mrdoob mrdoob added this to the r99 milestone Oct 24, 2018
@DavidPeicho
Copy link
Contributor Author

DavidPeicho commented Oct 25, 2018

This was a request made by @mrdoob, as you commented:

I do not think we need this method. I disagree with @mrdoob.

Whenever everyone will agree on this I can make the change, meaning deleting it if needed.

@mrdoob mrdoob modified the milestones: r99, r100 Nov 29, 2018
@DavidPeicho
Copy link
Contributor Author

It has been a long time. I am going to fix the linting.
Regarding the setter, I am going to remove it and use the public attribute. If this causes trouble we can still talk about that in another PR as @WestLangley said.

@mrdoob mrdoob modified the milestones: r100, r101 Dec 31, 2018
@DavidPeicho DavidPeicho force-pushed the feature/texture-internal-format branch 2 times, most recently from 6a9c62f to 38ccba8 Compare January 10, 2019 18:56
@mrdoob mrdoob modified the milestones: r101, r102 Jan 31, 2019
@ixcviw7bw
Copy link

Many formats are still missing, for example R16UI. Here is a table showing all possible formats:

https://webgl2fundamentals.org/webgl/lessons/webgl-data-textures.html

It would be good to have them all.

@DavidPeicho
Copy link
Contributor Author

Good point, the spec is a bit fuzzy on all the available formats / internal formats.
If you want to improve this PR that's even better, you just need to add them in src/constants.js and src/renderers/webgl/WebGLUtils.js.

@mrdoob mrdoob modified the milestones: r102, r103 Feb 28, 2019
@mrdoob mrdoob modified the milestones: r103, r104 Mar 27, 2019
@mrdoob mrdoob modified the milestones: r104, r105 Apr 24, 2019
@DavidPeicho DavidPeicho force-pushed the feature/texture-internal-format branch from 7298a83 to 570afb6 Compare May 14, 2019 11:32
@DavidPeicho
Copy link
Contributor Author

DavidPeicho commented May 14, 2019

Thanks @bilgorajskim for the thorough list of formats.

@Mugen87 @WestLangley Are you happy with it?

@ixcviw7bw
Copy link

I've opened another PR in your fork that fixes a few things that were still missing. @DavidPeicho

@mrdoob mrdoob modified the milestones: r105, r106 May 30, 2019
@mrdoob
Copy link
Owner

mrdoob commented Dec 13, 2019

Considering that internalFormat is pretty much GL...

How about just doing:

texture.internalGLFormat = 'R8'

And make the code just do gl[ texture.internalGLFormat ]?

@DavidPeicho
Copy link
Contributor Author

I would be really up for that, because it makes debugging so much easier. Though right now format and type are custom Three.js constants mapped to GL, so this doesn't "compliant" in with what has been made.

I am not too opinionated about that in general. In the same ideas of what you propose, what about

texture.gpuFormat

So it can match later with a WebGPU renderer?

@mrdoob
Copy link
Owner

mrdoob commented Dec 13, 2019

Though right now format and type are custom Three.js constants mapped to GL, so this doesn't "compliant" in with what has been made.

That's okay.

I am not too opinionated about that in general. In the same ideas of what you propose, what about

texture.gpuFormat

So it can match later with a WebGPU renderer?

Sounds good to me 👍

@mrdoob
Copy link
Owner

mrdoob commented Dec 14, 2019

@DavidPeicho do you think you can update the PR with that change? 😇

@DavidPeicho
Copy link
Contributor Author

@mrdoob Yes I will change it. Do we still go through the mapping in WebGLUtils or do we use it like:

// User sets the internal format using the GL name.
texture.gpuFormat= 'R8'
// Renderer uploads the texture using:
var glInternalFormat = WebGL2RenderingContext[texture.gpuFormat]

@mrdoob
Copy link
Owner

mrdoob commented Dec 16, 2019

Those two lines should be enough I think.

@GuDuJian-J-Zhang
Copy link

hope to merge soon

@DavidPeicho
Copy link
Contributor Author

  • Renamed internalFormat to gpuFormat. Includes:
    • Changing the attribute name in Texture.js
    • Modifying slightly the getInternalFormat() function in WebGLTextures.js to accommodate this change.
    • Updating docs to reflect the new name

I didn't change the current Three.js constant mapping to WebGL2 constants, but I kept it as-is, i.e:

myTexture.gpuFormat = THREE.R11F_G11F_B10FFormat; // THREE.R11F_G11F_B10FFormat = 1067
// 1067 is later mapped to WebGL2RenderingContext.R11F_G11F_B10F

For a simple reason, the WebGL2 Specification states that you can still use the RGB and RGBA formats as internal formats. So it means that a user will not be able to do:

myTexture.gpuFormat = THREE.RGBFormat;

Except if in WebGLTextures.js we hardcode something... which didn't seem great like that.

Otherwise we can also delete all constants for gpu format and simply use string, but WebGPU will anyway supports those kind of formats, so I guess it's actually better for Three.js to define it's own constants, and later perform the mapping for each GPU backend (like it's done right now with every constant).

Tell me what you think @mrdoob. If we keep the constants, maybe it could make sense to rename them a bit, I didn't think too much on that.

@mrdoob
Copy link
Owner

mrdoob commented Dec 17, 2019

I didn't change the current Three.js constant mapping to WebGL2 constants, but I kept it as-is, i.e:

myTexture.gpuFormat = THREE.R11F_G11F_B10FFormat; // THREE.R11F_G11F_B10FFormat = 1067
// 1067 is later mapped to WebGL2RenderingContext.R11F_G11F_B10F

For a simple reason, the WebGL2 Specification states that you can still use the RGB and RGBA formats as internal formats. So it means that a user will not be able to do:

myTexture.gpuFormat = THREE.RGBFormat;

Then can't do myTexture.gpuFormat = 'RGB';?

What I'm proposing is to not add all these new constants and just pass a string to gpuFormat.

@DavidPeicho
Copy link
Contributor Author

@mrdoob done.

  • Updated gpuFormat to a string attribute, matching one of the internal format defined in the WebGL2 Specification.
  • Added console.warn in WebGLTextures.js when the specified gpuFormat isn't found
  • Removed every internal format constants from constants.js and WebGLUtils.js
  • Updated docs to reflect the change from constants to string.
  • Updated Texture.d.ts and constants.d.ts to reflect the string type

@DavidPeicho
Copy link
Contributor Author

@mrdoob

  • Renamed gpuFormat back to internalFormat in Texure.js, WebGLTextures.js
  • Updated docs with internalFormat
  • Updated Texture.d.ts with internalFormat as well

Do you want me to squash the commits?

@Mugen87
Copy link
Collaborator

Mugen87 commented Dec 20, 2019

Do you want me to squash the commits?

That would be great! 😇

* Adds `internalFormat` attribute to Texture.js and Texture.d.ts
* Adds `internalFormat` string mapping to GL constant in WebGLTextures.js
* Adds `internalFormat` in Texture documentation
* Adds internal format string constants to constants documentation
@DavidPeicho DavidPeicho force-pushed the feature/texture-internal-format branch from 88fb7fe to d97151b Compare December 20, 2019 11:08
@DavidPeicho
Copy link
Contributor Author

Squashed the commits and rebased.

Just a note to @bilgorajskim, sorry your commits went away. The decision has been made to use directly the GL string as interalFormat anyway. Thanks a lot for the help you provided with all the constants!

@ixcviw7bw
Copy link

No problem :) The way it works now is simpler, and I agree with the decision.

…ork in all cases

Some WebGL2 internal formats rely on new WebGL2 formats. This change
add the 4 new PixelFormat constants that are needed
@DavidPeicho DavidPeicho force-pushed the feature/texture-internal-format branch from eb2189e to 2f188f6 Compare December 20, 2019 12:01
@DavidPeicho
Copy link
Contributor Author

@mrdoob

I pushed again a few changes to constants.js and to WebGLUtils.js. I forgot about that, but some GL internal formats require new GL formats as well. It's just 5 new constants. Unfortunately if we don't add them it makes some nice internal formats unavailable.

According to the spec,

  • format RED_INTEGER is required to use internal format R8UI
  • format RG is required to use internal formats RG*
  • format RG_INTEGER is requried to use internal format RG8UI
  • format RGB_INTEGER is requried to use internal format RGB8UI
  • format RGBA_INTEGER is requried to use internal format RGBA8UI

@mrdoob
Copy link
Owner

mrdoob commented Dec 20, 2019

Great! I'll do some additional clean up after merging but this looks great.

@mrdoob mrdoob merged commit 5f74ae5 into mrdoob:dev Dec 20, 2019
@mrdoob
Copy link
Owner

mrdoob commented Dec 20, 2019

Thanks for being patient! 😇

@mrdoob
Copy link
Owner

mrdoob commented Dec 20, 2019

@DavidPeicho a8cc32b

@WestLangley
Copy link
Collaborator

@DavidPeicho Awesome!

@rotu
Copy link
Contributor

rotu commented Feb 13, 2023

It seems like you can uniquely infer the format from an internalFormat (but not vice versa).

Any reason not to accept an internalFormat value in the Texture constructor instead of a base format?

@mrdoob
Copy link
Owner

mrdoob commented Feb 14, 2023

Moving properties around in the constructor is a pain and the usage is messy.
Better to keep constructor properties to a minimum and modify the desired values after construction.

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.

7 participants