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

Launcher: support Component icons inside apworlds #3629

Merged
merged 7 commits into from
Nov 30, 2024

Conversation

qwint
Copy link
Contributor

@qwint qwint commented Jul 9, 2024

What is this fixing or adding?

Add kivy overrides to allow AsyncImage source paths of the format ap:worlds.module/subpath/to/data.png that use pkgutil to load files from within an apworld

How was this tested?

locally edited the a hat in time world to use icon_paths['yatta'] = f"ap:{__name__}/data/yatta.png" instead of the existing icon_paths value, moving said png to inside a "data" folder in the world, and opened the launcher with:

  • the world unzipped
  • the world zipped as an apworld in /worlds
  • the world zipped as an apworld in /custom_worlds

and all displayed the icon, also tested

  • with the original icon, loaded quickly with no issue
  • with an upscaled icon, which tripped zipbomb protection and did not load
  • with a less upscaled icon, which took a few seconds to load, in which the launcher opened, displayed a loading icon, and was interactable until the image finished loading and replaced the loading icon as expected

If this makes graphical changes, please attach screenshots.

image

Testing notes

feel free to reference the branch https://github.com/qwint/Archipelago/tree/apAsyncImage_test to see the ahit changes for testing

…worlds.module/subpath/to/data.png that use pkgutil to load files from within an apworld
@github-actions github-actions bot added affects: core Issues/PRs that touch core and may need additional validation. waiting-on: peer-review Issue/PR has not been reviewed by enough people yet. labels Jul 9, 2024
kvui.py Outdated Show resolved Hide resolved
kvui.py Outdated Show resolved Hide resolved
kvui.py Outdated
Comment on lines 803 to 807
def load_override(filename, default_load=DefaultLoad, **kwargs):
if filename[:3] == "ap:":
return ImageLoaderPkgutil(filename)
else:
return default_load(filename, **kwargs)
Copy link
Collaborator

Choose a reason for hiding this comment

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

If we're overriding load, why aren't we returning the same type that load returns?
It looks like the default load returns an instance of Image, while the new load would return an instance of ImageLoaderBase (which itself has a load function that returns a list of ImageData).
Can you explain how this works?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

if ImageLoader.load() needs to load asynchronously it returns an ImageLoaderBase that can handle the request
https://github.com/kivy/kivy/blob/master/kivy/core/image/__init__.py#L454
this is mimicking that workflow but just short-circuiting it as early as possible because the current loader picking code in kivy cannot handle the information we need in the filename

kvui.py Outdated Show resolved Hide resolved
kvui.py Outdated Show resolved Hide resolved
kvui.py Outdated


# grab the default loader method so we can override it but use it as a fallback
DefaultLoad = ImageLoader.load
Copy link
Collaborator

Choose a reason for hiding this comment

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

variable name

  • This isn't a type, so this isn't the right capitalization pattern for it.
  • Is there any reason for people to import this from the module and use it directly? If not it should begin with underscore (_)

maybe _original_load or _original_image_load or _original_image_loader_load

kvui.py Outdated Show resolved Hide resolved
kvui.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@ScipioWright ScipioWright left a comment

Choose a reason for hiding this comment

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

Can confirm it works, making a similar change in the Animal Well apworld.
While I don't understand the code changes all that much, the syntax looks fine to me.

Copy link
Contributor

@benny-dreamly benny-dreamly left a comment

Choose a reason for hiding this comment

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

read the code and it looks pretty good. I don't know kivy at all, but I do know how to use PIL to display images, and that syntax looks correct. otherwise LGTM.

didn't test it however

@nicholassaylor
Copy link
Contributor

Is this something that we want to document in https://github.com/ArchipelagoMW/Archipelago/blob/main/docs/apworld%20specification.md ?

Mentioning #3387 for posterity

@Exempt-Medic Exempt-Medic added the is: enhancement Issues requesting new features or pull requests implementing new features. label Jul 10, 2024
@qwint qwint requested a review from beauxq July 27, 2024 16:28
Copy link
Collaborator

@beauxq beauxq left a comment

Choose a reason for hiding this comment

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

I didn't test it, but I looked over the code a bit.

@ScipioWright ScipioWright added waiting-on: core-review Issue/PR has been peer-reviewed and is ready to be merged or needs input from a core maintainer. and removed waiting-on: peer-review Issue/PR has not been reviewed by enough people yet. labels Jul 28, 2024
@qwint
Copy link
Contributor Author

qwint commented Aug 13, 2024

bad news: I have learned that PIL is not included in frozen so this did not function when frozen
good news: I have learned how to find a native kivy loader that can load a bytes io object without needing a path on disk

retested the same apAsyncImage_test branch on source and frozen and now it works in both

@massimilianodelliubaldini

Great feature, especially for custom/unsupported apworlds!

@NewSoupVi NewSoupVi merged commit a537d8e into ArchipelagoMW:main Nov 30, 2024
17 checks passed
@qwint qwint deleted the pkgutil_launcher_icons branch November 30, 2024 02:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
affects: core Issues/PRs that touch core and may need additional validation. is: enhancement Issues requesting new features or pull requests implementing new features. waiting-on: core-review Issue/PR has been peer-reviewed and is ready to be merged or needs input from a core maintainer.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants