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

Snorm range max #4121

Merged
merged 9 commits into from
Jul 20, 2016
Merged

Snorm range max #4121

merged 9 commits into from
Jul 20, 2016

Conversation

lasalvavida
Copy link
Contributor

@lasalvavida lasalvavida commented Jul 14, 2016

Adds the ability to vary the range size for SNORM and oct-encoding values for #100.

@pjcozzi, can you look at this when you get a chance?

You can use this to get oct24P and oct32P.

@pjcozzi
Copy link
Contributor

pjcozzi commented Jul 14, 2016

For CesiumGS/3d-tiles#100

* @returns {Cartesian2} The 2 byte oct-encoded unit length vector.
*
* @exception {DeveloperError} vector must be normalized.
*
* @see AttributeCompression.octDecode
*/
AttributeCompression.octEncode = function(vector, result) {
AttributeCompression.octEncode = function(vector, result, rangeMax) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

result parameters should come last so we need to introduce a new function (and we can implement this function using that new one; no need to deprecate this).

*
* @see AttributeCompression.octEncode
*/
AttributeCompression.octDecode = function(x, y, result) {
AttributeCompression.octDecode = function(x, y, result, rangeMax) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same comment.

@pjcozzi
Copy link
Contributor

pjcozzi commented Jul 14, 2016

@bagnell can you take a quick look at the math and the tests?

@lasalvavida
Copy link
Contributor Author

@pjcozzi Updated

@bagnell
Copy link
Contributor

bagnell commented Jul 14, 2016

The code looks good to me. The only comment I have is do we want to use an arbitrary range? Shouldn't it take the number of bits?

@pjcozzi
Copy link
Contributor

pjcozzi commented Jul 14, 2016

Did you remember to commit CHANGES.md? I don't see it.

@lasalvavida
Copy link
Contributor Author

Did you remember to commit CHANGES.md? I don't see it.

You're right, I missed it. It has been added.

@lasalvavida
Copy link
Contributor Author

The code looks good to me. The only comment I have is do we want to use an arbitrary range? Shouldn't it take the number of bits?

I wanted to avoid calling Math.pow; I would be fine with it either way though.

@bagnell
Copy link
Contributor

bagnell commented Jul 14, 2016

I'm fine with it either way as well, but if we keep it this way, it should be mentioned in the doc.

@lasalvavida
Copy link
Contributor Author

lasalvavida commented Jul 14, 2016

Could you be more specific? Is there a part of the doc that is unclear?

@bagnell
Copy link
Contributor

bagnell commented Jul 14, 2016

rangeMax isn't just the maximum value. It is supposed to be a power of 2 and determines how many bits the result is stored in. Doesn't the doc say an oct encoded normal is stored in 2 bytes? If not, it should. There's other functions in this class that assume that's true.

@lasalvavida
Copy link
Contributor Author

The behavior of octEncode and octDecode is unchanged - it is a specific case of octEncodeInRange and octDecodeInRange where rangeMax=255, and the doc for those functions still says 2 bytes.

octEncodeInRange and octDecodeInRange take an arbitrary range currently. Should they throw an exception if rangeMax isn't a power of two? I'm fine with doing that, I'm just not sure if it adds any value.

@bagnell
Copy link
Contributor

bagnell commented Jul 15, 2016

octEncodeInRange and octDecodeInRange take an arbitrary range currently. Should they throw an exception if rangeMax isn't a power of two?

Yes, it can be an arbitrary range but it determines how many bits the number can be stored in. No need to throw an exception. Add something like used to store in log2(rangemax) bits to the doc.

@lasalvavida
Copy link
Contributor Author

Updated

@lasalvavida
Copy link
Contributor Author

Can I get an update on this? I'm going to need this to finish i3dm.

@bagnell
Copy link
Contributor

bagnell commented Jul 20, 2016

Looks good to me. @pjcozzi do you want to take another look?

@lasalvavida lasalvavida mentioned this pull request Jul 20, 2016
2 tasks
@pjcozzi
Copy link
Contributor

pjcozzi commented Jul 20, 2016

Thanks @lasalvavida.

@lilleyse can you merge master into 3d-tiles to make @lasalvavida next merge clean?

@pjcozzi pjcozzi merged commit 62c3d28 into CesiumGS:master Jul 20, 2016
@pjcozzi pjcozzi deleted the snorm-range-max branch July 20, 2016 20:12
@lilleyse
Copy link
Contributor

@lilleyse can you merge master into 3d-tiles to make @lasalvavida next merge clean?

Ok

@lilleyse
Copy link
Contributor

Ready now.

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.

4 participants