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

Simplify pygame resource loaders #2695

Merged
merged 8 commits into from Oct 23, 2021
Merged

Simplify pygame resource loaders #2695

merged 8 commits into from Oct 23, 2021

Conversation

Starbuck5
Copy link
Contributor

@Starbuck5 Starbuck5 commented Aug 25, 2021

In my journey of #2694, I realized that almost every resource loading function in pygame duplicates a lot of the same code, and they are very complex.

But I noticed there was one function provided by rwobject.c that could simplify a lot of this down, pgRWops_FromObject. It can handle string filenames and file like objects.

The reason it hasn't been used much is that lots of the resource loaders either need to pass the filepath to SDL, or the type / extension of the file. And you can't get the extension out of an SDL_RWops.

So all our resource loaders would re-do all the functionality nicely wrapped up in pgRWops_FromObject just to have the actual name of the file, and use the non-RW loader to load directly from a file name.

To simplify this, I made resource loaders that use SDL_RWops unconditionally, and changed rwobject.c to save the extension if loading an SDL_RWops from a real file. I then added the pgRWops_GetFileExtension function to allow the resource loaders to access the information.

Here are the 7 resource loading functions in pygame's C code:

mixer.Sound              - used pgRWops_FromObject already
image.load_basic         - converted in this PR (easily, because it didn't need the file type, saved 15 lines)
image.load_extended      - converted in this PR (saved 130 lines)
mixer.music.load         - converted in this PR (saved 19 lines)
mixer.music.queue        - can't mess with this one, because #2558 is pending
freetype.Font            - scared to mess with this one
font.Font                - scared to mess with this one

I would like to eventually hit freetype.Font and font.Font, along with perhaps image.save (looks really complex), but this PR will be hard enough to review with just the changes I've done already.

Oh, and this will make the errors from #2694 be emitted by mixer.music and image, alongside mixer.

src_c/rwobject.c Outdated Show resolved Hide resolved
Copy link
Member

@illume illume left a comment

Choose a reason for hiding this comment

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

Apart from the issue ankith26 found, LGTM.

🎉 thanks!

@illume illume requested a review from ankith26 October 6, 2021 10:50
Copy link
Contributor

@ankith26 ankith26 left a comment

Choose a reason for hiding this comment

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

Yeah, looks good now! 🎈

@Starbuck5 Starbuck5 added this to the 2.0.3 milestone Oct 8, 2021
@illume illume added image pygame.image mixer pygame.mixer mixer.music pygame.mixer.music rwops SDL's IO loading/streaming code labels Oct 16, 2021
Copy link
Member

@illume illume left a comment

Choose a reason for hiding this comment

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

👍 🎉

@illume
Copy link
Member

illume commented Oct 16, 2021

I'll fix the conflict, because I merged in the other PR which caused it.

Copy link
Member

@illume illume left a comment

Choose a reason for hiding this comment

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

After merging main into this because of the conflict I noticed an issue.

This changes the API for the file loaders, because different exceptions are raised.

Code that checks for pygame.error will now not work, because things like IOError will be raised instead.

What to do?

  • changes need to be documented where loading happens
  • we need to check the impact of this API change. Does it break much? How common is catching pygame.error here? Then we need to decide if it's worth the break (probably is because nice error messages are good... but if it breaks 50% of programs... maybe not.)
  • Is there a way to keep the good exceptions/messages but also keep compatibility? (maybe an exception that inherits from both? Something else?)

@illume illume modified the milestones: 2.0.3, 2.1 Oct 16, 2021
@Starbuck5
Copy link
Contributor Author

I want this to happen in 2.0.3, not 2.0.1.

The only API changed is mixer.music.load and mixer.music.queue, so I suppose they could be "fixed up" to return the content as a pygame.error instead.

Also, IOError is python 2 only, I think. FileNotFoundError is the way now.

@Starbuck5 Starbuck5 modified the milestones: 2.1, 2.0.3 Oct 18, 2021
@Starbuck5
Copy link
Contributor Author

@illume, I fixed it so it can go in with no API change!

Rather than passing along the error it gets from the files, it reads that error and retransmits it as a pygame.error.

If this is merged we should raise an issue so we don't forget to change this in 2.1.

I also removed the macOS test mp3 skip, because it is no longer necessary.

Copy link
Member

@illume illume left a comment

Choose a reason for hiding this comment

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

Amazing that this can go in AND be backwards compatible. Thanks a lot. 🎉

@illume illume merged commit a0521b4 into pygame:main Oct 23, 2021
@Starbuck5 Starbuck5 deleted the rwops branch May 15, 2022 23:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
image pygame.image mixer.music pygame.mixer.music mixer pygame.mixer rwops SDL's IO loading/streaming code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants