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: take camera FOV into account #26535

Conversation

alberto-taiuti-skydio
Copy link

@alberto-taiuti-skydio alberto-taiuti-skydio commented Aug 4, 2023

Make Sprites which use a SpriteMaterial also consider the FOV of a Perspective Camera.
This ensures that Sprites maintain a constant size in screen-space regardless of the changes in the FOV , not only the distance from the camera.

The existing issue was that Sprites would still change in size when the FOV of the camera was modified, even if sizeAttenuation was set to false.

To do this, modify the sprite.glsl.js shader to also take into account the camera FOV.
Specifically, multiply the scale factor by the tangent of half of the FOV. This value's reciprocal (i.e. 1 / the value) is located at the [1][1] position in the perspective projection matrix in OpenGL, so we use it directly.
See https://stackoverflow.com/a/57822509 for details.

Finally, add an example that shows non-size attenuated Sprites specifically, with sliders that change the distance of the camera and the FOV of the camera.

Contribution also by @vincent-lecrubier-skydio.

Make `Sprite`s which use a `SpriteMaterial` with `sizeAttenuation` set to
`false` also consider the FOV of a Perspective Camera. This ensures
that `Sprite`s maintain a constant size in screen-space regardless of the
changes in the FOV , not only the distance from the camera.

The existing issue was that `Sprite`s would still change in size when
the FOV of the camera was modified, even if `sizeAttenuation` was set to
`false`.

This is particularly useful when animating the FOV of the camera if one
expects the Sprites with `sizeAttenuation` set to `false` to maintain
their original size.

To do this, modify the `sprite.glsl.js` shader to also take into
account the camera FOV.
Specifically, divide the `scale` factor by the tangent of half of the
FOV. This value is located at the [1][1] position in the perspective
projection matrix in OpenGL, so we use it directly.
See https://stackoverflow.com/a/57822509 for details.

Also add an example that shows non-size attenuated `Sprite`s
specifically, with sliders that change the distance of the camera and
the FOV of the camera.
@github-actions
Copy link

github-actions bot commented Aug 4, 2023

📦 Bundle size

Full ESM build, minified and gzipped.

Filesize dev Filesize PR Diff
646 kB (160.2 kB) 646.6 kB (160.4 kB) +591 B

🌳 Bundle size after tree-shaking

Minimal build including a renderer, camera, empty scene, and dependencies.

Filesize dev Filesize PR Diff
439.1 kB (106.3 kB) 439.5 kB (106.4 kB) +376 B

@alberto-taiuti-skydio alberto-taiuti-skydio marked this pull request as draft August 4, 2023 15:28
@alberto-taiuti-skydio alberto-taiuti-skydio marked this pull request as ready for review August 4, 2023 16:19
Make the `Sprite` object also take the FOV of the camera into account
in its raycast method.
@alberto-taiuti-skydio
Copy link
Author

alberto-taiuti-skydio commented Aug 8, 2023

I've realized that this change would cause a lot of developers issues if it was implemented like this: everyone relying on Sprite would suddenly have the screen-space (i.e. the final rendered size) of their Sprites be different and they'd need to fix it by changing the scale.

Basically it would be a breaking change in the sense that any developer that upgrades to a new version of Three which includes this change would suddenly have their sprites be way smaller than before the upgrade, without a clear reason why. Existing apps would suddenly look different.

So I'll make this be dependent on another parameter on SpriteMaterial. This way, it's an opt-in change, and existing applications that upgrade to newer versions of Three won't be surprised.

@alberto-taiuti-skydio alberto-taiuti-skydio changed the title Sprite: take FOV into account if sizeAttenuation is false Sprite: take camera FOV into account Aug 8, 2023
@WestLangley
Copy link
Collaborator

How about a one-liner in your app, instead, applied whenever camera.fov is changed?

For example,

spriteA.scale.setScalar( 0.2 * camera.fov / 60 );

@alberto-taiuti-skydio
Copy link
Author

@WestLangley

How about a one-liner in your app, instead, applied whenever camera.fov is changed?

That's the first thing I went for, but then I realized that:

  1. Since one potentially wants to animate changes to the FOV, by changing the scale of each sprite in a scene one by one at 60FPS is slower than just using the data already available in the vertex shader, especially in the case where there are a lot of sprites in a scene
  2. Especially when you have a large number of sprites in a scene, like in our app, we can save CPU cycles by just reusing what's already available in the vertex shader
  3. Since we use react-three-fiber, to do the above in it you'd have to subscribe to render loop updates using useFrame. Which means that for every Sprite in the scene you are creating a subscription that needs to be called and code to be run. When the number of sprites is large, this eats up CPU cycles

So in general the reason to do this is that we can achieve the same as the one liner above but do it without using any extra CPU cycles nor having to remember to do it for each sprite.

@alberto-taiuti-skydio
Copy link
Author

alberto-taiuti-skydio commented Sep 27, 2023

@WestLangley what do you think of the above? Could we maybe try and work on this so we can merge it? Happy to discuss the changes if needed!

@WestLangley
Copy link
Collaborator

Actually, I am not the one who makes that decision. :-)

@Mugen87 will make a decision based on what he believes is best for the project.

@Mugen87
Copy link
Collaborator

Mugen87 commented Sep 27, 2023

Especially when you have a large number of sprites in a scene, like in our app, we can save CPU cycles by just reusing what's already available in the vertex shader

When you have a large number of sprites (let's say > 100), THREE.Sprite is problematic in any event because of the resulting number of draw calls. This is a way larger overhead than animating the scale property for all sprites every frame. So using instanced planes with InstancedMesh or even THREE.Points would be a noticeably faster alternative than THREE.Sprite in these scenarios (assuming materials can be shared to a certain degree). When I remember correctly, a plane can be oriented towards the camera by doing this:

plane.quaternion.copy( camera.quaternion );

Using this in combination with InstancedMesh you have instanced billboards.

The workaround @WestLangley has shared seems like a good solution. Considering the changes to the core (a new material property and the shader/renderer modifications), I'm afraid the PR seems not justified at the moment.

@Mugen87 Mugen87 closed this Oct 6, 2023
@alberto-taiuti-skydio
Copy link
Author

Makes sense, thank you both for reviewing.

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.

3 participants