Skip to content

Update bindings to SDL 2.0.20 #1213

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

Merged
merged 13 commits into from
Apr 25, 2022
Merged

Conversation

moralrecordings
Copy link
Contributor

@moralrecordings moralrecordings commented Mar 7, 2022

Notes:

  • Bindings were generated using SDL2_INCLUDE_PATH=path/to/SDL-2.0.20/include cargo build --features use-bindgen
  • I'm still fairly new to Rust development, so apologies for any amateur mistakes. Any feedback is welcome
  • New renderer features moved to WIP: Add new SDL_Renderer APIs #1216

@moralrecordings moralrecordings force-pushed the bindings_update branch 3 times, most recently from 89af657 to 9d26e50 Compare March 7, 2022 18:00
@Cobrand
Copy link
Member

Cobrand commented Mar 8, 2022

SDL_RenderGeometryRaw uses a buffer of SDL_Color; a type which is not wrapped (the pixels::Color code is from 2016 so I think it predates bindgen?). Would it be okay to refactor pixels::Color to use a wrapped type?

it's fine to refactor, but please keep the old type and mark it a "deprecated", and use a new type for a few versions. We don't want to break everything in one update.

The validation logic for InternalTexture::update_nv I nicked verbatim from InternalTexture::update_yuv, and I'm not sure if it works. My understanding from this page is that the UV plane in NV doesn't have half the stride like the individual U and V planes in YUV?

I'm extremely familiar with NV, so we will have to find people to test it for us. If we can't, label this part of the docs with UNTESTED UNSTABLE so that people that know what they are doing can guide us on the best way to implement this part.

There is a lot of code to cover here, so it might take some time to review everything.

@moralrecordings moralrecordings force-pushed the bindings_update branch 2 times, most recently from b8c2f11 to 1e53495 Compare March 10, 2022 12:07
@moralrecordings
Copy link
Contributor Author

Ack. Now I thnk about it, the wrapper for SDL_RenderGeometryRaw isn't going to work. The C version does everything with pointers and strides, so it supports the pattern of having one array per data type, and the pattern of having one array with different data types interleaved. My wrapper goes for option 1, but loads of things go for option 2. Maybe the solution is to make a second wrapper, which takes vertex: &[T] and a byte offset for each data type, then finangle Rust to somehow do the right thing...

@Cobrand
Copy link
Member

Cobrand commented Mar 14, 2022

Would you mind splitting this into 2 different pull requests? One for simply updating to 2.0.20 with the very simple new calls added (rumble, audio, joystick, window, etc); and then another entirely different PR for the video and renderer-related stuff (with Color as well)? This PR is very large, I'm thankful for all the work but it's really hard to follow what is intereconnected to each other and what is not here.

@kellerkindt
Copy link
Contributor

kellerkindt commented Mar 17, 2022

This is great @moralrecordings! I am using your branch locally with SDL 2.0.20 and the geometry-function (Canvas::geometry/SDL_RenderGeometry) works really well for me. It would be awesome if you could find the time for the PRs @Cobrand mentioned.

EDIT: ToColor impl is missing on RColor

@moralrecordings
Copy link
Contributor Author

Ack sorry, got distracted by other things! That sounds reasonable, I'll pare this back to just the basics and split the new geometry/render/colour features into a new PR.

Copy link
Member

@Cobrand Cobrand left a comment

Choose a reason for hiding this comment

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

Very minor changes requested, otherwise very nice work!

@moralrecordings moralrecordings requested a review from Cobrand April 13, 2022 03:39
@Cobrand
Copy link
Member

Cobrand commented Apr 13, 2022

@moralrecordings please cargo fmt, push and then once the CI checks there are no regressions we should be good to go.

@Cobrand Cobrand merged commit 7bae279 into Rust-SDL2:master Apr 25, 2022
@Cobrand
Copy link
Member

Cobrand commented Apr 25, 2022

Thanks!

This was referenced Mar 20, 2025
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.

3 participants