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

Introducing THREE.Image #17766

Closed
Mugen87 opened this issue Oct 18, 2019 · 12 comments
Closed

Introducing THREE.Image #17766

Mugen87 opened this issue Oct 18, 2019 · 12 comments

Comments

@Mugen87
Copy link
Collaborator

Mugen87 commented Oct 18, 2019

Moved the discussion from #5728 to a separate issue.

The idea is to add THREE.Image as an additional class which is used by THREE.Texture. An instance of THREE.Image is directly mapped to an instance of WebGLTexture (the raw WebGL texture object). That means multiple textures can refer to the same instance of THREE.Image and not causing redundant texture uploads anymore (related issues #5728, #5821, #7223)

Ideally, THREE.Image will make the handling of image objects transparent (e.g. the difference in HTMLImageElement, ImageBitmap or the image stub in DataTexture is encapsulated in this new class).

@donmccurdy
Copy link
Collaborator

What would be the goals of this change? From the issues attached I see two:

  1. Ability to use different values of repeat and offset on the same texture, without duplicate GPU upload. Using multiple THREE.Texture instances would not duplicate data as long as the underlying THREE.Image was reused.
  2. Prevent UniformsUtils.clone() from causing duplicate GPU upload. Cloning THREE.Texture instances would not duplicate data as long as the underlying THREE.Image was not cloned.

For (1), adding THREE.Image sounds like a good solution under the current API. Depending on how/if/when we are introducing NodeMaterial, there may be others:

material1.color = new TextureNode( texture, uvNode1 );
material2.color = new TextureNode( texture, uvNode2 );

For (2), I'm not sure whether UniformsUtils really needs to clone textures, or could just copy by reference.

I don't have an opinion for or against this change yet, just trying to understand the goal and whether the additional abstraction is the right solution.

@Mugen87
Copy link
Collaborator Author

Mugen87 commented Oct 18, 2019

The goal is to solve both issues. When cloning a texture, THREE.Image would be shared between the original and new texture. Implementing THREE.Image would enable a solution without waiting for NodeMaterial landing in the core.

@aardgoose
Copy link
Contributor

Is the intention to use this to map to the separate texture and sampler objects provided by WebGL2. This would seem a logical extension.

@Mugen87
Copy link
Collaborator Author

Mugen87 commented Oct 18, 2019

Actually, WebGL 2 was not considered so far. And I think it's at the beginning not important. The main goal is a better management of internal WebGLTexture objects.

@WestLangley
Copy link
Collaborator

Related: #12497.

@adevart
Copy link

adevart commented Oct 21, 2019

BabylonJS seems to have a similar class for this:

https://doc.babylonjs.com/api/classes/babylon.internaltexture
https://github.com/BabylonJS/Babylon.js/tree/master/src/Materials/Textures
https://github.com/BabylonJS/Babylon.js/blob/master/src/Materials/Textures/baseTexture.ts
https://github.com/BabylonJS/Babylon.js/blob/master/src/Materials/Textures/internalTexture.ts

Their baseTexture (base class for their texture class) stores an InternalTexture and InternalTexture has references to the direct webGL buffers so the InternalTexture can be shared between different textures and use the same buffers.

@Mugen87
Copy link
Collaborator Author

Mugen87 commented Oct 21, 2019

Thanks for this pointer. Indeed, ideally THREE.Image would be something similar than InternalTexture.

@EliasHasle
Copy link
Contributor

Will certain parameters be moved into THREE.Image, such as format, type or encoding?

@ivanpopelyshev
Copy link

ivanpopelyshev commented Oct 26, 2019

Hello from pixi.js!

https://github.com/pixijs/pixi.js/tree/dev/packages/core/src/textures

BaseTexture, always was there.

Ideally, THREE.Image will make the handling of image objects transparent (e.g. the difference in HTMLImageElement, ImageBitmap or the image stub in DataTexture is encapsulated in this new class).

While you are at it, you can also look at our brand-new v5 resource system - texture upload is separated. Example: https://pixijs.io/examples/#/textures/gradient-resource.js . Standard ones are ImageBitmap/HTMLImageElement/HTMLCanvasElement , users can implement their own , call texImage2D from the overriden method, set up extra params in BaseTexture.

I use it for texture generation, partial uploading, runtime atlases.

It was very difficult to rewrite texture subsystem and to pass reviews of whole team. Its really 2-year experience, I recommend you to look at it to save the time.

@Mugen87
Copy link
Collaborator Author

Mugen87 commented Oct 26, 2019

It was very difficult to rewrite texture subsystem

Yeah, I've realized this when I've hacked around at this topic last year 😞 . Thanks for the input though!

@ivanpopelyshev
Copy link

ivanpopelyshev commented Oct 26, 2019

Yeah, I've realized this when I've hacked around at this topic last year

That's why I suggest to look more first before you start it. I gave you more reason to try that refactoring =)

texture -> baseTexture
baseTexture -> internalTexture (per context)
baseTexture -> resource (YUMMY)

@mrdoob
Copy link
Owner

mrdoob commented Feb 2, 2022

THREE.Image ended up being named THREE.Source #22846

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

Successfully merging a pull request may close this issue.

8 participants