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 #12220: Add Decompress Bc5 to Squish #13025

Closed
wants to merge 1 commit into from
Closed

Fix #12220: Add Decompress Bc5 to Squish #13025

wants to merge 1 commit into from

Conversation

gad-o
Copy link
Contributor

@gad-o gad-o commented Nov 18, 2017

This Commit fixes the corrupted file preview described in #12220.
Added DecompressColourBc5 function to squish.

This Commit fixes the corrupted file preview described in #12220.
Added DecompressColourBc5 function to squish.
@reduz
Copy link
Member

reduz commented Nov 19, 2017 via email

@gad-o
Copy link
Contributor Author

gad-o commented Nov 19, 2017

Thanks!
Actualy this is a partialy solution. I think, we should implement bc4 compression also...

@vnen vnen added this to the 3.0 milestone Nov 19, 2017
@ghost ghost requested a review from akien-mga November 19, 2017 00:56
@akien-mga
Copy link
Member

akien-mga commented Nov 19, 2017

So we're forking libsquish?..

Have you tried to propose this change upstream?

Edit: They don't seem to have a public issue tracker, so you'd have to send your patch directly to the maintainers listed in their README: https://sourceforge.net/projects/libsquish/files/

Godot's policy is to not modify our thirdparty library unless there is really no way around it and no possibility to contribute our enhancements and bug fixes directly upstream.

@gad-o
Copy link
Contributor Author

gad-o commented Nov 19, 2017

I try solve without touch squish but you can see actually Bc5 and Probably bc4 not working.

Yes, i search for issue tracker or push for upstream, i can't find any way to how contirubute.
Also try find another project that uses libsuqish for how they resolve these decompression but cant find. I think there is no way arround it but fork or wait for upstream. What i see, this is critical for godot and godot have more fast development cycle. I'm not sure about is development libsquish is going on.
I think we should fork library and send patch if it work. Or wait for bc5 compression became mature

@ghost
Copy link

ghost commented Nov 20, 2017

How about maintaining this as a patch and maybe wrap it in between markers like /* GODOT start */ until we get this into upstream (which might take some time)? We already have this kind of stuff in third party.

@akien-mga
Copy link
Member

akien-mga commented Nov 20, 2017

How about maintaining this as a patch and maybe wrap it in between markers like /* GODOT start */ until we get this into upstream (which might take some time)? We already have this kind of stuff in third party.

That's what should be done, yes, but I want to see a discussion with upstream at least start, so far I see no indication that contact with them was made. As upstream dev for various projects, I know that it's very important that downstream users tell you when they want API changes, even more so if they have patches for it already :)

@gad-o
Copy link
Contributor Author

gad-o commented Nov 20, 2017

That's what should be done, yes, but I want to see a discussion with upstream at least start, so far I see no indication that contact with them was made. As upstream dev for various projects, I know that it's very important that downstream users tell you when they want API changes, even more so if they have patches for it already :)

You're right. I will try contanct with them.

@akien-mga
Copy link
Member

Thanks! Feel free to put me in CC with my email from https://godotengine.org/contact

@akien-mga
Copy link
Member

Any news from the Squish devs?

@gad-o
Copy link
Contributor Author

gad-o commented Dec 9, 2017

I put you on cc; since than, no news.

@akien-mga
Copy link
Member

Seems like things might take a while with upstream, so I merged this manually with comments to properly show what we did: e021097

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants