-
Notifications
You must be signed in to change notification settings - Fork 705
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
Conversation
…worlds.module/subpath/to/data.png that use pkgutil to load files from within an apworld
kvui.py
Outdated
def load_override(filename, default_load=DefaultLoad, **kwargs): | ||
if filename[:3] == "ap:": | ||
return ImageLoaderPkgutil(filename) | ||
else: | ||
return default_load(filename, **kwargs) |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
Co-authored-by: Doug Hoskisson <[email protected]>
kvui.py
Outdated
|
||
|
||
# grab the default loader method so we can override it but use it as a fallback | ||
DefaultLoad = ImageLoader.load |
There was a problem hiding this comment.
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
Co-authored-by: Doug Hoskisson <[email protected]>
There was a problem hiding this 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.
There was a problem hiding this 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
Is this something that we want to document in https://github.com/ArchipelagoMW/Archipelago/blob/main/docs/apworld%20specification.md ? Mentioning #3387 for posterity |
There was a problem hiding this 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.
bad news: I have learned that PIL is not included in frozen so this did not function when frozen retested the same apAsyncImage_test branch on source and frozen and now it works in both |
Great feature, especially for custom/unsupported apworlds! |
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:and all displayed the icon, also tested
If this makes graphical changes, please attach screenshots.
Testing notes
feel free to reference the branch https://github.com/qwint/Archipelago/tree/apAsyncImage_test to see the ahit changes for testing