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

Sprite issues #7956

Closed
kdrnic opened this issue Jan 13, 2016 · 15 comments
Closed

Sprite issues #7956

kdrnic opened this issue Jan 13, 2016 · 15 comments

Comments

@kdrnic
Copy link

kdrnic commented Jan 13, 2016

The current THREE.Sprite implementation has a number of issues.

  • Sprites can't have their texture translated, that is you can't change UVs, so you can't use them for explosions or other effects
  • You can't translate the image of a sprite, that is, the center of the texture is aligned with the origin of the object and you can't move it horizontally or vertically
  • Sprites can't be rotated independently, as rotation is set in their material. If you want to rotate sprites independently you must assign each one a different material, even if they share the same texture. I see no reason the rotation attribute couldn't and shouldn't be stored on the Sprite structure instead of the SpriteMaterial structure.
  • The BufferGeometry in the sprite is make-believe and actually ignored, which is not only inefficient as it is also confusing as it implies you can rotate, translate and animate the sprite by changing it, but you can't

The last one is actually quite interesting.
See https://github.com/mrdoob/three.js/blob/master/src/objects/Sprite.js#L12
and https://github.com/mrdoob/three.js/blob/master/src/renderers/webgl/plugins/SpritePlugin.js#L24
I even tested this by commenting out the BufferGeometry creation in THREE.Sprite, and sprites continued working.

Meanwhile I imagine the workaround is to use a Mesh with a PlaneGeometry and a .lookAt(camera.position) in every loop in place of Sprites if one needs the features mentioned above.

Fixing this issue I imagine is not terribly difficult.

Here is my suggested plan:

  • Move rotation from SpriteMaterial to Sprite (and take rotation from Sprite instead of SpriteMaterial in SpritePlugin)
  • Either add an UV array and a translation array to Sprite, and use that in SpritePlugin, or actually use the BufferGeometry already in Sprite
  • Depending on the above, remove the BufferGeometry from Sprite
@kdrnic
Copy link
Author

kdrnic commented Jan 13, 2016

Also, I am aware that the issue title is bad, please change it to something more adequate, I could not think of a sufficiently descriptive name myself

@karimbeyrouti
Copy link
Contributor

Couple more to add:

@WestLangley
Copy link
Collaborator

Sprites can't have their texture translated

What about the sprite sheet example in http://threejs.org/examples/misc_ubiquity_test2.html?

In any event, there is room for improvement.

In fact, in the above example, type webglRenderer.info into the console to see that in the sprite sheet example, there are many textures pushed to the GPU.

@karimbeyrouti
Copy link
Contributor

I was looking at your lovely prototype with translate, scale, rotation and centre properties for Materials ( on this feature: #5876 ) when I was implementing sprite sheets.

However nice that material update would be, repeat and offset to worked nicely with translation / scaling of texture.

@kdrnic
Copy link
Author

kdrnic commented Jan 13, 2016

One has to have a different THREE.Texture for each sprite as long as one wants them to be individually animated. Though, as I understand that example uses .clone() to make the texture copies, which is not a deep copy thus resulting in no texture repetition in the video RAM, so it is not an issue.

Is webglRenderer.info counting each THREE.Texture existent, or somehow accounts for the clones?

I agree that .repeat and .offset should be moved to THREE.Material.

Also I was not aware that .repeat was for scaling, the name is a little inadequate IMHO.

@karimbeyrouti
Copy link
Contributor

Could be wrong - but I think you can hack it not to re-upload the cloned to the GPU texture by setting the cloned uuid to the same as the source texture, i.e.:

clonedTexture.uuid = sourceTexture.uuid;

@WestLangley
Copy link
Collaborator

Confirmed. In the sprite sheet example, using the hack

var tex = texture1.clone();
tex.uuid = texture1.uuid;

prevents the texture from being uploaded to the GPU repeatedly.

@kdrnic
Copy link
Author

kdrnic commented Jan 14, 2016

If so, couldn't/shouldn't THREE.Texture.clone be patched to include setting the uuid perhaps? Or is there something I am missing

@karimbeyrouti
Copy link
Contributor

Note the GPU texture hack:

var tex = texture1.clone();
tex.uuid = texture1.uuid;

caused a huge performance drop in my render time ( sub 1ms to 35 ms + ).

@kdrnic
Copy link
Author

kdrnic commented Jan 19, 2016

Are you sure this uuid hack affects texture being uploaded to the GPU instead of just affecting webglRenderer.info counting of textures?

@karimbeyrouti
Copy link
Contributor

I am going with whatever WestLangley says on this one :-) - however the huge performance drops are caused by Image Decode events in chrome: http://imgur.com/dcUliZc

These go away when I disable 'tex.uuid = texture1.uuid;'

@bhouston
Copy link
Contributor

bhouston commented Mar 3, 2016

I have just submitted a PR that allows for per-map/per-material UV offset/repeat that should address this issue: #8278

@augusttty
Copy link

I have a solution to make spriteMaterial objects to rotate with scene.
mesh.material.rotation = - controller.constraint.getAzimuthalAngle()
add this dynamicly when scene updates,or controller rotates.
: )

@Mugen87
Copy link
Collaborator

Mugen87 commented Jul 3, 2018

BTW: The mentioned hack in #7956 (comment) does not work anymore since R91. #13102 introduces the usage of WeakMap so the uuid is no longer used to manage texture objects. Also see #14391

@Mugen87
Copy link
Collaborator

Mugen87 commented Jul 7, 2018

Closing. Most of this issue is already addressed in #5876 and #7522. Besides, the implementation of sprites has fundamentally changed with #14411. If new stuff is added to WebGLRenderer, it should be also available for sprites.

@Mugen87 Mugen87 closed this as completed Jul 7, 2018
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

7 participants