Skip to content

Cleanup Texture2DConverter and fix typos in pyproject.toml #324

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

isHarryh
Copy link
Contributor

@isHarryh isHarryh commented Jun 1, 2025

Summary

This PR does some minor changes to Texture2DConverter.

  • platform_blob can be List[int] in newer version of Unity, instead of only bytes.
  • One platform_blob type hint still uses bytes, now corrected to List[int].
  • if TYPE_CHECKING: assert xxx is not None seems redundant, now removed if.
  • Pre-converts platform to BuildTarget enum.
  • Wraps swizzle texture_format conversion into TextureSwizzler.
  • Simplifies the redundant if-statements in image_to_texture2d.
  • Optimizes docstrings and typos.

Along with two typo fixes in the pyproject.toml:

  • Fixes the invalid license field in pyproject.toml which causes CodeQL failing.
  • Fixes the typo of the keyword data-minig in pyproject.toml.

@isHarryh isHarryh force-pushed the patch-texture-type branch from 6660d27 to 3517281 Compare June 6, 2025 02:02
Copy link
Owner

@K0lb3 K0lb3 left a comment

Choose a reason for hiding this comment

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

Sorry that I'm only replying now.

I will be blunt, I don't really see the point of this PR.

The typo fixes, doc string adjustments and the BC format collapsing are okay, but that's it.

TextureSwizzler.get_corrected_texture_format is basically a one-time use function,
which only adds a layer of indirection without any benefit, as at one of the two places where it's being used, the same check has to be done again anyway.

TextureSwizzler.PlatformBlobType is also not really useful.
The usage of bytes from before was actually wrong, as the platformblob type is defined by the Texture2D field of it, so just List[int] is enough.

The platform type also doesn't have to be narrowed down to BuildTarget,
as the only use of platform is in an equality check, which works just fine with an int as well.

@isHarryh isHarryh force-pushed the patch-texture-type branch from 3517281 to 9178ed7 Compare June 6, 2025 09:58
@isHarryh
Copy link
Contributor Author

isHarryh commented Jun 6, 2025

Apologies - I didn’t fully considered the current needs, so I’ve decided to go ahead and revert the 3 changes you mentioned, to narrow the PR to only typo fixes.

My initial intention with these changes was to centralize the logic and make the code more readable in the long run. But I understand that, the added abstraction doesn’t bring enough value and instead introduces unnecessary complexity.

Thanks again for your constructive feedback - it’s much appreciated.

@isHarryh
Copy link
Contributor Author

isHarryh commented Jun 6, 2025

This PR now contains:

  1. Typo and docstring adjustments.
  2. BC format if-statements collapsing.

with some new changes:

  1. One platform_blob type hint still uses bytes, now corrected to List[int].
  2. if TYPE_CHECKING: assert xxx is not None seems redundant, now removed if.
  3. Fixes the invalid license field in pyproject.toml which causes CodeQL failing.
  4. Fixes the typo of the keyword data-minig in pyproject.toml.

@isHarryh isHarryh changed the title Refactor swizzle process and cleanup Texture2DConverter Cleanup Texture2DConverter and fix typos in pyproject.toml Jun 6, 2025
@isHarryh isHarryh requested a review from K0lb3 June 6, 2025 10:14
@isHarryh isHarryh force-pushed the patch-texture-type branch from 9178ed7 to 17f581b Compare June 6, 2025 12:41
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants