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 new render target types for 3D and array textures. #23498

Merged
merged 4 commits into from Feb 17, 2022
Merged

Conversation

Mugen87
Copy link
Collaborator

@Mugen87 Mugen87 commented Feb 15, 2022

Fixed #23466.

Description

The PR implements WebGL3DRenderTarget and WebGLArrayRenderTarget similar to WebGLCubeRenderTarget.

The undocumented WebGLRenderTarget.setTexture() can be removed now. It was previously used to overwrite the texture reference of a render target to make webgl2_rendertarget_texture2darray.html work. This was actually a hack since the texture reference of a render target should not be replaced.

I'll add the doc pages for both new render targets if the PR gets merged.

@mrdoob
Copy link
Owner

mrdoob commented Feb 15, 2022

@DavidPeicho Also FYI

@mrdoob
Copy link
Owner

mrdoob commented Feb 15, 2022

I think we should probably figure out these names too...

Texture
Texture > CanvasTexture
Texture > CompressedTexture
Texture > CubeTexture 
Texture > DataTexture
Texture > DataTexture2DArray // Should this be renamed to Data2DArrayTexture? Or DataArray2DTexture?
Texture > DataTexture3D // Should this be renamed to Data3DTexture?
Texture > DepthTexture
Texture > FramebufferTexture
Texture > VideoTexture

WebGLRenderTarget
WebGLRenderTarget > WebGL2DArrayRenderTarget // New. Should it be WebGLArray2DRenderTarget?
WebGLRenderTarget > WebGL3DRenderTarget // New
WebGLRenderTarget > WebGLCubeRenderTarget
WebGLRenderTarget > WebGLMultipleRenderTargets // Could we remove this one and use an Array of WebGLRenderTargets instead? (Like how we did with MultiMaterial)

@mrdoob mrdoob added this to the r138 milestone Feb 15, 2022
@DavidPeicho
Copy link
Contributor

Just a heads up, Texture3D might still be broken on ANGLE isn't it? At least when you render to a level > 0. Linking the open bug: https://bugs.chromium.org/p/angleproject/issues/detail?id=5009

I think the same will happen with layered textures.

Regarding namings, it's not consistent with DataTexture, but technically we could go for:

Texture2DArray
Texture3D

Because those types are constructible only with raw data anyway?

@Mugen87
Copy link
Collaborator Author

Mugen87 commented Feb 16, 2022

Good point! However, I like the suggestion of @mrdoob to ensure the texture classes always end like so *Texture. I vote for:

DataTexture3D > 3DTexture
DataTexture2DArray > 2DArrayTexture

The two new render target names already sound good to me.

@DavidPeicho
Copy link
Contributor

DavidPeicho commented Feb 16, 2022

I don't think we can start a class with a number in JS :(

@Mugen87
Copy link
Collaborator Author

Mugen87 commented Feb 16, 2022

Totally forgot that 🤦 . Too bad...

@Mugen87
Copy link
Collaborator Author

Mugen87 commented Feb 16, 2022

How about using the same approach like for 3MFLoader? The JS file becomes 3DTexture.js but the class:

DataTexture3D > ThreeDTexture
DataTexture2DArray > ArrayTexture

@DavidPeicho
Copy link
Contributor

Or we alias all the existing texture class and go for:

TextureCanvas
TextureCompressed
TextureCube
TextureData
Texture2DArray
Texture3D
...

Bigger change in the codebase, but from the user's pesperctive all the previous type exists as an alias?

@mrdoob
Copy link
Owner

mrdoob commented Feb 16, 2022

If we did that we'd have to change the whole API... CameraPerspective, GeometryBox, MaterialPoints, ...

@DavidPeicho
Copy link
Contributor

To me it doesn't seem confusing to only do it for textures, we don't need to do it for every class.
Otherwise we just keep the current names 😄

@mrdoob
Copy link
Owner

mrdoob commented Feb 16, 2022

How about this?

Data3DTexture
DataArrayTexture
DataCubeTexture

WebGL3DRenderTarget
WebGLArrayRenderTarget
WebGLCubeRenderTarget

Basically *Array* instead of *Array2D* or *2DArray*.
Isn't 2D implied when we don't specify 3D anyway?

@Mugen87
Copy link
Collaborator Author

Mugen87 commented Feb 17, 2022

Basically Array instead of Array2D or 2DArray.

Sounds good!

DataCubeTexture isn't right though since you can setup a cube texture of normal HTML5 images or canvas elements.

@Mugen87
Copy link
Collaborator Author

Mugen87 commented Feb 17, 2022

Updated the PR with the renamed data textures.

@mrdoob mrdoob merged commit f9cd9ca into mrdoob:dev Feb 17, 2022
@mrdoob
Copy link
Owner

mrdoob commented Feb 17, 2022

Thanks!

@0b5vr 0b5vr mentioned this pull request Mar 1, 2022
16 tasks
0b5vr added a commit to 0b5vr/three-ts-types that referenced this pull request Mar 1, 2022
See: mrdoob/three.js#23498

### Description

- Add new classes:
  - `WebGL3DRenderTarget`
  - `WebGLArrayRenderTarget`
  - `Data3DTexture`
  - `DataArrayTexture`
- Made them deprecated:
  - `DataTexture2DArray`
  - `DataTexture3D`
- Removed `WebGLRenderTarget.setTexture`
- Fixed related classes
- Fixed related examples

### What I didn't do

- changes applied to `examples/jsm/renderers/webgpu/WebGPUTextures` , since it does not exist in this repository.
joshuaellis pushed a commit to three-types/three-ts-types that referenced this pull request Mar 1, 2022
* change: Add new render target types for 3D and array textures

See: mrdoob/three.js#23498

### Description

- Add new classes:
  - `WebGL3DRenderTarget`
  - `WebGLArrayRenderTarget`
  - `Data3DTexture`
  - `DataArrayTexture`
- Made them deprecated:
  - `DataTexture2DArray`
  - `DataTexture3D`
- Removed `WebGLRenderTarget.setTexture`
- Fixed related classes
- Fixed related examples

### What I didn't do

- changes applied to `examples/jsm/renderers/webgpu/WebGPUTextures` , since it does not exist in this repository.

* fix: Fix a test case that fails because of the previous commit
donmccurdy pushed a commit to donmccurdy/three.js that referenced this pull request Mar 10, 2022
* Add new render target types for 3D and array textures.

* Rename data textures.

* Examples: Clean up.

* Remove options parameter from new render targets.
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.

(docs) Missing changelog/docs for r126 WebGLRenderTarget.setTexture
3 participants