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

Use libdragon compression scheme #817

Open
wants to merge 13 commits into
base: develop/2.4.0
Choose a base branch
from

Conversation

mountainflaw
Copy link
Contributor

Uses devwizard's assembly decompressor and Wiseguy's compressor tool. This would replace yay0 with yaz0 with zero downsides.

@mountainflaw mountainflaw changed the base branch from master to develop/2.4.0 July 9, 2024 18:17
@mountainflaw mountainflaw marked this pull request as ready for review July 9, 2024 18:29
@mountainflaw mountainflaw requested a review from gheskett as a code owner July 9, 2024 18:29
@mountainflaw
Copy link
Contributor Author

As posted in Discord, the performance improvement seems to be around 9.61% faster level loads with a slightly smaller ROM size.

image

The left is YAZ0 and the right is YAY0.

Copy link
Collaborator

@gheskett gheskett left a comment

Choose a reason for hiding this comment

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

Replacing LOAD_YAY0 with LOAD_YAZ0 only serves to introduce merge conflicts and break fast64 compatibility. If we're replacing yay0, then changing the level script command doesn't matter other than to not be misleading (which is already the case with MIO0; maybe add a comment explaining the reality to the macro definitions or something idk).

@mountainflaw
Copy link
Contributor Author

Replacing LOAD_YAY0 with LOAD_YAZ0 only serves to introduce merge conflicts and break fast64 compatibility. If we're replacing yay0, then changing the level script command doesn't matter other than to not be misleading (which is already the case with MIO0; maybe add a comment explaining the reality to the macro definitions or something idk).

or fix fast64?

@gheskett
Copy link
Collaborator

Not worth it tbh; that's its own logistical headache even with a HackerSM64 checkbox (that still isn't implemented).

@mountainflaw
Copy link
Contributor Author

Not worth it tbh; that's its own logistical headache even with a HackerSM64 checkbox (that still isn't implemented).

how is it a logistical headache when you can make it equivalent to the current macro?

@gheskett
Copy link
Collaborator

gheskett commented Jul 10, 2024

By being incompatible with HackerSM64 2.3; this alone means destroying HackerSM64 backwards compatibility (on fast64's end).

@mountainflaw
Copy link
Contributor Author

By being incompatible with HackerSM64 2.3; this alone means destroying HackerSM64 backwards compatibility (on fast64's end).

Don't most people auto update fast64?

@gheskett
Copy link
Collaborator

No probably, but that doesn't matter; HackerSM64 is not the source of truth for exporting defaults, nor should it attempt to be wherever possible

@gheskett
Copy link
Collaborator

Regardless, adding level script merge conflicts here would also put this PR well outside of 2.4 scope

include/level_commands.h Outdated Show resolved Hide resolved
Copy link
Collaborator

@gheskett gheskett left a comment

Choose a reason for hiding this comment

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

Wait nevermind, the segment names all have yaz0 suffixes. Double reject.

@mountainflaw
Copy link
Contributor Author

I undid all of the renames.

@mountainflaw mountainflaw changed the title Replace yay0 with yaz0 Use libdragon compression scheme Jul 12, 2024
@mountainflaw
Copy link
Contributor Author

mountainflaw commented Jul 12, 2024

As per usual with my pull requests, this will be experiencing some scope creep. I've decided it's probably best to just ditch the current compression options entirely, and replace them with their libdragon equivalents rather than just add aPLib and YAZ0.

I plan to actually ditch YAZ0 and move forward with importing LZ4 as its replacement, and replace deflate with shrinkler.

By the end, we should just have uncomp, lz4, aplib, and shrinkler as the four options ("levels" of compression in libdragon) to use, and as stated in Discord, only with upsides over the current choices in the repository.

@gheskett gheskett added questionable This is risky or potentially invalid or undesirable monkaS monkaS labels Aug 28, 2024
@gheskett gheskett requested a review from arthurtilly September 5, 2024 04:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
monkaS monkaS questionable This is risky or potentially invalid or undesirable
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants