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

Compressing integer accessors #121

Merged
merged 10 commits into from
Jul 15, 2016
Merged

Compressing integer accessors #121

merged 10 commits into from
Jul 15, 2016

Conversation

lasalvavida
Copy link
Contributor

@lasalvavida lasalvavida commented Jun 30, 2016

For #120

  • Add utility for accessor type (this should exclude index accessors and doesn't currently) - just having bufferView and buffer accessible on AccessorReader is good enough. You can check the bufferView target from there.
  • Make functions that iterate over accessors use the added AccessorReader - Some of removedUnusedVertices was left alone because it is faster to copy data in chunks
  • Tests

Results look good except for cases where the first checkmark is an issue.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.2%) to 93.894% when pulling 650e38f on compressIntegerAccessors into 5a7bdda on master.

@lasalvavida lasalvavida changed the title Skeleton for compressing integer accessors Compressing integer accessors Jun 30, 2016
@lasalvavida
Copy link
Contributor Author

Results:

Box.gltf CesiumMan.gltf BrainStem.gltf
Original 8.10 KB 1.55 MB 15.0 MB
w/o compressIntegerAccessors 6.54 KB 716 KB 4.33 MB
w/ compressIntegerAccessors 6.25 KB 662 KB 3.27 MB

All models render correctly and this compression is completely lossless!
Just needs tests

@lilleyse
Copy link
Contributor

Nice results! I'll check the code out when everything is ready.

@lasalvavida
Copy link
Contributor Author

@lilleyse This should be ready for a look. Not super high priority, but it would be nice to get into master since it comes with a decent amount of cleanup as well.

@lilleyse
Copy link
Contributor

Using -q is creating broken models on this branch.
capture5

@pjcozzi
Copy link
Contributor

pjcozzi commented Jul 14, 2016

For Box.gltf above (#121 (comment)), why is it smaller with integer compression? What integer attributes did it have?

@lasalvavida
Copy link
Contributor Author

lasalvavida commented Jul 14, 2016

For Box.gltf above (#121 (comment)), why is it smaller with integer compression? What integer attributes did it have?

Its positions and normals are all made up of 0, 1 or -1, which can be stored as a byte instead of a float.

edit: actually just the normals

@lasalvavida
Copy link
Contributor Author

Good catch @lilleyse! This was an issue with AccessorReader that should be fixed now

@pjcozzi
Copy link
Contributor

pjcozzi commented Jul 14, 2016

For Box.gltf above (#121 (comment)), why is it smaller with integer compression? What integer attributes did it have?

It's positions and normals are all made up of 0, 1 or -1, which can be stored as a byte instead of a float.

Ah, OK. Then it is rendered with bytes, right?

This is fine for now, but I would not be surprised if this is slower to render on many GPUs or if the data is padded / converted when uploaded to the WebGL buffer (e.g., we said bytes, but the driver converts to floats to get the fast path).

No need to do performance testing now since this is an edge case for most models.

@lasalvavida
Copy link
Contributor Author

Yes, it is rendered with bytes. Definitely an edge case though; this is mostly for compressing JOINT that get stored as floats. I just wanted to show that rendering still worked for other semantics.

@pjcozzi
Copy link
Contributor

pjcozzi commented Jul 14, 2016

All good with me, what's left before we can merge this? #121 (comment)?

@lasalvavida
Copy link
Contributor Author

I don't have anything left on my list; should be good to merge.

@pjcozzi
Copy link
Contributor

pjcozzi commented Jul 14, 2016

@lilleyse when you are ready...

@lilleyse
Copy link
Contributor

I confirmed that quantization works normally now.

@lilleyse lilleyse merged commit 98fe338 into master Jul 15, 2016
@lilleyse lilleyse deleted the compressIntegerAccessors branch July 15, 2016 15:44
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