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

Create RWops with custom impl #626

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

sagebind
Copy link

@sagebind sagebind commented Apr 3, 2017

Add a builder type for creating custom implementations of RWops, and add a buffer-less method for creating a RWops from a Read + Seek. There might be more things to change using this, and we might be able to do more of the custom implementing at compile time instead with macros.

@Cobrand
Copy link
Member

Cobrand commented Apr 5, 2017

Pardon my french but what would be the use case for something like this ? Would you mind adding example(s) either directly in the doc as markdown or in the examples/ folder ?

I'd like to merge this but I must admit I have no idea what the use case would be. If you've implemented this, you surely must have a reason for this right, could you give me a hint as of what good does this do ?

Once that's settled and that I can test that I'd be happy to merge this

@sagebind
Copy link
Author

sagebind commented Apr 5, 2017

@Cobrand You are right, this is mostly copied from a game I'm working on and figured it would be useful to add upstream. The primary use-case is to support the RWops::from_stream() method, which allows you to create a lazy RWops from any Read type instead of reading the entire stream into a memory buffer first, like RWops::from_read() currently does. This is a big deal when loading large images or audio using SDL from something other than a file (like inside a zip file or in a database).

It would actually make more sense to change RWops::from_read() to be

pub fn from_read<R: Read + 'static>(reader: R) -> RWops<'static> {
    CustomRWopsBuilder::new(reader)
        .with_read(|reader, buf| reader.read(buf))
        .build()
}

since that would behave more how I would expect, but that would be a BC break. from_read sounds like it means, "Take this Read from Rust side and turn it into a RWops for the SDL side." Ideally we would have an ergonomic way of creating a RWops from something implementing any combination of Read, Seek, and Write, and provide the necessary callbacks for each capability.

I'm still not 100% happy with this implementation as it adds some extra runtime overhead, but I opened this PR early to start a discussion around it.

@Cobrand
Copy link
Member

Cobrand commented Apr 5, 2017

but that would be a BC break

don't sweat it, SDL2 is still pre-1.0 for this exact reason. Sometimes we can see mistakes like this one, and even though it breaks code for later versions, people must be prepared to face breaking change when they change their Cargo.toml file.

That being said, if there is no reason to break code don't break it, but if you think there is a valid and understandable reason like what you've said, feel free to propose such a change.

I can see now your use case, but I must say that I've not digged into how to use it code-wise yet. Examples would definitely be extremely useful as well. Reading from a zip or anything like that sounds like a very interesting topic, and I'm sure more people would be thrilled to have simple examples they can base themselves upon :).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants