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

Clean up and optimize BasisUniversal #88464

Merged
merged 2 commits into from
Feb 19, 2024

Conversation

BlueCube3310
Copy link
Contributor

This PR aims to clean up BasisUniversal's code to make it easier to read, as well as extracts the code from register_types.cpp into its own files.

It also slightly optimizes the memory usage and compression times by using the get_mipmap_offset_size_and_dimensions() function instead of copying each mipmap's data into a new image.

While it seems to function correctly, it still needs some testing, especially on mobile devices.

@fire
Copy link
Member

fire commented Feb 18, 2024

I'm generally ok with this but I agree that testing on mobile devices is required.

@akien-mga
Copy link
Member

Unlike our usual practice, such a change would be much easier to review if done as two separate commits: one to move the code to a separate file, and one to actual do logic changes. With both types of changes made in the same commit it's pretty difficult to review.

@BlueCube3310
Copy link
Contributor Author

The commits are now split, as suggested.

Copy link
Member

@akien-mga akien-mga left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks really good!

@akien-mga akien-mga modified the milestones: 4.x, 4.3 Feb 19, 2024
modules/basis_universal/image_compress_basisu.cpp Outdated Show resolved Hide resolved
modules/basis_universal/image_compress_basisu.h Outdated Show resolved Hide resolved
modules/basis_universal/image_compress_basisu.cpp Outdated Show resolved Hide resolved
@akien-mga akien-mga merged commit aa414ab into godotengine:master Feb 19, 2024
16 checks passed
@akien-mga
Copy link
Member

Thanks!

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.

5 participants