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

Fix K-bits range for meshopt octahedral filter #2463

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

lilleyse
Copy link
Contributor

Looking at meshopt_encodeFilterOct as a reference, the bit range should be 1 <= K <= 16, not 4 <= K <= 16.

@zeux does this change look correct?

@zeux
Copy link
Contributor

zeux commented Dec 17, 2024

While 1 is the smallest possible K from the encoding perspective, unsure if the spec should state that as the bounds - a 1-bit signed normalized integer has two values, -1 and 0, so that's not super useful for normal encoding :) My recollection for why the spec stated K=4 is the minimum was that I felt that this is the lowest value assets could be reasonably encoded in (at a great quality loss!), but maybe this is too conservative. When normals are used for specular reflections, Suzanne from glTF-Sample-Assets looks broken on K=3:

K=4
image

K=3
image

... but BrainStem model is perhaps okay for some contexts with K=3:

image

... or K=2

image

A 2-bit signed normalized integer can represent values -1, 0, 1 in [-1, 1] range. So if we want to update the spec then minimum K should probably start with 2; K=1 is not useful.

@emackey
Copy link
Member

emackey commented Feb 11, 2025

@lilleyse What's the use case here, is it 3D Tiles with unlit and hence no normals are being used? What's the next step here?

@lexaknyazev
Copy link
Member

It's a request to align the extension spec with the implementation. I think the next step is to update the PR to use 2 as the lower bound.

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