Fix oct-encoded normal upsampling. #1961
Conversation
…s in all cases, due to how the octrahedron is unwrapped into a unit square.
|
@kring Can you take a look at this? |
| return Cartesian3.normalize(result, result); | ||
| } | ||
|
|
||
| function octEncode(vector, out) { |
There was a problem hiding this comment.
Can you make this a generic unit-tested function (even if it is @private) since we are going to want it for the geometry pipeline, which we are going to start work on again soon for KML? Perhaps it is a function Cartesian3? Or perhaps it is better to make a new oct class and put static decode and encode functions in it.
signNotZero and toSNorm could probably go in Math.js.
There was a problem hiding this comment.
I'd be happy to. I was on the fence about moving these into Core in this pull request since it is currently only used in this location.
There was a problem hiding this comment.
…mMath. Unit tests included.
| /** | ||
| * Encodes a normalized vector into 2 bytes following the 'oct' encoding. | ||
| * The 'oct' encoding is described in "A Survey of Efficient Representations of Independent Unit Vectors", | ||
| * Cigolle et al 2014: http://jcgt.org/published/0003/02/01/ |
There was a problem hiding this comment.
I think this wording is a little klunky and we we usually make these link tags. How about?
Encodes a normalized vector into 2 bytes following the 'oct' encoding as described in {@link http://jcgt.org/published/0003/02/01/|A Survey of Efficient Representations of Independent Unit Vectors}.
There was a problem hiding this comment.
Actually, I would consolidate most of the doc for this function and the decode function and move it into the Oct declaration above.
|
This is ready for review. |
| } | ||
| var magSquared = Cartesian3.magnitudeSquared(vector); | ||
| if (Math.abs(magSquared - 1.0) > CesiumMath.EPSILON6) { | ||
| throw new DeveloperError('vector must be normalized.'); |
There was a problem hiding this comment.
This should be documented.
|
Alright, exceptions have been documented. Thanks for the review. |
| * Cigolle et al 2014: {@link http://jcgt.org/published/0003/02/01/} | ||
| * | ||
| * @namespace | ||
| * @alias Oct |
There was a problem hiding this comment.
Does this need to be public? Seems like it would be a private class to me.
There was a problem hiding this comment.
Yeah, seems like it probably should be private. Easy enough to make it public in the future if there's a need for it.
| @@ -107,6 +107,8 @@ Change Log | |||
| * Added northUpEast transform to help support display of glTF models because Y is their up axis. | |||
| * Cesium can now render an unlimited number of imagery layers, no matter how few texture units are supported by the hardware. | |||
| * Added `czm_octDecode` and `czm_signNotZero` builtin functions. | |||
| * Added `Oct` namespace that defines static functions for encoding and decoding normalized unit vectors to and from oct-encoding. | |||
There was a problem hiding this comment.
This needs to go away now that Oct is private.
|
Just those two comments, one of which is incredibly minor and you can feel free to ignore it. Everything else looks good. |
|
Alright, thanks for the feedback. Updates are included. |
Fix oct-encoded normal upsampling.
Cannot linearly interpolate oct encoded normals in all cases, due to how the octrahedron is unwrapped into a unit square.