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

Float16BufferAttribute: Unpack/pack float16's in getters/setters. #25519

Merged
merged 4 commits into from
Feb 24, 2023

Conversation

simondevyoutube
Copy link
Contributor

@simondevyoutube simondevyoutube commented Feb 16, 2023

Fixed #25517

Description

As far as I can tell, when using half floats for position, unpacking the attribute buffer to compute the bounding volume doesn't do the half float conversion. You get the uint16 versions of the float16's, a bounding volume is computed off those, which will be really off from the actual one and the object may be culled mistakenly,

I've updated the getters/setters for Float16BufferAttribute along with tests that looked relevant, ptal? Hoping I got this right, first PR on github

@Mugen87
Copy link
Collaborator

Mugen87 commented Feb 17, 2023

Seeing your proposed code, I'm glad we have decided to introduce Float16BufferAttribute and not BufferAttribute.useHalfFloat (see #15480). A separate class allows us to implement the conversion logic without cluttering BufferAttribute.

We just have to note in the migration guide that half float conversion is now done automatically and does not to be performed on app level anymore. Otherwise the conversion happens twice.

@Mugen87 Mugen87 added this to the r150 milestone Feb 17, 2023
@Mugen87 Mugen87 changed the title Unpack/pack float16's in Float16BufferAttribute in getters/setters Float16BufferAttribute: Unpack/pack float16's in in getters/setters. Feb 17, 2023
@simondevyoutube
Copy link
Contributor Author

Yeah that's a good point, I didn't think about whether client code would be using the setters to populate the buffer, so yeah in that case now it'll do something different and potentially break existing code. Although, if I'm the first to notice this, maybe this isn't a commonly used path?

Not totally sure what the right solution is here. Removing the setters would be compatible with how it worked before, but then maybe it's unexpected to have the getter's do an unpack.

Maybe down the line, the better approach would be to have an iterator type of interface that does conversions, that way someone using a crazier format like packing positions into fixed point could provide an interface. Just rambling at this point.

@Mugen87
Copy link
Collaborator

Mugen87 commented Feb 18, 2023

For now I prefer the solution for your PR. The half float conversion is ideally something the engine should care about. As you pointed out, without it other functionality like bounding volume computation, frustum culling or even ray casting breaks.

@Mugen87
Copy link
Collaborator

Mugen87 commented Feb 18, 2023

BTW: The best solution would be Float16Array support in JavaScript 😁 .

@LeviPesin
Copy link
Contributor

Shouldn't we move this code into Float16BufferAttribute? (Also, there are some linting problems in added tests)

@simondevyoutube
Copy link
Contributor Author

Definitely having Float16Array would be awesome.

LeviPesin: The code that I added should be in Float16Attribute already, unless I missed something. Also, can you point out the linting problems? I ran the linter as per the contribution guidelines and thought I fixed any issues in my files.

@LeviPesin
Copy link
Contributor

The code that I added should be in Float16Attribute already, unless I missed something. 

Sorry, I didn't noticed it is file BufferAttribute.js but Float16BuffferAtrribute class.

Also, can you point out the linting problems?

Things like f16Array[i] (which should be f16Array[ i ]).

@simondevyoutube
Copy link
Contributor Author

The code that I added should be in Float16Attribute already, unless I missed something.

Sorry, I didn't noticed it is file BufferAttribute.js but Float16BuffferAtrribute class.

Also, can you point out the linting problems?

Things like f16Array[i] (which should be f16Array[ i ]).

Cool, uploaded a new CL with those changes, ptal

@CodyJasonBennett
Copy link
Contributor

BTW: The best solution would be Float16Array support in JavaScript 😁.

Let's hope: gpuweb/gpuweb#3848

@mrdoob mrdoob modified the milestones: r150, r151 Feb 23, 2023
@Mugen87 Mugen87 merged commit 2b54088 into mrdoob:dev Feb 24, 2023
@Mugen87 Mugen87 changed the title Float16BufferAttribute: Unpack/pack float16's in in getters/setters. Float16BufferAttribute: Unpack/pack float16's in getters/setters. Feb 24, 2023
@Methuselah96 Methuselah96 mentioned this pull request Apr 25, 2023
37 tasks
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.

Half Float position doesn't render
5 participants