Request for feedback: double-precision accurate translation #1665
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
pixelflinger
force-pushed
the
ma/accurate-transforms
branch
from
2e072f7
to
ae4b4d8
Compare
prideout
approved these changes
Sep 19, 2019
There was a problem hiding this comment.
Really cool idea. It does not introduce any complexity and the changes are limited to TransformManager. ECS for the win!
I could also imagine allowing users to use a hypothetical TransformManager<double> which simply stores the entire 4x4 as double, potentially giving us better precision parity with JavaScript engines like ThreeJS.
romainguy
reviewed
Sep 19, 2019
|
|
||
| const math::mat4f& getTransform(Instance ci) const noexcept { | ||
| return mManager[ci].local; | ||
| } | ||
|
|
||
| math::mat4 getAccurateTransform(Instance ci) const noexcept { |
There was a problem hiding this comment.
It would be worth documenting these two new functions.
bejado
reviewed
Sep 19, 2019
Augment the TransformManager so it can maintain the 4th column as
double precision; we accomplish this by storing an extra float3 per matrix.
It's calculated as `doubleTranslation - float3{doubleTranslation}`.
We also add methods to set and get a transform as mat4, internally only
the 4th column is kept as double precision.
Finally when we calculate the worldTransform, we take this extra data into
account, but only for calculating the new 4th column, so the extra work
is small.
pixelflinger
force-pushed
the
ma/accurate-transforms
branch
from
ae4b4d8
to
228f9b7
Compare
pixelflinger
commented
Dec 13, 2019
|
Closing PR, keeping the branch open |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Filament does all its calculations for rendering in view-space,
which means it's able to handle very large scenes.
However, the position of objects (and camera) in the scene is still
limited to float precision.
A related issue is that our TransformManager is only able to store
float matrices.
This PR is an attempt to address these issues efficiently.
The first thing to notice is that we only need a high precision on the translation component of matrices, this is because most of our objects are simply rotated and translated, and rarely scaled by very-large amounts.
Here me augment the TransformManager so it can maintain the 4th column as
double precision; we accomplish this by storing an extra float3 per matrix.
It's calculated as
doubleTranslation - float3{doubleTranslation}.We also add methods to set and get a transform as mat4, internally only
the 4th column is kept as double precision.
Finally when we calculate the worldTransform, we take this extra data into
account, but only for calculating the new 4th column, so the extra work
is small. This makes assumption that the 4th row is [0 0 0 x], but even
if it's not, the result wouldn't be worse than it is now.
Filament could them make use of this to maintain the precision of
the position when converting everything to viewspace.