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

Chore: Update document for MIPMAP_TEXTURES and MIPMAP_MODES #8781

Merged
merged 3 commits into from
Oct 25, 2022
Merged

Conversation

SuperSodaSea
Copy link
Member

@SuperSodaSea SuperSodaSea commented Oct 24, 2022

Description of change

Some document update for MIPMAP_TEXTURES and MIPMAP_MODES.

Pre-Merge Checklist
  • Documentation is changed or added
  • Lint process passed (npm run lint)
  • Tests passed (npm run test)

@SuperSodaSea
Copy link
Member Author

@bigtimebuddy Do you think this change is OK for v6.x, or only for v7?

@bigtimebuddy
Copy link
Member

Whenever we change default settings that could have some impact on users, I'd consider that breaking (similar to #7974). So I would say v7 for this.

@bigtimebuddy bigtimebuddy added this to the v7.0.0 milestone Oct 24, 2022
@GoodBoyDigital
Copy link
Member

Hey @SuperSodaSea , thanks for the PR!

What would be your thoughts around mipmaps being turned off by default for all textures rather than on? Just thinking about the memory bandwidth we could save people..

@bigtimebuddy
Copy link
Member

I'd agree with Mat on this one. The negative memory impact seems substantial enough to keep it as-is.

For the original issue, the user has two very viable options:

  1. Enable MIPMAPS by for all textures settings.MIPMAP_TEXTURES = MIPMAP_MODES.ON
  2. Only use POT sizes for BitmapFonts

@SuperSodaSea
Copy link
Member Author

That make sence. So should I keep the current default value POW2 or change it to OFF?

@bigtimebuddy
Copy link
Member

There are numerous places in Pixi with defaults where we need to balance good rendering with fast performance. This is one of those places. I would vote to keep it as-is (POW2) since I think it's representative of the best practices for Pixi (eg using pow2 spritesheets) over arbitrary sized assets (background images, text). It may not be the best for all scenarios, but that's why we expose a global default and have a local mipmap option on BaseTexture to override per asset.

@SuperSodaSea SuperSodaSea changed the title Change default value of MIPMAP_TEXTURES to ON Chore: Update document for MIPMAP_TEXTURES and MIPMAP_MODES Oct 25, 2022
@SuperSodaSea
Copy link
Member Author

OK, now I reverted the default value to POW2 and only keep the necessary changes in documents.

@bigtimebuddy bigtimebuddy added the ✅ Ready To Merge Helpful when issues are in the queue waiting to get merged. This means the PR is completed and has t label Oct 25, 2022
@bigtimebuddy bigtimebuddy merged commit 1aa827d into dev Oct 25, 2022
@bigtimebuddy bigtimebuddy deleted the fix/8775 branch October 25, 2022 20:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
✅ Ready To Merge Helpful when issues are in the queue waiting to get merged. This means the PR is completed and has t
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants