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

Major compression of assets textures #156

Closed
wants to merge 0 commits into from
Closed

Major compression of assets textures #156

wants to merge 0 commits into from

Conversation

riccardogiorato
Copy link
Contributor

MAJOR changes
I compressed all models textures as I already explained in this issue: Compressing assets.

Minor changes

@toji
Copy link
Member

toji commented Jan 24, 2020

Thanks for the contribution! This seems like a valuable change, given that we expect these meshes to be downloaded a lot. I do have some questions/concerns, though:

I don't believe that we can add the dist/ folders to .gitignore, because it would prevent our gh-pages deployment step from picking them up, and we need that for the viewer.

I tried reading up on tinypng, but I'm still fuzzy on a couple of things. It is a lossy compression algorithm, right? While that is probably OK for base color maps I'm concerned that it would introduce noticable artifacts into non-sRGB inputs, such as normal maps or metallic-roughness maps. Also, does repeatedly running tinypng on an image cause stacking artifacts? And just to verify, the compressed files are still just standard PNGs, right?

To answer a question from the related issue: I would be totally fine with changing any JPG textures to PNG as long as it doesn't significantly increase file size. Many of the textures have large sections of flat or similar colors, so I suspect that we should be OK on that front with the tinypng compression.

As for the http-server change, I'd prefer that be separated out into a separate PR, just to keep this one focused on the asset change.

Finally, I'm afraid that a change I made this morning has introduced a conflict in this path

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.

2 participants