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

Using atlas causes texture to reupload multiple times. Works in R89/R90, doesnt in R91+ #14391

Closed
8 of 15 tasks
Excaa opened this issue Jun 28, 2018 · 9 comments
Closed
8 of 15 tasks

Comments

@Excaa
Copy link

Excaa commented Jun 28, 2018

Description of the problem

In version R90 I have an atlas that is loaded/created with:

new THREE.TextureLoader(manager).load(url, function(t){
         //Upload to gpu 
	renderer.setTexture2D(t, 0);
         //Frames is the atlas data.
	for (name in frames)
	{
		var frame = frames[name];
		var tex = t.clone();
		tex.uuid = t.uuid; // prevents loading texture multiple times
		tex.repeat.set(frame.w / t.image.width, frame.h / t.image.height);
		tex.offset.x = ((frame.x) / t.image.width);
		tex.offset.y = 1 - (frame.h / t.image.height) - (frame.y / t.image.height);
		tex.needsUpdate = true;
		
                                  //AssetManager is just a map for quick access to textures.
		AssetManager.add(n, tex);
	}
});

In addition to atlas I have large equirectangular textures loaded. With version 90 (and 89) renderer.info.memory says 23 textures when scene is being rendered and it keeps at that whenever objects found in the scene are set to visible / invisible.

With R91 and newer the texture count starts to ramp up quickly. Initial state has textures set at 79 (most likely due to overlay ui which has lots of assets all found in one atlas. Whenever I click an area where object is and it is set to visible then the count jumps by some sum (depending on what the object is). All of the textures used on objects are found in another atlas image.

Checking with the debugger in R90 and R89 texture upload is done only initially. In R91+ it happens everytime an object is set to visible (one that has not been visible before).

Will provide an example after I find some images I can distribute freely.

Three.js version
  • Dev
  • r94
  • r93
  • r92
  • r91
Browser
  • All of them
  • Chrome (67.0.3396.87)
  • Firefox (61.0)
  • Internet Explorer

Most likely also IE. Cannot test with one at the moment.

OS
  • All of them
  • Windows
  • macOS
  • Linux
  • Android
  • iOS
Hardware Requirements (graphics card, VR Device, ...)

GPU used in tested is nvidia 1060 and 770.

@Excaa
Copy link
Author

Excaa commented Jun 28, 2018

https://excaa.github.io/threejs/atlas/indexR90.html
https://excaa.github.io/threejs/atlas/indexR94.html

Topleft text is the renderer.info.memory data. With R90 the texture amount stays properly on 2. With R94 it increases until all textures are used at least once. Clicking changes the texture in cubes.

@Mugen87
Copy link
Collaborator

Mugen87 commented Jun 29, 2018

That happens because of #13102

You can easily verify this with the following fiddle: https://jsfiddle.net/f2Lommf5/8204/

It uses your code with three.js R90. I've commented out tex.uuid = t.uuid; in line 113 which was the reason for the missing updates because WebGLProperties returned for different texture objects the same internal properties object. Overwriting the uuid like this is actually a hack. You are creating new Texture objects via Texture.clone() and therefore they should have unique uuids.

The new implementation based on WeakMap() is just more strict (and correct) since it prevents this type of manipulation.

@Excaa
Copy link
Author

Excaa commented Jun 29, 2018

Is there another way then to reuse the basetexture. Will have a huge performance hit if texture uploading happens multiple times.

@Mugen87
Copy link
Collaborator

Mugen87 commented Jun 29, 2018

Why not having a single texture and alter the uv-coordinates of the geometries, instead?

@Mugen87
Copy link
Collaborator

Mugen87 commented Jun 29, 2018

Any yes, having multiple Texture objects leads to multiple texture uploads. It is not possible to avoid this right now.

@Excaa
Copy link
Author

Excaa commented Jun 29, 2018

I have a shared geometry currently in use. But will check if that would be more optimal solution. Thanks for the ideas & info.

Closing the issue as it's not really an issue after all.

@Excaa Excaa closed this as completed Jun 29, 2018
@donmccurdy
Copy link
Collaborator

It is an issue — either the texture.image data from cloned textures should be reused to avoid a duplicate upload, or properties like UV transforms should not be on the texture object. See #12788 for more details. This is addressed, or can be, through the (in progress) NodeMaterial system.

@Mugen87
Copy link
Collaborator

Mugen87 commented Jul 2, 2018

@donmccurdy There was an interesting suggestion by @mrdoob to introduce something like THREE.Image (see #5876 (comment)).

@donmccurdy
Copy link
Collaborator

Interesting... now that ES Map and WeakMap support is common, I think it should be possible for WebGLRenderer to detect reuse of texture.image without modifying image or canvas elements, in which case we shouldn't need the extra class? I looked into that a while ago and it seemed like a non-trivial change to WebGLRenderer.

From my perspective if we were adding a layer of abstraction it should be between Material and Texture, rather than beneath a Texture, like Material -> Map/Slot -> Texture -> texture.image, where the Map/Slot becomes a good place for UV transforms, x/y/z channel switches, etc., and removes most reasons to clone a texture. But of course that's also a very large change, and overall the NodeMaterial approach seems much more flexible.

@Mugen87 Mugen87 mentioned this issue Jul 3, 2018
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

No branches or pull requests

3 participants