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

bevy_image: PCX image support (off by default) #16207

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

lupine
Copy link

@lupine lupine commented Nov 1, 2024

Objective

PCX is an obsolete raster image format from 1985. However, it was popular in a range of popular Windows games throughout the 1990s, and is in every respect a "proper" format.

I have a hobby game engine recreation project, which is currently written in Go ( https://code.ur.gs/lupine/ordoor ). I'm looking to rewrite it in rust, using Bevy. The first step is support for the various file formats this game needs.

Because I'm dealing with proprietary assets for a game that's still on sale, it's cleanest all round if I avoid processing or rewriting them in any way. So, I need bevy to understand what a pcx file is and load it (from a custom asset source, incidentally, although that doesn't matter too much for this PR).

Solution

Although I could achieve this without changing bevy through a custom asset loader (and intend to follow that route for a whole set of other file formats), the image -> texture support seems to be handled automatically if bevy_image just supports the pcx format natively, so this PR adds support for that - off by default, with the pcx feature flag to turn it on.

A pcx crate already exists in the Rust ecosystem, and I recently got support wiring that up to the image crate merged: image-rs/image#2364 . So all we're doing here is wiring the image-rs pcx code up to Bevy.

PCX images come in a range of colour formats; as an implementation detail, the upstream image-rs code takes paletted images and fakes them into 24-bit RGB, so they work fine as bevy textures.

Fair warning, there are some other "proper" image formats I've got my eye on - particularly ANI and CUR - if there's no objection to old formats in general!

Testing

In progress. I just wanted to get this PR up early for comment. I assume we'd want to wait for image-rs to tag a new release before merging as well, but it would be good to get thoughts on whether bevy wants to support pcx early on.

I've had this compile and run end-to-end locally, leading to bevy loading a set of .pcx files into memory, but I've not made it display on screen yet. I'll definitely do that before taking this off draft.

  • Did you test these changes? If so, how?
  • Are there any parts that need more testing?
  • How can other people (reviewers) test your changes? Is there anything specific they need to know?
  • If relevant, what platforms did you test these changes on, and are there any important ones you can't test?

Copy link
Contributor

github-actions bot commented Nov 1, 2024

Welcome, new contributor!

Please make sure you've read our contributing guide and we look forward to reviewing your pull request shortly ✨

Copy link
Contributor

github-actions bot commented Nov 1, 2024

You added a new feature but didn't update the readme. Please run cargo run -p build-templated-pages -- update features to update it, and commit the file change.

@lupine
Copy link
Author

lupine commented Nov 1, 2024

the pcx crate is WTFPL, which is a very permissive license that splits floss people slightly. FSF don't mind it, google do mind it 🤷

@alice-i-cecile alice-i-cecile added C-Feature A new feature, making something new possible A-Rendering Drawing game state to the screen X-Contentious There are nontrivial implications that should be thought through D-Straightforward Simple bug fixes and API improvements, docs, test and examples S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Nov 3, 2024
@alice-i-cecile
Copy link
Member

@superdump @IceSentry @robtfm do any of you have opinions / objections here? I'm personally fine with this on an off-by-default basis, but I would prefer if we could somehow re-architect to make this easy to do externally.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Rendering Drawing game state to the screen C-Feature A new feature, making something new possible D-Straightforward Simple bug fixes and API improvements, docs, test and examples S-Needs-Review Needs reviewer attention (from anyone!) to move forward X-Contentious There are nontrivial implications that should be thought through
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

2 participants